Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build: Fix flakey E2E tests #25852

Closed
shilman opened this issue Feb 1, 2024 · 4 comments · Fixed by #26072 or #26950
Closed

Build: Fix flakey E2E tests #25852

shilman opened this issue Feb 1, 2024 · 4 comments · Fixed by #26072 or #26950
Assignees
Labels
build Internal-facing build tooling & test updates

Comments

@shilman
Copy link
Member

shilman commented Feb 1, 2024

Describe the bug

As part of #25850 I had to disable a few E2E tests:

Before I disabled them I verified by hand that the features are working and it is a race condition in the tests and not a problem with Storybook. We should fix the tests before RC.

To Reproduce

Only one of the race conditions reproduces locally for me in Webkit. The rest can be reproduced by re-enabling the tests and observing CI.

System

No response

Additional context

No response

@shilman shilman added the build Internal-facing build tooling & test updates label Feb 1, 2024
@shilman shilman added this to the 8.0-RC milestone Feb 1, 2024
@kasperpeulen kasperpeulen reopened this Feb 20, 2024
@kasperpeulen
Copy link
Contributor

Update 20 februari:
I re-enabled them, as we thought they only where flaky in safari, and that we disabled safari all together in playwright.
But we actually still test safari in playwright, so that didn't fix it. 😅

So we should either:

  • debug what is going on in safari
  • or do disable safari in playwright

@vanessayuenn
Copy link
Contributor

Let's label these with @flaky as discussed at retro last week, for now. And I will leave this open to investigate these flaky tests as a future Empathy task.

@vanessayuenn vanessayuenn removed this from the 8.0-RC milestone Feb 26, 2024
@vanessayuenn vanessayuenn added this to the 8.0-Stable milestone Mar 4, 2024
@shilman shilman modified the milestones: 8.0-Stable, 8.0-nice to have Mar 7, 2024
@vanessayuenn vanessayuenn removed this from the 8.0-nice to have milestone Mar 7, 2024
@vanessayuenn vanessayuenn assigned JReinhold and shilman and unassigned shilman and JReinhold Apr 9, 2024
@JReinhold
Copy link
Contributor

Some findings on the tags-test:

  1. This is not the test being flaky, it's the implementation that is actually bugged in Safari. Building the Storybook and opening it Safari will occasionally show dev-only and docs-only stories in the sidebar that should have been filtered out.
  2. The original hypothesis that this was caused by a race condition with setting window.TAGS_OPTIONS is false. Logging that variable just before the it's read shows that it's there even when bug appears.
  3. Our best guess right now is that the setFilter function is either called too late or too early in a race with whatever uses the filter. This still doesn't explain why this is only a problem in Safari.

@yannbf
Copy link
Member

yannbf commented Apr 19, 2024

More findings on tags-test:

  1. It's definitely a bug and not a flake.
  2. This happens to any project, regardless of sandbox or anything. As long as it's in Safari.
  3. Seems like the implementation for the Store component has some inconsistencies in its setState method, where it should be async and it actually isn't (internally).
  4. There is a timing issue when the index is fetched and then the internal store state is set, where sometimes the state does contain the filters, sometimes it won't

I have a branch with logs that help understand what's going on: https://github.com/storybookjs/storybook/compare/yann/investigate-safari-flakiness?expand=1

If you run a sandbox on that branch and take a screenshot of the logs, then refresh until the bug happens, and compare, you will see the timing issue.

I couldn't find a solution. Seems like it's a quite delicate thing and potentially there might be other errors related to the store state sync situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates
Projects
Archived in project
6 participants