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

Disable flaky undo spec #1414

Closed
wants to merge 1 commit into from
Closed

Disable flaky undo spec #1414

wants to merge 1 commit into from

Conversation

bytasv
Copy link
Contributor

@bytasv bytasv commented Dec 5, 2022

Since the spec randomly fails in CI, I've decided to disable it until I can find a way to fix it

@bytasv bytasv added the core Infrastructure work going on behind the scenes label Dec 5, 2022
@bytasv bytasv requested a review from a team December 5, 2022 08:15
@bytasv bytasv self-assigned this Dec 5, 2022
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why not leave it enabled, as a forcing function for us to fix?
  2. Normally there is test.skip for this

edit

When I hover over page.waitForTimeout(600); I see:

Note that page.waitForTimeout() should only be used for debugging. Tests using the timer in production are going to be flaky. Use signals such as network events, selectors becoming visible and others instead.

Could be related?

@bytasv
Copy link
Contributor Author

bytasv commented Dec 5, 2022

Why not leave it enabled, as a forcing function for us to fix?

I don't mind leaving it enabled. I'm currently working on this and trying to figure out why it's caused anyways (so no need for extra motivator to get this fixed). My main motivation of disabling it was to get rid of failing CI runs which can prevent other PRs from being merged. Given it takes ~15-20 mins for CI to run and fail personally for me this might be annoying, but if you are fine with it I definitely don't mind leaving it as is

Could be related?

Could be, I will try to mock timers if that's possible, but I need to actually WAIT without any signals, because that's the duration after throttle should be finished. Anyways, once I find the solution or find a root cause I will share it with the rest, maybe that's something to look for in the future as well 🤷

-edit: @Janpot I've changed comment out to use test.skip (for some reason I though that we have a lint rule which forbids using .skip..)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 5, 2022
@Janpot
Copy link
Member

Janpot commented Dec 5, 2022

Superseded by #1415

@Janpot Janpot closed this Dec 5, 2022
@Janpot Janpot deleted the disable-flaky-spec branch April 21, 2023 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants