Reduce flakiness in several tests #1487
Conversation
These are the tests and the source and fix for flakiness for each: "does not fire turbo:load twice after following a redirect" src/tests/functional/navigation_tests.js:451 PROBLEM: The the event can sometimes arrive slightly before the check. FIX: By counting the events over the whole scenario is more robust than checking that it happens at a specific moment and more closely matches the purpose of the test. "link method form submission targeting frame submits a single request" src/tests/functional/form_submission_tests.js:1019 PROBLEM: Redirect accounting differs between browsers and Firefox sometimes won't count the redirect as a request, returning just 1. FIX: Stop checking the redirect by counting requests. Instead assert that redirect happened by checking that the page contains the content from the redirect target. And then directly that we did a single request (as the test describes) by counting only the requests. "successfully following a link to a page without a matching frame shows an error and throws an exception" src/tests/functional/frame_tests.js:152 PROBLEM: The assertion on the error can run before the error setting callback runs. FIX: Wait for the error to be set and the page click to finish before continuing with assertions. "failing to follow a link to a page without a matching frame shows an error and throws an exception" src/tests/functional/frame_tests.js:181 PROBLEM: The assertion on the error can run before the error setting callback runs. FIX: Wait for the error to be set and the page click to finish before continuing with assertions. "reloads when tracked elements change" src/tests/functional/rendering_tests.js:48 PROBLEM: Since we're triggering a full page reload it is possible to try to evaluate on the page at the moment of full page navigation. This can then lead to a race condition where you trigger a check on the page as it is being navigated away and the actual evaluation happens when its context was already destroyed. This then throws "Execution context was destroyed, most likely because of a navigation". This happens when trying to wait for `turbo:load` event to assert that navigation has completed. FIX: Use the native playwright page.waitForURL to wait for the new page to be loaded. This method doesn't rely on page evaluation and is resistant to navigation issues.
| expect(fetchOptions.method, "[data-turbo-method] overrides the GET method").toEqual("POST") | ||
| expect(requestCounter, "submits a single HTTP request then follows a redirect").toEqual(2) | ||
| await expect(page.locator("#hello h2")).toHaveText("Hello from a frame") | ||
| expect(requestCounter, "submits a single HTTP request").toEqual(1) |
There was a problem hiding this comment.
PROBLEM: Redirect accounting differs between browsers and Firefox sometimes won't count the redirect as a request, returning just 1.
FIX: Stop checking the redirect by counting requests. Instead assert that redirect happened by checking that the page contains the content from the redirect target. And then directly that we did a single request (as the test describes) by counting only the requests.
| const [error] = await Promise.all([ | ||
| page.waitForEvent("pageerror"), | ||
| page.click("#missing-frame-link") | ||
| ]) |
There was a problem hiding this comment.
PROBLEM: The assertion on the error can run before the error setting callback runs.
FIX: Wait for the error to be set and the page click to finish before continuing with assertions.
| const [error] = await Promise.all([ | ||
| page.waitForEvent("pageerror"), | ||
| page.click("#missing-page-link") | ||
| ]) |
There was a problem hiding this comment.
PROBLEM: The assertion on the error can run before the error setting callback runs.
FIX: Wait for the error to be set and the page click to finish before continuing with assertions.
| await nextEventNamed(page, "turbo:load") | ||
| await nextBeat() | ||
|
|
||
| expect(await page.evaluate(() => window.turboLoadCount)).toEqual(1) |
There was a problem hiding this comment.
PROBLEM: The the event can sometimes arrive slightly before the check.
FIX: By counting the events over the whole scenario is more robust than checking that it happens at a specific moment and more closely matches the purpose of the test.
|
|
||
| await page.click("#tracked-asset-change-link") | ||
| await nextEventNamed(page, "turbo:load") | ||
| await page.waitForURL("**/tracked_asset_change.html") |
There was a problem hiding this comment.
PROBLEM: Since we're triggering a full page reload it is possible to try to evaluate on the page at the moment of full page navigation. This can then lead to a race condition where you triggera check on the page as it is being navigated away and the actual evaluation happens when its context was already destroyed. This then throws "Execution context was destroyed, most likely because of a navigation". This happens when trying to wait for turbo:load event to assert that navigation has completed.
FIX: Use the native playwright page.waitForURL to wait for the new page to be loaded. This method doesn't rely on page evaluation and is resistant to navigation issues.
1 participant
I decided to look at some of the tests that are flaking. The selection of tests which I improved are a bit random because I basically ran the test suite a few times on my machine and observed which ones happened to flake and then looked into which ones I can fix.
The commit message has all of them listed together with the explanation of the problem and the fix but for easier PR review, I'll copy paste the problem and fix as a comment under each test.
These are not all of the flaky tests, it's just the ones I managed to fix in this coding session. When I'll have more time I'll look at a few more and open a new PR.