Stop releases failing browser tests that pass everywhere else - #53
Merged
Conversation
Three separate failures during today's releases were one bug wearing different hats: an assertion that reads the DOM once, against a dashboard that re-renders asynchronously. `Assert.Equal(14, await rows.CountAsync())` takes whatever is on screen at that instant, so on a runner already busy with eleven hundred unit tests across two frameworks it caught the board half-built and reported a broken feature. Playwright's own assertions retry until the page settles or the timeout expires, which is what these always wanted. Twenty-four such reads become `Assertions.Expect(...)`. The readiness signal moves with them: waiting for the first row only proves the list has started, so `WaitForTheBoardAsync` waits for every checker before a test may read anything, which is what makes the counts the remaining tests compare safe at all. Three fixed sleeps go: two guessed at a theme switch, and the third guessed at a write to the state provider, where the event-log entry that write produces is the real signal. Failures are diagnosable now too. Each test gets its own browser context with tracing on, and a test that fails keeps its trace; CI uploads them from both the pull-request and the release workflow. A browser failure that happens only on a runner was a stack trace and a guess -- three guesses, as it turned out. It is now a zip with the DOM, the network and a screenshot at the moment it gave up.
It was added with one call site and left unused everywhere else, so four tests still clicked a row and then typed, clicked RUN NOW, or read the interval picker without waiting for the panel to be showing the checker they had just selected -- which is the exact failure its own remarks describe. The comment explaining why moves onto the helper, where it now covers every caller.
Three tests still opened by counting the rows on screen and comparing against that number later. With `WaitForTheBoardAsync` in front of them the number is known, so the reads go and the assertions state what the sample actually declares -- which is also what fails usefully if a row goes missing, rather than comparing one wrong number against itself. Also drops a stray BOM from two files and clears a page's captured console errors when its test finishes, alongside the context it belongs to; they were held for the whole run before.
`OnceDisposed_ItStopsCounting` failed a full-suite run: expected 3, got 4. Not the feature -- the test. A `MeterListener` subscribes to the meter by name, so it counts every checker in the process, and the count it took before calling Dispose was overtaken by a check another test ran in the gap. The assertion then read that difference as a listener still attached. Taking the number after disposal makes the very thing under test the thing that freezes it, so concurrent traffic cannot move it -- while a Dispose that failed to detach still does, which is how this was checked: neutering Dispose turns the test red.
This is what actually made releases fail browser tests that were green on
every pull request. `ci.yml` gives the unit suite and the browser suite a
runner each; the release ran one solution-level `dotnet test`, which hands
both to the same machine at the same time.
Reproducing that locally failed four browser tests, and the traces say why:
Connection disconnected with error
'WebSocket closed with status code: 1006 (no reason given).'
The browser suite drives a Blazor Server circuit over a WebSocket while
eleven hundred unit tests across two frameworks saturate the runner. A
starved circuit closes, and from that moment nothing on the page can
change -- so every assertion waits out its timeout and reports the element
it was watching, which reads as a broken feature. No amount of retrying
helps; the page is dead.
Two sequential steps instead. Both suites still gate the release, they
just no longer compete. Run separately the same machine passes both: 53/53
browser, 558/558 unit on each framework.
A failing browser test now also prints the console errors it collected
straight into the CI log, so the next dropped circuit says so in the run
output rather than only in a downloaded trace.
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.
The browser suite needed three separate fixes during today's releases. Rather than a fourth patch,
this goes after why a suite that is green on every pull request keeps failing during a release.
The asymmetry nobody was looking at
ci.ymlgives the unit suite and the browser suite a runner each. The release ran onesolution-level
dotnet test, handing both to the same machine at the same time.Reproducing that locally failed four browser tests. The traces say exactly why:
The browser suite drives a Blazor Server circuit over a WebSocket while eleven hundred unit tests
across two frameworks saturate the runner. A starved circuit closes, and from that moment nothing on
the page can change — so every assertion waits out its timeout and then reports the element it was
watching, which reads as a broken feature. No amount of retrying helps; the page is dead.
publish.ymlnow runs the two suites as sequential steps. Both still gate the release, they just nolonger compete.
The other half: assertions that read once
That takes whatever is on screen at that instant, against a dashboard that re-renders
asynchronously — so a busy machine catches the board half-built. Playwright's
Assertions.Expect(...)retries until the page settles, which is what these always wanted.
OpenDashboardAsyncwaited for the first row, whichonly proves the list has started.
WaitForTheBoardAsyncwaits for every checker before a test mayread anything, which is what makes the few remaining plain counts safe at all.
CheckerCountnamesthe number once.
provider, where the event-log entry that write produces is the real signal.
SelectCheckerAsyncis used where it was written for — it had one call site and was leftunused, so four tests still clicked a row and then immediately typed, clicked RUN NOW, or read the
interval picker.
So the next failure is diagnosable in one pass
Each test gets its own
IBrowserContextwith tracing on. A failing test keeps its trace; a passingone does not.
ci.ymlandpublish.ymluploadplaywright-traceson failure — open the zip attrace.playwright.dev. A failing test also prints the console errors
it collected straight into the CI log, so the next dropped circuit says so in the run output without
anyone downloading anything.
That is not speculative: it is how the 1006 above was found, minutes after the mechanism existed.
One unit test came along
Verifying under load turned up
OnceDisposed_ItStopsCountingfailing — expected 3, got 4. Not thefeature: a
MeterListenersubscribes to the meter by name and counts every checker in the process,so the count taken before
Disposewas overtaken by a check another test ran in the gap. It nowreads the count after disposal, where the thing under test is the thing that freezes it.
Verification
Both new mechanisms were proven by making them fail, not by being read:
Assert.Failproduced exactly two named trace zips from its two failing cases, andnone from the two that passed.
The browser reported errors before this failed:in therun output.
MeterMetricsInsights.Disposeturns the metrics test red (expected 1, actual 2), so itstill catches a listener left attached.
No library code changed — tests and workflows only.