Conversation
## Because - `misc/cookiesDisabled.spec.ts` "visit verify page with localStorage disabled" fails at random on the Playwright PR job. It blocks the merge gate, then passes on a rerun. It hit #21061, #21067 and #21071 on the same day. - The assertion gave `.card-header` a 500ms visibility budget. A loaded CI container needs longer than that to paint the element. - The budget was also redundant. `pages/cookiesDisabled.ts` already waits with `header.waitFor()`, so the 500ms only capped a wait that was already correct. ## This pull request - Removes the explicit `timeout: 500` from the header assertion. - Awaits the page object helper instead, so its own `waitFor()` governs the wait. This is the form the sibling test at line 29 has always used, on the same element, and that call site has not been reported flaky. - Rewrites the comment above the assertion. It still credits FXA-9323, which correctly replaced `waitForTimeout`, and now says why the 500ms budget went away. ## Issue that this pull request solves Closes: https://mozilla-hub.atlassian.net/browse/FXA-14384
Contributor
There was a problem hiding this comment.
Pull request overview
Removes a flaky 500ms visibility timeout from the cookies-disabled functional test.
Changes:
- Uses the standard Playwright assertion timeout.
- Aligns the assertion with the sibling test.
- Updates the timeout rationale comment.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| timeout: 500, | ||
| }); | ||
| // FXA-9323 replaced waitForTimeout here, but its 500ms budget was too short | ||
| // for CI. The page object already waits, so let its waitFor() govern. |
clouserw
approved these changes
Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because
misc/cookiesDisabled.spec.ts"visit verify page with localStorage disabled" fails at random on the Playwright PR job. It blocks the merge gate, then passes on a rerun. It hit fix(rate-limit): match ignoreEmails against the non-normalized email #21061, refactor(content-server): remove Backbone reset password routes #21067 and test(functional): assert the de l10n bundle is delivered #21071 on the same day..card-headera 500ms visibility budget. A loaded CI container needs longer than that to paint the element.pages/cookiesDisabled.tsalready waits withheader.waitFor(), so the 500ms only capped a wait that was already correct.This pull request
timeout: 500from the header assertion.waitFor()governs the wait. This is the form the sibling test at line 29 has always used, on the same element, and that call site has not been reported flaky.waitForTimeout, and now says why the 500ms budget went away.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-14384
Checklist
Put an
xin the boxes that applyHow to review (Optional)
packages/functional-tests/tests/misc/cookiesDisabled.spec.tsScreenshots (Optional)
No user interface change.
Other information (Optional)
I did not run the spec locally. It needs a live stack, so CI is the check.
What I did run:
npx tsc -p packages/functional-tests/tsconfig.json --noEmit. It reports the three errors that already exist on main, inlib/sub-helpers.tsandlib/testAccountTracker.ts. This change adds none.npx nx lint functional-tests. Exit 0, no errors.The test box above is unticked on purpose: the test changed, but I could not run it here.