playwright: fix race conditions causing flakiness - #4754
Merged
regexowl merged 14 commits intoAug 14, 2026
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #4754 +/- ##
==========================================
- Coverage 77.90% 77.81% -0.09%
==========================================
Files 264 264
Lines 7069 7063 -6
Branches 2597 2565 -32
==========================================
- Hits 5507 5496 -11
- Misses 1465 1473 +8
+ Partials 97 94 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
The wizard asks the server whether a blueprint name is already taken. That answer used to live in a useState inside useDetailsValidation, filled in later by a useEffect that waited 300ms and then called the API. Two things go wrong when you keep server data in useState. First, state belongs to one component. Three components call useDetailsValidation, so there were three copies of the answer, three timers, and three identical requests for the same name. Second, state does not know which request it came from. Type a name, then change it, and two requests are in flight at once. Whichever one finishes last wins, even if it is the older one. The wizard could settle on the answer for a name you had already replaced, and it would stay that way, because nothing re-runs the effect to correct it. Ask the query for the debounced name instead. RTK Query keys its cache by the argument you pass it, so every component asking about the same name shares one cache entry and one request, and an answer for an old name can never overwrite the answer for the current one. While the answer is still on its way, nothing is known to be wrong yet. That is not the same as a validation error, so report it separately as isPending. Before, a pending check made disabledNext true, and the footer reads disabledNext as "there are errors to reveal". It revealed nothing, refused to advance, and left the button enabled. Clicking it did nothing at all. Now the footer disables Next until the answer arrives, so the button is never enabled and inert. Skip the request entirely when the name has not been edited. A name that already belongs to this blueprint cannot collide with anything.
The wizard resets its form the first time it opens. A useRef held the flag that says "this already happened", so that the reset would not run twice. A ref survives re-renders, but it does not survive unmounting. When React unmounts a component it throws away everything inside it, refs included. Mount it again and you get a brand new ref, back to false. That matters here, because insights-chrome unmounts and remounts this whole module as a matter of routine - roughly twice per page load, in every run we measured. Usually it happens while the page is still loading and nobody notices. Occasionally it happens after you have opened the wizard and started typing. The fresh ref then says "not initialized yet", the reset runs a second time, and the name and description you entered are replaced by the defaults. Redux state is not stored inside the component, so it survives the remount. Keep the flag there and the second mount can see what the first one did. Closing the modal already resets that slice, so the flag resets along with it. The symptom looked like several unrelated bugs, because which fields were lost depended only on what you had typed before the remount.
If the API ever reports a non-zero count alongside an empty data array, indexing the first element throws while rendering. There is no error boundary above this, so a TypeError here takes down the whole page rather than the one field that could not be validated. This is the same shape as a686d33, where an empty share_with_accounts list turned into a crash on the images table for anyone whose account happened to contain such an image.
The helper matched an existing repository on its name alone, so a repository left behind with the wrong url or snapshot setting was reused indefinitely. RepeatableBuild asserts that its repository is rejected for having no snapshots, and a reused repository with snapshots enabled fails that assertion on every run, looking like a product bug rather than stale fixture data. It also returned as soon as the create call succeeded. Content sources introspects a new repository asynchronously, and until that finishes the wizard disables it with "we are still learning about it" instead of the reason the test is asserting on. Verify the url and snapshot setting match what was asked for, replacing the repository when they do not, and wait for introspection to finish before returning.
Content sources allows one repository per url per organization. Repositories.spec claims its url by deleting whatever already holds it, and it used the same url as RepeatableBuild - so whenever the two overlapped, it deleted RepeatableBuild's repository mid-run. RepeatableBuild would list the repository, assert it was disabled, and then fail on the next assertion once the list refetched without it. The symptom was a dropdown reading "No repositories found", which looks like a product bug rather than another test deleting the fixture. Every other spec already owns its url outright. Give this one its own, and wait for the new repository to be introspected before using it, since the wizard filters the list by architecture and version and a repository does not qualify for either until introspection finishes.
Specs assert on dates as the wizard renders them. The browser followed the host clock, so a date stored as UTC midnight rendered as the previous day anywhere west of Greenwich: RepeatableBuild asserts "Dec 24, 2025" and saw "Dec 23, 2025". CI runners are already UTC, so this only ever failed on a developer's machine - the least useful place for a test to disagree with CI.
lucasgarfield
force-pushed
the
playwright-tests-race-condition
branch
from
August 13, 2026 18:44
f31acaa to
f8f32a0
Compare
Races between the browser and the API are only a few hundred milliseconds wide, so reproducing one means getting a test to click inside that window by luck. PW_CHAOS=1 widens the window on purpose. Uniform slowness finds nothing, because Playwright waits for elements to become actionable and simply absorbs it. Reordering is what breaks code that assumes replies arrive in the order they were sent, so delays come from a heavy tailed distribution: most requests pass straight through and a small share are held long enough to overtake a request issued earlier. Delays are capped below actionTimeout so slowness alone cannot fail a test, which keeps every red run meaningful. Each test records the seed it used, so a failure can be replayed rather than fished for again. Off unless PW_CHAOS is set.
createBlueprint used isVisible() to decide whether the save-and-build modal had appeared. isVisible() ignores the timeout passed to it and reports the state at that instant, so a modal that took a moment to paint read as absent. The second click never happened, the blueprint was never created, and the test failed later at the blueprint list - nowhere near the cause. Use waitFor(), which honours the timeout. trace: 'on' also kept a trace for every passing test, hundreds of megabytes of artifacts nobody opens. 'retain-on-failure' matches the existing video setting and keeps a trace whenever there is a failure to look at, including locally where retries are 0 and 'on-first-retry' would record nothing at all. The UI tests project raised the per test timeout to 29.5m, which only governs how long a hung test burns before it is killed - a third of globalTimeout. The slowest attempt observed is under 3m.
The default has only ever gone up: 5s out of the box, raised to 30s in 34fdd8f and to 50s in ff2cc68, both times in response to flakes. A longer wait has never fixed a race, it just makes each failure slower and buries the signal - the blueprint name race lived under toBeEnabled({ timeout: 10000 }) guards for months. It also inverted the meaning of the explicit timeouts at call sites. Most of them ask for 10s or 30s, which was an extension when written against a 5s default and is a reduction against 50s. Nothing at a call site tells you what the effective wait is any more. Assertions that genuinely wait on slow work - image builds, policy loading, introspection - already say so explicitly, and those are unaffected. A low default leaves those as deliberate statements rather than noise against a background where everything waits 50s. The full UI suite passes at 15s with no assertion needing more.
Three guards asserted a button was enabled immediately before clicking it. click() already waits for actionability, so the assertion only changed which error appeared on failure. One of them came in with dfaccff, "playwright: Improve flakey behaviour", and the blueprint name race it was working around is fixed. The rest are explicit expect timeouts of 5s and 10s. Those were extensions when written against Playwright's 5s default, but the default has since been raised twice, so they had quietly become reductions - the opposite of what their authors asked for. Removing them restores the intent. Left alone: short timeouts on waitFor and click, where being short is the point, since they probe whether an optional element is present rather than wait for one that should be.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="playwright/helpers/apiHelpers.ts" line_range="218-222" />
<code_context>
- );
- if (existing) {
- return existing;
+ if (!existing || !matchesRequest(existing, repository)) {
+ try {
+ await createRepositoryViaApi(page, repository);
+ } catch {
+ // A concurrent run may have created it in the meantime.
+ if (!(await findRepositoryByName(page, repository.name))) {
+ throw new Error(
</code_context>
<issue_to_address>
**issue:** Concurrent creation fallback may return a repository that does not match the requested configuration.
In the catch block we treat any repository with the same name as success:
```ts
} catch {
// A concurrent run may have created it in the meantime.
if (!(await findRepositoryByName(page, repository.name))) {
throw new Error(...);
}
}
```
But a concurrent run could have created a repo with the same name but different URL or `snapshot` settings, breaking the `ensureRepositoryExists` contract and causing subtle test failures.
It would be safer to reuse `matchesRequest` here (e.g., re-check the found repo and delete or throw if it still doesn’t match the requested configuration).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+218
to
+222
| if (!existing || !matchesRequest(existing, repository)) { | ||
| try { | ||
| await createRepositoryViaApi(page, repository); | ||
| } catch { | ||
| // A concurrent run may have created it in the meantime. |
There was a problem hiding this comment.
issue: Concurrent creation fallback may return a repository that does not match the requested configuration.
In the catch block we treat any repository with the same name as success:
} catch {
// A concurrent run may have created it in the meantime.
if (!(await findRepositoryByName(page, repository.name))) {
throw new Error(...);
}
}But a concurrent run could have created a repo with the same name but different URL or snapshot settings, breaking the ensureRepositoryExists contract and causing subtle test failures.
It would be safer to reuse matchesRequest here (e.g., re-check the found repo and delete or throw if it still doesn’t match the requested configuration).
lucasgarfield
changed the title
ensureAuthenticated waited for the app heading and treated a timeout as proof the user was logged out. It then called login(), which looked for a "Red Hat login" textbox on a page that was already the console, and failed 30 seconds later with a missing login field. So any reason the app was slow to render arrived as evidence that authentication was broken. A stage 502, a chrome-service outage, a saturated dev proxy and a landing page that simply took longer than 30s have all been reported that way, and each cost time before anyone looked past the message. Wait for the app or the login form, whichever appears, and log in only when the login form is the thing that showed up. When neither appears, say that the app did not render and point at the console and network log rather than at SSO. This narrows the budget for a genuinely absent login form from 60s to 30s. The extra 30s was never deliberate - it came from spending the first timeout on the wrong question.
A blank landing page usually means the federated module never mounted, and a reload recovers it often enough to be worth trying. The previous code navigated a second time as a side effect of falling through to the login flow, so this restores that attempt deliberately rather than as a consequence of guessing wrong. Also say which layer failed. If the console chrome rendered then image builder failed to mount, and if it did not then the problem is upstream of this repository entirely. Those get chased in different places, and the message now distinguishes them instead of leaving it to whoever opens the trace.
A timed out assertion can only say what it was waiting for, never why the thing never arrived. A blank page, a crashed module and a slow backend produce the same message, so working out which one it was has meant reading the trace by hand, and often asking someone to do it. Record uncaught exceptions, console errors and warnings, failed requests and any response of 400 or above, and attach them to tests that fail. Passing tests attach nothing. On a landing page that never rendered this immediately shows the 404 on fed-modules.json, which is the manifest chrome needs to mount any micro frontend - the difference between "our app is broken" and "chrome could not load anything" without opening the trace at all.
The diagnostics fixture gives warnings their own smaller cap so a burst of them cannot crowd errors out of the attachment. The console handler never said which budget it was drawing from, and the default is the error budget, so every warning was charged there and the warning cap was never consulted. Found reviewing the branch; the attachment content was always right, only the cap routing was wrong.
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
2 participants
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.
I think that this might help quite a bit with the flakiness in the Playwright tests.
To simulate a degraded stage environment locally, I created (and full disclosure, everything in this PR was done with Claude) a chaos testing middleware for Playwright. It simulates API responses coming back out of order which causes failures when there are race conditions.
The first commit fixes about 90% of the flakes. The tl;dr is that it was related to validating the uniqueness of blueprint names – this had a three state (true, false, in flight) that was treated as a boolean and some crufty debounce code that resulted in a race condition on advancing the blueprint.
The other changes all came about from other less common flakes that cropped up during the chaos testing.
I still probably didn’t manage to catch everything but the chaos testing was so useful that I went ahead and committed it so that we can use it again the next time we start to observe flakes.
I’ve watched this run a few times now in the CI on this PR and not only is Playwright passing every time, it’s passing with 0 flakes (apart from the first run where it did flake, but then I used the chaos testing tool to reproduce and fix that flake, too!).
With the flakes hopefully sorted I went ahead and also tightened up the timeouts in Playwright – over time, in response to flakes, we’d added a lot of little increased timeouts here and there and increased the global timeout from 5 seconds to 50 seconds!