Skip to content

Make tests less flaky #501

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

Merged
merged 6 commits into from
Mar 12, 2021
Merged

Make tests less flaky #501

merged 6 commits into from
Mar 12, 2021

Conversation

GrygrFlzr
Copy link
Member

@GrygrFlzr GrygrFlzr commented Mar 12, 2021

Combines most .click tests with .waitForNavigation, which stabilizes the tests somewhat.

@GrygrFlzr GrygrFlzr changed the title (Attempt to) make tests less flaky Make tests less flaky Mar 12, 2021
@GrygrFlzr GrygrFlzr merged commit ea63518 into master Mar 12, 2021
@GrygrFlzr GrygrFlzr deleted the better-tests branch March 12, 2021 09:52
@Conduitry
Copy link
Member

Running the promises in parallel sounds like it'd be asking for another race condition. We shouldn't be awaiting the page navigation promise once we've already awaited the click promise?

@GrygrFlzr
Copy link
Member Author

GrygrFlzr commented Mar 12, 2021

This was the recommended solution from the playwright docs and was actually to avoid a race condition:

Clicking an element could trigger asynchronous processing before initiating the navigation. In these cases, it is recommended to explicitly call page.waitForNavigation([options]). For example:

  • Navigation is triggered from a setTimeout
  • Page waits for network requests before navigation
// Note that Promise.all prevents a race condition
// between clicking and waiting for a navigation.
await Promise.all([
  page.waitForNavigation(), // Waits for the next navigation
  page.click('a'), // Triggers a navigation after a timeout
]);

Presumably because it's possible for the click to finish processing before we call waitForNavigation(), in which case it'll just time out after 30 seconds.

I also ran the CI on ubuntu + macOS on node 12 + 14 + 15 and it succeeded three times in a row, so either I got really lucky or it's slightly better off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants