Repository navigation
Conversation
|
Status: reworked after review. Measured where the file's time goes (3.0 of 3.9s is the harness's fixed error overlay poll, which #37866 removes for all of |
|
Warning Review limit reached
Next review available in: 6 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only change that tightens assertions and cuts ~2s of fixed polling.
What was reviewed:
errors: nullmatches the established opt-out inhot.test.ts/bundle.test.ts/css.test.ts;dev.patchstill awaits the client ack, and the newpageState()assertions cover what the overlay poll guarded.clickButton()replaces the fixedsetTimeout(10)with aMutationObserver— the observer is registered before.click(), so no race.- Fixture change resets
nextIslandIdalongside the existingislands = {}reset so repeated renders produce stableI:0ids for the snapshot;normalizeBunSnapshothandles theBun v<bun-version>substitution andDev/Clientare exported from the harness. - Snapshot embeds svelte scoped-class hashes (
svelte-1f5jrvn) — stable while the fixture CSS is unchanged, normal snapshot maintenance otherwise.
Extended reasoning...
Overview
Test-only PR touching one bake dev-server test and its svelte fixture. ecosystem.test.ts is rewritten to (a) pass errors: null on the two dev.patch() calls to skip the harness's fixed 1s overlay poll, (b) replace three toContain checks and a throw new Error("failed") with a full-document inline snapshot via servedPage(), (c) replace a setTimeout(10) click wait with a MutationObserver, (d) assert exact page state (toEqual on server/island/button text) after connect, after the reload, and after the hot update, and (e) drop timeoutMultiplier: 2. The fixture gains nextIslandId = 0 in render() (alongside the existing islands = {} reset) and an id="server_text" on the server paragraph. No production code is touched.
Security risks
None. Changes are confined to test/bake/ — a test file and two test-fixture files.
Level of scrutiny
Low-to-medium. Test-only, but the repo guidelines are explicit that weakening or de-flaking tests must preserve the property being asserted. Here the assertions move in the stronger direction: full-document snapshots replace substring checks, exact toEqual replaces toInclude, page state is now asserted after the reload (previously unchecked), and interactivity is verified after each edit rather than once. The dropped overlay poll is compensated by directly asserting rendered content after each write, which would fail if a build error left stale content.
Other factors
errors: nullis the same opt-out already used in three sibling bake test files;Dev/Clientare exported frombake-harness.tsandc.js<T>accepts a type parameter.normalizeBunSnapshotintest/harness.tsreplacesBun v${version}withBun v<bun-version>, matching the snapshot; the asset hash and route generation are normalized by the test's own regexes.- The
MutationObserveris attached beforebutton.click()and disconnects on first change, so the promise cannot miss the mutation. - The fixture's
nextIslandIdreset is a straightforward per-request state reset in a test-only server; it makes the second SSR snapshot deterministic (I:0instead of a carried-over counter). - The PR description documents 30/30 sequential release runs, 131/132 under heavy CPU oversubscription, and debug+ASAN passes; the one stress failure was traced to a pre-existing late watcher event that the old test is equally exposed to.
- Snapshot brittleness from svelte's scoped-class hashes is acceptable — they derive from the fixture's own
<style>blocks, which this PR does not change.
… in ecosystem.test.ts
The svelte islands test checked the served HTML with three substring
matches, treated an SSR failure as `throw new Error("failed")`, waited a
fixed 10ms after clicking, checked the hot update with toInclude("magical")
and did not look at the page after the reload at all.
It now compares the whole served document (with bake's generated names
normalized) against one expected-document template before and after the
edits, asserts the response status instead of searching for the fallback
page, reads the server text, island text and button text of the live page
into one object after connecting, after the reload and after the hot
update, and clicks the button after each of those, waiting for its text to
change with a MutationObserver, so the page is shown to be hydrated and the
re-mounted island to be interactive rather than just rendered.
The fixture numbers islands per render instead of across the server's
lifetime, so the document served after the edits differs from the first one
only by the edits, and the server component's paragraph gets an id the test
can select.
timeoutMultiplier: 2 predates the harness scaling its timeouts for debug,
ASAN and CI builds and is dropped.
The file's time (about 3.9s locally, of which 3.0s is the harness's fixed
error overlay poll on connect and after each write) is left to #37866,
which removes that poll in the harness.
b5eeeec to
bb6c8ad
Compare
robobun
changed the title
1 participant
Problem
test/bake/dev/ecosystem.test.ts(one test, the svelte islands fixture) was flagged by the slow-test sweep at 11-13s on the release Linux lanes. Measured locally it is 3.9s, of which 3.0s is the harness's fixed poll for an error overlay (Client.expectErrorOverlay,bake-harness.ts:1046: 5 x 200ms, no early exit when nothing is shown), paid once indev.client()and once after each of the two writes; the dev server, both bundles, the client and the test's own work add up to about 0.3s. On the 13s CI run (build 99854, where this was the third file the job ran) the log timestamps put 4.8s in starting the dev server (it imports the svelte compiler), 3.8s in starting node + happy-dom, 3.0s in the polls and about 0.25s in the rest, so the CI number is mostly the cold box.test/bake, which takes this file to about 0.9s without touching it. A per-fileerrors: nullopt-out would cover two of this file's three polls and become dead as soon as that lands (bake test harness: check the error overlay once instead of polling for a second per connect and write #37866 deletes the one existing opt-out of that kind; the othererrors: nulluses intest/bakeare for writes that expect errors or have no client), so this PR does not add one; the speed of this file is bake test harness: check the error overlay once instead of polling for a second per connect and write #37866's.throw new Error("failed"), the served HTML was checked with threetoContains, the click waited a fixed 10ms, the hot update was checked withtoInclude("magical"), and the page was not looked at after the reload at all.Fix
servedDocument()asserts the response is 200 (a throw during SSR is served as the 500 fallback page; the error itself is in the dev server output the harness echoes) and returns the whole document with bake's generated names normalized and one tag per line; it is compared withtoBeagainstexpectedDocument({ server, island })before and after the edits, so the two fetches are shown to differ exactly by the two edited strings, and the stylesheet link, island wrapper and$islandsscript are covered as well. A template rather than a snapshot because snapshot matchers throw outsidebun test, which would break the harness's interactive mode (bun test/bake/dev/ecosystem.test.ts svelte); checked that mode still passes.svelte-cssnamespace, and the bundler currently merges the server component's CSS into the component (the module identity bug bundler: key PathToSourceIndexMap on (namespace, path) for plugin module identity #36549 fixes; with distinct paths both stylesheets are served). The line carries a comment saying so, so that bundler: key PathToSourceIndexMap on (namespace, path) for plugin module identity #36549 knows to add the second link here when it lands.pageState()reads the server component text, the island text and the button text in one round trip and is compared withtoEqualafter connecting, after the reload and after the hot update. The post-reload page is now asserted at all (new server text, and its counter started over), and the hot update is checked for its exact text and for leaving the server text alone.clickButton()waits for the button's text to change with aMutationObserverregistered before the click instead of sleeping 10ms, and is repeated after the reload and after the hot update, so the reloaded page and the re-mounted island are shown to be interactive, not just rendered.render()resets the island counter per render (it already reset the island map), so the document after the edits hasI:0like the first one instead of a counter carried across renders; the server component's paragraph gets anidforpageState().timeoutMultiplier: 2is dropped: it predatesWAIT_MULTIPLIER, which now scales the base 30s by 3 for debug, 3 for ASAN and 2 for CI; the release CI budget becomes 60s and the ASAN one 180s for a test that took at most 13s on a cold lane.bun bd test test/bake/dev/ecosystem.test.tspasses (debug + ASAN); 10/10 runs with the release binary at 3.85-3.94s, i.e. unchanged from before (3.87-3.99s), as expected since the time is the poll; the interactive-mode run above.Earlier version of this PR
The first push also passed
errors: nullto the two writes, which took the file from 3.9s to 1.86s locally (median of 30 runs) and passed 131 of 132 runs under 5x CPU oversubscription; the one failure was a late watcher event rebuildingindex.sveltein the next batch, which the previous version of the test is equally exposed to. Review pointed out that this duplicates #37866 for two of the file's ~three poll sites while that PR removes the same opt-out elsewhere, so it was dropped; the assertions are unaffected, sincedev.patch()resolves on the client's ack either way.Background
test/bakedrives it throughbake-harness.ts:dev.patch()edits a file and resolves once the rebuild is done and every connected client has acked it;dev.client()loads the page in a node process running happy-dom (client-fixture.mjs);c.jsevaluates code inside that page. Running a test file directly instead of throughbun testruns one case interactively, stopping before each step.pages/index.svelteis a server component rendered withsvelte/server;_Counter.svelteis marked"use client", so the server renders it inside a<bake-island>and the client bundle hydrates it from theself.$islandsscript. Editing the island is a hot update applied in place (svelte's HMR re-mounts the component, which is why its count goes back to 5); editing the server component makes the runtime reload the page, which the test allows withc.expectReload()./_bun/client/route-<index><generation>.js, where the generation is randomized every time the route's client bundle is invalidated, and the stylesheet URL is a hash, which is why both are normalized before the comparison.