test(e2e): make the elision block report its real failure, not an off-by-one - #1221
Merged
Conversation
vivek7405
force-pushed
the
fix/deflake-elision-e2e
branch
from
August 3, 2026 14:13
f6d5d08 to
9904d78
Compare
vivek7405
changed the title
8 tasks
…lock The differential elision block intermittently red main. The two failing assertions blamed the wrong thing: "counter increments identically on vs off (on=4, off=3)" reads as a counter bug, and is really the OFF page never having hydrated, so its click landed on nothing. Behind it is an asymmetry that is easy to miss. Both servers transform modules on demand, but the ON server has been serving the rest of this suite for about a hundred tests by the time this block runs, so it paid that cost long ago. The OFF server is started in this block's own before() and its very FIRST request is the assertion. Measured on an idle 24-core box across three samples, that first request costs about 1840ms on OFF against about 1290ms on ON, because WEBJS_ELIDE=0 ships 13 modulepreloads to the ON build's 6. Against a fixed 2500ms wait that leaves roughly 660ms of headroom on a fast idle machine, before the page has fetched or run a single module. A slower or contended CI runner is where that margin goes, and OFF is the side it runs out on, which matches OFF being the side that always failed. I could NOT reproduce the failure locally on a clean machine: both the old and new versions pass repeatedly, including pinned to two cores. So this is a fix for a measured margin rather than a reproduced fault, and the numbers above are what justifies it, not a red run. Warm the OFF server in before(), where nothing is timed, and replace every fixed sleep with the condition it was standing in for: hydration waits on the counter actually being rendered, the post-click assertion waits for the value to settle (the re-render is batched to a microtask, so it is not readable synchronously), and the static route waits on load since it has no component to hydrate at all. Also make clickCounterButton throw instead of optional-chaining into a no-op. A silent miss is what turned "the page was not ready" into a baffling off-by-one two assertions later, and this helper is shared with five other tests that would mislead the same way.
The hydration wait reported a bare puppeteer "Waiting failed: 15000ms exceeded", which names neither which page stalled nor what was being waited for. That is the same diagnostic dead end as the off-by-one counter assertion it replaced: it tells you a thing did not happen without telling you what. Report the state instead. Whether customElements.define ran, whether the host is present, whether the increment button rendered, and which side (ON or OFF) it was, plus the reading that matters: if define never ran, the component module was not served, which is what a wrongly-dropped elision verdict looks like from the browser. Also drop the wait from 30s to 15s. Warm hydration is single-digit milliseconds and a cold first request is under two seconds, so 15s is still an order of magnitude of headroom, and it halves how long a genuine failure takes to surface.
…y sent
Reproducing the failure changed what the fix has to be. The readiness
check I had waited on the host, the output and the increment button, and
every one of those is in the server's HTML before a line of JavaScript
runs:
<my-counter count="3" data-wj-host><!--webjs-hydrate-->
<button aria-label="Decrement">...<output>3</output>
<button aria-label="Increment">...
So it was waiting on something already true. A click at that point lands
on a real button with no listener attached and is silently swallowed,
which is exactly the reported "on=4, off=3": not a counter that counted
wrong, a counter whose module never ran.
Blocking the counter module reproduces it deterministically, and that is
also what a wrongly-elided component looks like from the browser. Under
the old fixed wait the counter goes 3 to 3 and the suite reports the
off-by-one with no hint of a cause. Waiting on the element INSTANCE being
upgraded, tested with instanceof against the registered constructor,
fails instead, and fails naming the page and which of define / host /
upgrade did not happen. Before upgrade the element is a plain HTMLElement;
only after is it an instance of the class, and that is the first moment
the @click binding exists.
One earlier claim in this branch was wrong and is worth correcting: a
delayed module does NOT reproduce this. Module scripts are deferred, and
DOMContentLoaded waits for deferred scripts, so goto(domcontentloaded)
already absorbs the whole module graph. Measured with a 4s delay injected
on the counter module, goto took 4076ms and the element was upgraded by
the time it returned. The failure needs a module that never runs, not one
that runs late.
The warmup navigated offPage twice in before() to pay the cold dev server's transform cost off the clock. It has to go, for two independent reasons. Its premise was already disproved on this branch. Module scripts are deferred and DOMContentLoaded waits for deferred scripts, so goto(domcontentloaded) absorbs the whole module graph on its own and there is no residual cost for a warmup to remove. Measured with a 4s delay injected on the counter module: goto took 4076ms and the element was upgraded by the time it returned. And it correlated exactly with a deterministic CI failure. All three pushes carrying it failed both e2e jobs with the OFF page reporting that customElements.define never ran, while main without it is green and the same code passes locally. I could not reproduce that locally, so I am not claiming the mechanism, only that driving extra navigations through the page under test buys nothing measurable and changes browser-side state the assertions then depend on. Unjustified machinery that correlates with a red build is not worth keeping on a guess. What stays is what is independently justified: waiting for the element to upgrade rather than for markup SSR already sent, waiting for the counter to settle after the click instead of sleeping, and a click helper that throws rather than silently no-opping.
…diagnostic) CI fails this deterministically with customElements.define never running on the OFF page, and it does not reproduce locally on any configuration I have tried. Rather than keep guessing at the mechanism, make the failure report what the page knows: whether the browser ever fetched counter.ts, what the server returns when asked again, what the boot script contains, and which component tags did register. Temporary. It comes out once the cause is identified.
It did its job: the elision-OFF boot intermittently 404s a module and leaves components inert, now tracked as its own bug with a reproduction. The permanent failure message already names the page and which of define / host / upgrade did not happen, which is what a reader needs. The rest (refetching the module, dumping resource timings and boot scripts) was scaffolding for that investigation and does not belong in a test that runs on every push.
Review found the block asserting things it could not observe. Five fixes.
clickCounterButton could not catch the miss it was written for. It threw
only when the button was ABSENT, and SSR emits the counter's buttons, so
on an un-hydrated page the button is present, click() runs, nothing
happens and nothing throws. That is the same trap the hydration wait had
and it survived in the helper. It now requires the element to be
UPGRADED, which is the first moment a listener exists, and names which of
registered / upgraded failed.
The post-click wait swallowed its own rejection, so a side that never
applied its click fell through to fail as "on=4, off=3": exactly the
misleading message this block is meant to stop producing. It now reports
which side stalled and what the value stuck on.
The snapshot compared chat-box's status line, which is driven by a
WebSocket handshake rather than by hydration ("Connecting…" until the
socket opens, then "Live · N others online"). The two pages talk to two
independent servers, so that text is a race in one direction and a mask
in the other, and no readiness wait can fix it because the sides are
genuinely independent. It is normalised out, like the wall-clock, since
it is not a function of elision.
The hydration diagnostic printed unobserved state as fact: when its probe
failed it still reported "define DID NOT run, host absent", sending the
next reader after the wrong bug. It now says what it could not determine.
It also relabelled every waitForFunction rejection as a suspected elision
defect; a detached frame or target crash is now rethrown as itself.
Two comments asserted things that are false: that the counter is the only
interactive element on the page (chat-box and theme-toggle are too), and
that the static route has no component to hydrate (its own docstring says
the boot re-emits the layout's theme-toggle). Corrected, and the before()
note no longer blames the warmup for a CI failure that recurs without it
and is tracked as #1228.
The previous commit normalised chat-box's status line out of the <main> snapshot, calling it content that is not a function of elision. Review showed that is wrong twice over. It is not independent of elision. chat-box calls connectWS from connectedCallback, so leaving its SSR 'Connecting…' state requires its module to have shipped and the element to have upgraded. The status is a real signal that chat-box hydrated on both sides, and normalising it threw away coverage in the very test that exists to compare hydration. And the normalisation did not even close the race it targeted. The server broadcasts the join to every client INCLUDING the joiner, and the handler appends a 'someone joined' line, so the socket opening changes the message pane as well: both the text AND the tag structure move. A regex over the status line left that untouched. So wait for the connected state on both pages before snapshotting, and normalise only the participant COUNT, which really is environmental (each page talks to its own server and counts whoever is connected to it). The 'Live' that proves hydration is kept. Also from the same review: the settle predicate read .textContent.trim() with no optional chain, so a missing <output> threw a TypeError that the new handler could not recognise as a timeout and rethrew raw, unlabelled, defeating the reporting it was added for. Every hop is chained now, so that case times out and names the side. The readiness docstring claimed the counter is "the component the assertions below actually drive", which is false for the snapshot test that drives nothing. And getCounterValue's JSDoc still described a shadow DOM component, contradicted by this file's own assertions two lines below it.
…s hydration Two rounds of review chased chat-box through the snapshot, each fix closing one facet and leaving others. The pattern was the tell: the pane cannot be made comparable at all, so it should not be compared. Why waiting cannot work. The pane accumulates a line per join and leave on that page's OWN server, each adding elements to the tag list. A reconnect re-opens and appends another. And the two servers are not comparable populations by construction: the OFF server is created fresh and privately in this block's before(), while the ON server is shared with the whole suite and outlives it, so anyone else connecting to it during the window adds a line with no OFF counterpart. These are independent populations, not one lagging the other, so no readiness wait converges them. The previous commit's wait also keyed on the status line, which onOpen sets BEFORE the join message lands, so it did not even close the race it claimed to. So hold the pane out of the comparison, on a clone so the live DOM is untouched. The chat-box ELEMENT is still compared, must be present and in the same position on both sides; only its feed is excluded. That loses no coverage worth having, because the thing worth asserting about chat-box is that it HYDRATED, not what its feed said. It connects from connectedCallback, so leaving the SSR state proves the module shipped and the element upgraded, and waitForChatLive now asserts exactly that on both sides. Its docstring says plainly that it is a hydration check and not a settle barrier, since the earlier version claimed a guarantee it did not provide. Also widen the wall-clock normaliser, which matched only a 12-hour rendering and so compared raw on any runner whose default locale is 24-hour.
…g the wait Two corrections from review of the previous commit. replaceChildren() on <chat-box> emptied its WHOLE render output, not the feed: the status header, the form, the input and the Send button went with it, so a structural difference in chat-box's rendering stopped being observable, and the 'Live' marker the commit before had deliberately kept was gone too. Four places said "only the pane". Now only the pane is actually excluded, addressed as the middle child of chat-box's wrapper (header div, pane div, form), with the participant count normalised in the header text as before. That selector is positional, because the markup carries no id or data hook, so it is asserted rather than assumed: if it stops matching, the snapshot fails loudly naming the cause instead of silently comparing the feed raw again, which would have restored the flake invisibly. Both snapshot call sites go through that check. waitForChatLive's docstring called itself a hydration check. It is hydration AND a successful websocket handshake, since it keys on the status line that onOpen sets. Hydration is the necessary half, but a red can equally be a dev server that never completed the upgrade, and stating only the first is the same misattribution this block exists to remove. The failure message already named both; the docstring now does too.
…states Three corrections from review. The pane guard only fired when the positional selector matched NOTHING, which is the less likely failure. Insert an element above the header, or swap header and pane, and the selector cheerfully returns the WRONG child: the count still matches, the guard stays green, the header is emptied instead of the pane, and the live feed goes back into the comparison. That is the flake restored invisibly, which is precisely what the guard's own comment claimed it prevented. It now pins the wrapper's child tag sequence ([div, div, form]), so an insertion or a reorder fails loudly. Verified by inserting a div above the header: the old guard passed, this one names the shape it found. Two comments still justified the pane exclusion with "the excluded pane is where an un-hydrated chat-box would have shown". That was true of the previous commit, which excluded the whole subtree; this one keeps the status header, so an un-hydrated side reading 'Connecting…' now diverges in the snapshot itself. Corrected, and waitForChatLive is described as the belt-and-braces check it now is rather than the only guard. The reinstated normaliser covered only 'Live · N others online', leaving 'Reconnecting…' raw. A socket that drops between the wait and the snapshot then diverges with no counterpart, since the two pages hold sockets to two independent servers. Both CONNECTED renderings now collapse to one token. 'Connecting…' is deliberately left alone: that is the never-connected state and it should still fail.
…dead branch Three corrections from review. The shape guard could not catch a header/pane reorder, which is half of what the previous commit claimed for it. Both are plain divs, so no tag sequence and no index distinguishes them: swap the two and the guard stays green while the header is emptied and the live feed returns to the comparison. I had verified only the insertion case and asserted both. The pane is now found by CONTENT, as the wrapper div that is not the one carrying the status line, with both roles asserted. The retained status header was credited with catching an un-hydrated side reading 'Connecting…'. That branch is unreachable. waitForChatLive runs on both pages before either snapshot and throws on a side still in the SSR state, and chat-box has no path back to it once open, so at snapshot time both sides are connected and the header collapses to the same constant on each. The coverage is real but it lives entirely in waitForChatLive; the comments now say so instead of claiming it twice. The guard's failure message reported a present-but-childless wrapper as 'no wrapper', an unobserved state printed as fact, which is the exact rule this block states a few lines above about not sending the next reader after the wrong bug. The two cases now report separately.
Four corrections from review. The content key was a substring test, so a chat MESSAGE containing 'Live ·' would look like a header. Message text is user-supplied and echoed back verbatim by the say broadcast, so that is reachable in principle. It would not mis-select, since the real header matches too and the guard trips on two headers rather than picking wrong, but it would be a spurious red in the block whose whole point is not producing those. The match is now anchored to the header's whole text, which is exactly one status string and nothing else. The guard's failure message named 'its markup changed' as the cause. A content collision is the other way to get there and leaves the markup untouched, so the message now offers both rather than asserting one it did not observe. The unreachability of the 'Connecting…' branch was justified by waitForChatLive running before every snapshot. It runs before the mixed-page snapshot only. The static-route test snapshots with no such wait, and the conclusion holds there for a different reason: that route renders no <chat-box>. Both comments now say which reason applies where. Also removed a sentence describing the old positional lookup, left behind by the previous commit and contradicted by the paragraph directly under it.
Six of seven review rounds went on chat-box, and every one found a defect the previous round's fix had introduced: a status normaliser that erased a hydration signal, a wait that fired before the thing it waited for, a guard that could not detect a reorder, a content key that a chat message could collide with. All in a widget this block does not test, present only because the snapshot covers the whole of <main>. So stop trying to keep part of it. Everything inside <chat-box> is now dropped from the snapshot in one line. The element itself stays in the tag list, so a chat-box that failed to render at all is still caught. That is blunt and the cost is stated in the comment rather than glossed: chat-box's own hydration and its header and form structure are no longer covered here. The counter still covers hydration on both sides, which is what this block is for. Keeping more than that needs the pane identified inside markup carrying neither an id nor a data hook, and the attempts to do that are the churn above. waitForChatLive and the snapshot() wrapper go with it, since both existed only to support the finer-grained exclusion. Net effect on the branch: +191 lines instead of +309, and the part that kept going wrong is gone. Verified over repeated runs that the snapshot race does not return: every remaining failure is #1228, the module that intermittently never executes, which no test change can fix.
vivek7405
force-pushed
the
fix/deflake-elision-e2e
branch
from
August 3, 2026 16:39
4dd9ab3 to
8c43fd1
Compare
vivek7405
marked this pull request as ready for review
August 3, 2026 17:53
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
1 participant
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 #1220
The
differential elision (#181)block reported its failures in a way that pointed away from the cause, and several of its guards could not observe the thing they named. This makes the block honest. It does not stop the block going red, see "What this does not do".The misdirection
The block failed with:
which reads as a counting bug. Nothing counted wrong. SSR emits the counter's full inner markup before any JavaScript runs:
So a click landing before the component's module executes hits a real button with no listener attached, and the counter stays on its seed. The seed is 3. Hence
off=3, three assertions away from the actual problem.Changes
instanceofagainst the registered constructor) rather than for markup the server already sent. Anything weaker is satisfied before hydration begins.clickCounterButtonrefuses to click an element that cannot respond, and says which of registered / upgraded failed. Checking the button exists is NOT that check, for the reason above; an earlier revision of this PR made exactly that mistake.on=4, off=3message.<chat-box>is dropped from the snapshot, in one line. The element stays in the tag list, so a chat-box that failed to render at all is still caught. This is deliberately blunt: its pane is a live websocket feed and the two pages hold sockets to two DIFFERENT servers (the OFF server is private to this block, the ON server is shared with the whole suite), so joins and leaves land on one side with no counterpart and no wait converges them. Earlier revisions tried to keep the status header and exclude only the pane; that needs the pane identified inside markup carrying neither an id nor a data hook, and it went wrong repeatedly under review. The cost is stated in the code: chat-box's own hydration and its header/form structure are not covered here. The counter still covers hydration on both sides, which is what this block is for.sleep()in the block is replaced by the condition it stood in for.What this does NOT do
It does not make CI green. By this PR's own measurement, module scripts are deferred and DOMContentLoaded waits for them, so on a healthy run the upgrade wait is already satisfied when
gotoresolves. The only run it changes is the one where the module never executes at all, which is #1228 (the elision-OFF boot intermittently 404s a module, leaving components inert) and is unfixed.So the block still reds at the same rate. What changes is that it now reds with
instead of an off-by-one. That diagnosis is what led to #1228 being findable at all.
An earlier revision also warmed the OFF server in
before(); it was removed (its premise was disproved by the DOMContentLoaded measurement) and the currentbefore()documents why.Test plan
counter 3 -> 3silently, and this version fails naming the cause.WEBJS_E2E=1 node --test test/e2e/e2e.test.mjs: the elision block is 3/3, repeatedly, including pinned to two cores.clickCounterButtoncallers pass; the new throw is a no-op where the counter is already hydrated.submittertests fail in my local worktree onmain's own copy of this file too (verified by runningorigin/main:test/e2e/e2e.test.mjsin the same tree), so they are environmental here, not from this diff. They pass on CI.Doc surfaces
AGENTS.md, skill references, docs site, MCP, editor plugins, scaffold templates, marketing copy: N/A, test-only diff with no public surface change.test:on purpose, so it generates no entry.