test(system): a response wait only where it is the evidence - #1135
Merged
Conversation
Two flakes in two runs, on branches touching neither the frontend nor the code under test: EventPageSystemTest timing out on a DELETE and RecoveryManagerPageSystemTest on a restore. Both wrapped a click in page.waitForResponse and then asserted the real outcome on the next line, so the wait was a weaker second copy of that assertion — and it is the copy that fails, because it caps the round trip on the browser's view of a request whose success re-renders the element the click was on. The rule is written down in Polling.kt, beside the budget every wait shares, and all 62 call sites were checked against it. Nineteen were duplicates and go. The rest stay, for two reasons the sweep had to learn: the response carries evidence nothing else does — a guest access token header, an error body — or something touches the page before the assertion does, a navigation or a second form, where the wait is what stops the request racing it. Removing the waits exposed one real race rather than causing it: the membership rendezvous writes the row and the MEMBER role separately, and the test polled for the row and then asserted the role, which the wait had been padding. The poll now waits for both. Closes #1125
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.
Closes #1125.
Why
Two flakes in two runs, on branches that touch neither the frontend nor the code under test:
Both wrapped a click in
page.waitForResponseand then asserted the real outcome on the very next line — the sign-up gone from the database, the account back in the pane. The wait was a weaker second copy of that assertion, and it is the copy that fails: it caps the round trip at the shared five-second budget, on the browser's view of a request whose success re-renders the element the click was on.What this achieves
The rule is written down and the suite follows it. 19 of the 62 call sites were duplicates and go; the assertion that follows each one is unchanged and now gets the full budget instead of what the wait left it.
How
The rule lives in
Polling.kt, beside the budget every wait already shares, because that is the file a test reads when it is choosing how to wait.A wait earns its place when the response is the evidence —
EventPageSystemTestreads anx-guest-access-tokenheader,ActivationPageSystemTestreads an error body,EventEditPageSystemTestasserts the banner is fetched at its own public address and nothing observable follows. It also earns it when something touches the page before the assertion does: a navigation, another pane opened, a second form filled. There the wait is what stops the request racing whatever comes next.It does not earn it when the assertion is the next thing. Those 19 became a plain click followed by the
pollForor Playwright assertion that was always the real check.Worth a reviewer's attention
The sweep exposed one real race rather than causing it. The membership rendezvous writes the membership row and the
MEMBERrole separately;confirming the address after applying starts the membershippolled for the row and then asserted the role without polling, and the response wait had been padding the gap. Removing the wait turned that intoExpecting ["GUEST"] to contain ["MEMBER"]. The poll now waits for both facts, which is what the scenario is named for.I tightened the rule twice while applying it, because the first two passes were wrong in ways only running the suite showed: a variable read more than once is a response being asserted, and an interaction between the click and the assertion means the wait is synchronising. Both are in the written rule now.
Verification
./gradlew :tests:system:testagainst a compose stack on the CI images, every class the sweep touched —EventPageSystemTest,EventSignUpsPageSystemTest,ActivationPageSystemTest,AddressPageSystemTest,CommitteeManagerPageSystemTest,JobManagerPageSystemTest,RecoveryManagerPageSystemTest,MembershipSignUpPageSystemTest— 37 tests, 37 passed.An earlier, looser version of the sweep failed two of those tests, which is how the two extra conditions were found. The full six-shard suite runs here in CI.
Diff breakdown —
█added░removed, scaled to the largest row.