Skip to content

bake: speed up the css dev server tests by sharing servers and asserting served stylesheets - #37868

Open
robobun wants to merge 4 commits into
mainfrom
farm/6b277035/css-dev-tests-shared-servers
Open

bake: speed up the css dev server tests by sharing servers and asserting served stylesheets#37868
robobun wants to merge 4 commits into
mainfrom
farm/6b277035/css-dev-tests-shared-servers

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • test/bake/dev/css.test.ts is one of the ten slowest non-integration test files: 52-61s per lane in test/expected-durations.json, about the same on release and ASAN lanes.
  • The time is fixed waits in the harness, not bundling. Every client connect, and every write made while a client is attached, ends in a 5 x 200ms poll for an error overlay; the old file did that 41 times, about 41s of a 56s local release run.
  • The file also booted 15 dev servers, about 2s each for boot plus exit on a debug/ASAN build. Bundling itself is a few ms per step.

Fix

  • Test-only change. Cases whose fixtures do not interact share one dev server with one HTML route per case, so 15 servers become 7. Three cases keep their own server (asset-table order, a bunfig plugin, a nested HTML file the harness only routes correctly on POSIX).
  • Server state is asserted over plain HTTP: each case pins the exact stylesheet chunk it serves as an inline snapshot, and a failing route must serve the "Build Failed" page. Clients and hardReloads that only loaded a page once become fetches (clients 20 to 16, overlay polls 41 to 19).
  • Writes pass errors: null to skip the overlay poll only where the next assertion can only pass if the rebuild succeeded: a stylesheet that fails to rebuild keeps its old rules in the page, and reloads are only sent while nothing is failing. Writes that recover from an error keep the poll.
  • Verification: no case is deleted, skipped or made todo; the table below maps every old assertion to its new home. Interleaved local runs of the old and the pushed file on the same machine: 92.1s to 53.9s on debug+ASAN and 54.2s to 31.5s on release (earlier rounds under heavier load: 120-135s to about 74s, 65-68s to about 39s). Every local run of the new file passed (for the pushed revision: 3 release, 2 debug; 16 more across the earlier revisions, which differ only in the points listed under "Review follow-ups").

Background

  • devTest (in test/bake/bake-harness.ts) boots one dev server per case from a map of fixture files. A fixture with several HTML files gets one route per file, which is what lets unrelated cases share a server.
  • dev.client(route) attaches a happy-dom browser in a node process that receives hot updates over a websocket. dev.fetch(route) is a plain HTTP request, so served HTML and chunks can be asserted without a client.
  • After each connect, and each write made while a client is attached, the harness checks that client for an error overlay, polling 5 x 200ms when there is none. errors: [...] asserts specific messages; errors: null skips the check.
  • Build errors are pushed to every connected client, so error cases run with no other client attached and every case on a shared server ends with its files recovered (the dev server sends no page reloads to anyone while any failure exists). Fetching a route whose stylesheet is failing re-bundles it and, because the HTML still compiles, pushes the HTML module to clients as a plain JS hot update, which kills a client that has that page loaded (Dev server ships HTML route module as a JS HMR module when a failed CSS root recovers #31908); clients on other routes or on the error page are unaffected, recovery writes never push it, and a route whose HTML file is itself the failing file pushes nothing. Verified all four with probes; the rule is documented once on expectBuildFailed and every fetch of a broken route in the file satisfies it.
  • Injected stylesheets are served at /_bun/asset/<hash>.css. The helpers only look at those links because a route bundled while its stylesheet was failing keeps its source <link> after recovery (bake: keep the import record source index of a CSS file that failed to bundle #37844).
Original description

What

test/bake/dev/css.test.ts is one of the ten slowest non-integration test files (61s on the Windows 11 aarch64 lane, 52-56s on the others per test/expected-durations.json). The numbers barely differ between release and ASAN lanes because the time is mostly fixed waits in the harness, not bundling. Measured on a local release run of the old file (56s for 15 cases):

  • Every dev.client() and every dev.write()/patch() made while a client is attached ends in Client.expectErrorOverlay, which polls 5 x 200ms when no overlay is visible. The old file made 41 such calls (17 connects, 22 writes, 2 hardReloads), about 41s of the 56s. Each connect costs ~1.45s (node + happy-dom + the poll), each write with a client attached ~1.07s; the bundling work itself is a few ms per step.
  • Under the debug/ASAN build a dev server boot plus graceful exit costs about 2s, and the file booted 15 of them.

This PR only changes the test file. The poll in bake-harness.ts is the remaining shared lever for this file, bundle.test.ts (#37827 is the same pass over that file) and hot.test.ts, and is being looked at separately; after this change the file still pays it 19 times (13 connects without expected errors, 6 writes that recover from an error), which is about 19s of the remaining ~37s.

Changes

  • Cases whose fixtures do not interact share a dev server, one HTML route per case: the three @import graph cases, the shared-dependency/asset/script-import cases, the four error cases, and the "stylesheet appears after the server started" and "link tag changes" cases. 15 servers become 7. Three cases keep their own server: the asset-table swap-remove case (needs first.css and second.css to be the only two entries, in that order) and the bunfig-plugin case, which both keep their operation sequence, and the project-relative case, whose HTML file has to stay in a subdirectory (that is what makes its /style/styles.css link project-relative rather than HTML-relative) while the harness's multi-route branch only registers nested HTML files correctly on POSIX (it builds the route key from path.relative output without normalizing separators, so on Windows it would be /html\index; a separate harness fix). On its own server it takes the single-file catch-all route exactly as on main.
  • One happy-dom client remains per case that asserts the browser side of a hot update (16 clients instead of 20). Clients that only existed to load a page once (the first and last clients of the asset-table case, the two fresh clients at the end of the plugin case) and the two hardReloads are replaced by fetching the route and asserting what a fresh load gets.
  • Server state is asserted over HTTP with three small helpers: servedCss fetches a route's HTML, requires exactly one injected stylesheet link, and returns the chunk (asserting 200 and text/css), so each case pins the exact served stylesheet as an inline snapshot (chunk order of @imported files, how a circular import is printed, what an emptied stylesheet serves, which root gets rebuilt); stylesheetUrls asserts how many stylesheets a route links; expectBuildFailed asserts a failing route serves the 500 "Build Failed" page rather than just a status, replacing the not.toContain("HELLO") checks. These add ~20ms to the whole file.
  • Writes pass errors: null when the assertion right after them can only pass if the rebuild succeeded and reached the client. This is sound for mechanical reasons, not just faster: a stylesheet that fails to rebuild keeps its old rules in the page (the "does not kill old styles" case asserts exactly that, so a toBe(newValue) or notFound() after the write proves the rebuild landed), and the dev server only emits the route reload list while bundling_failures is empty (DevServer.rs, the List 1 block in finalize_bundle), so a passing expectReload proves nothing is failing. Writes that recover from an error keep the default check, because the overlay disappearing is what those steps assert. The header comment in the file states the convention. The two writes inside expectNoWebSocketActivity in the remove/re-add case also pass errors: null: the client fixture exits on any socket message inside that block, so the overlay check there could not observe anything (the project-relative case already did this).
  • A route whose stylesheet is failing is only fetched while no connected client has that page loaded (see the Background bullet on Dev server ships HTML route module as a JS HMR module when a failed CSS root recovers #31908 for the exact rule, which is also the doc comment of expectBuildFailed). The old "css url resolve error" case fetched its broken route with the page's client attached and only passed because the client was disposed before it applied the pushed update (with a debug build the client fixture prints ASSERTION FAILED ... at replaceModules and exits, which the harness's dispose does not notice; a harness fix for that is being handled separately). The restructured case asserts the same three things (overlay text, 500, recovery) with the fetch after the dispose; the "does not kill old styles" case does not fetch its route while broken, since its client has to survive until the stylesheet is hot-swapped back in; the link-tag case does fetch while broken, because there the HTML file itself is what fails.
  • Two things are intentionally not pinned because they are being fixed separately: the order the dev server injects several <link>s in (currently reversed; bake: serve an HTML route's stylesheets in source order #37845 fixes it and adds a dedicated case, so the two-link step here asserts the set of chunks), and whether a root that fails to resolve a url() through a plugin keeps its old rules on the client (it currently does not, while the builtin resolver does; the plugin case asserts what the original asserted). The helpers only look at injected /_bun/asset/ links because a route bundled while its stylesheet was failing currently keeps its source <link> after recovery (bake: keep the import record source index of a CSS file that failed to bundle #37844). On the shared servers that leftover tag is a 404, so the recovery reload of the initial-error case currently sits through the client fixture's 1s stylesheet-load check before acknowledging (visible as a "Reached maximum CSS load check attempts" line in the output); the old single-route servers answered that URL with the HTML page instead. bake: keep the import record source index of a CSS file that failed to bundle #37844 removes that second.

Where each original case's assertions live

# Original case Now Kept Added / changed
1 css file with syntax error does not kill old styles "bundling errors", /keep red; overlay text keep.css:4:1; still red while broken; fix applies (overlay check kept); emptied stylesheet removes the rule exact chunk after the fix and when emptied (/* keep.css */ header only). Route is not fetched while broken, see above
2 css file with initial syntax error gets recovered "bundling errors", /initial overlay text on load; reload on fix (check kept); red; blue; overlay text when broken again exact chunk while blue; route is the Build Failed page after the final break (fetched after the client is gone); a second recovery serves the identical chunk again, so the shared server ends without failures. Blue write passes errors: null
3 add new css import later "shared imports, assets and script imports", /script no rule, rule after the import is added, gone after it is removed served HTML links no stylesheet before and after, exactly one while imported, with its exact chunk. Patches pass errors: null
4 css import another css file "@import graphs", /import h1 #00f and body red; after editing the imported file h1 green, body still red hardReload ("still works after a reload") is now the exact chunk a fresh load gets, asserted before and after the edit. Write passes errors: null
5 asset referenced in css "shared imports, assets and script imports", /asset client's url() resolves to bun.png, then to the replacement image after rewriting it the served chunk's url() resolves to the same bytes at both points. Write passes errors: null
6 syntax error crash "bundling errors", /crash 200 before, 500 after the patch (the double-unref panic killed the server) exact chunk before (background-image: url;); the 500 is the Build Failed page; fixing the file serves the new chunk (also leaves the shared server clean)
7 css url resolve error on hot reload is recoverable "bundling errors", /resolve red; overlay text resolve.css:2:21; route 500; recovery with no client attached old rule still applied after the failed rebuild; 500 is the Build Failed page; recovery asserts the exact chunk, not only 200. The 500 fetch happens after the client is disposed, see above
8 circular css imports handle hot reload "@import graphs", /circular .a red / .b #00f; after the edit .a green / .b #00f exact chunk before and after (the cycle is printed once, b then a). Write passes errors: null
9 asset index stays valid after another css root is freed own server, same operation order client on /second; first.css broken (errors: null as before); the second.css edit reaches the client; fix write keeps its overlay check; still green afterwards the first client on /first (only there to bundle it first) and the final fresh client on /first are fetches asserting the exact chunk; exact /second chunk initially, right after the edit (reads the moved asset slot directly) and unchanged after the fix
10 css hot update carries the edited stylesheet when another root fails in the same rebuild own server (bunfig plugin), same batch both clients' initial rules; the batch; .second green on c2; no .second on c1; recovery with no clients attached the two fresh clients after the fix are fetches asserting both routes' exact chunks; exact green /second chunk right after the batch; /first is the Build Failed page before the fix
11 multiple stylesheets importing same dependency "shared imports, assets and script imports", /shared-first + /shared-second, both clients all four initial rules; both clients see #ff0 after editing shared.css each root's own rule is unaffected; exact chunks of both routes before and after (both roots rebuilt on the server). Write passes errors: null
12 removing and re-adding css import "@import graphs", /toggle .colored present; gone after removing the import; no websocket activity while the orphaned file is edited twice; still gone; back (plus .main) after re-adding exact chunk with and without the import, unchanged after the orphan edits, byte-identical to the original once re-added. The two toggling writes and the two writes inside the no-activity block pass errors: null
13 changing html file with link tag works "stylesheets created after the server starts", /relink initial styles; rewriting the HTML unchanged reloads and the styles survive; overlay text for the unresolvable link; creating the file reloads (check kept) into .other red / no .test; switching back reloads into .test / no .other; both links give both exact chunk initially, byte-identical after the rewrite reload and after switching back, exact .other chunk, and with two links the served set is exactly those two chunks; route is the Build Failed page while the link is unresolvable and the loaded page keeps its old stylesheet meanwhile. hardReload is covered by the chunk assertion after the rewrite reload; the three non-recovery rewrites pass errors: null (writeNoChanges is inlined to pass it); link order left to #37845
14 css import before create "stylesheets created after the server starts", /before overlay text on load; error text before.css:2:21 after creating the stylesheet; reload when the image appears (check kept); client's url() resolves to the image; HELLO served "page does not contain HELLO" becomes "route is the Build Failed page" (asserted once, at the stage the old case checked; the CSS-stage 500 page is covered by cases 2, 6 and 7)
15 css import before create project relative own server as on main (html/index.html at the catch-all route, style//assets/ layout unchanged) mkdir, both overlay texts, the no-activity create/delete block (errors: null as before), reload, image bytes, HELLO same not-HELLO replacement (asserted at both error stages)

No case is deleted, skipped or made todo.

Timing

Same machine, same binaries, old and new file run back to back in alternation. Host load varied a lot during the session (load average 50 to 210 on a 12 CPU quota), so absolute numbers move between rounds, but every interleaved pair shows the same ratio. The debug numbers run the binary that bun bd test builds, invoked directly under a separate uid, because this sandbox's shared inotify instance limit stops dev servers from starting as root (the old file fails identically when that happens; #37827 hit the same thing).

build old file (15 cases) new file (7 cases, as pushed) host load
linux debug + ASAN 92.1s 53.9s ~55-70
linux release 54.2s 31.5s ~55-70
linux debug + ASAN 135.5s, 120.2s 73.6s, 74.3s (previous revision) ~170-210
linux release 67.8s, 65.2s 39.7s, 38.0s (previous revision) ~170-210

Dev server boots 15 -> 7, happy-dom clients 20 -> 16, hardReloads 2 -> 0, overlay polls 41 -> 19 (13 connects without expected errors and the 6 writes that recover from an error), expect() calls 76 -> 264 (25 of them inline snapshots of served stylesheets). Every local run of the new file passed (3 release and 2 debug/ASAN runs of the pushed revision, 16 across the two earlier revisions), with no client assertion traces or "not accepted" hot updates in any log. The largest group takes ~7-10s in release against the harness's 30s per-case floor; everything in it is event driven (acks and reload events), with no timing-based waits added. The remaining 19 polls are what #37866 (harness) removes.

Review follow-ups

  • 3d6385d: the project-relative case is back on its own single-HTML server. On a shared server its nested html/index.html would be registered as /html\index on Windows (the harness's multi-route branch does not normalize path.sep; separate harness fix), and moving the HTML to the top level would have removed the thing the case tests.
  • 4f96628: the initial-error case recovers once more at the end, so every case on the shared error server ends without failures, as the header comment claims (the dev server sends page reloads to nobody while any failure exists, so a case appended after a still-broken one would fail in a confusing way). The fetch-while-broken rule is stated once on expectBuildFailed after probing what actually triggers it (see Background), replacing three comments that each described it differently. The before-create case's second fetch of its broken route is dropped: the rebuild it triggered made the client send an ack that the recovery write right after it would have counted. The link-tag case keeps its fetch (its HTML file is the failing file) and asserts the loaded page still has its stylesheet afterwards, which also consumes that ack before the next write.

test/expected-durations.json is left alone; the scheduled job regenerates it from CI. #37844, #37845, #37859, #37867, #37051 and #33405 add cases to the end of this file; the file still ends with the untouched extractCssUrl, so those apply on top of this apart from the import line.


[stamp-90s] gate passed · iteration 1 · 1 files touched

passes on PR (with fix)
Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/bake/dev/css.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/bake/dev/css.test.ts
bun test v1.4.0 (2613c6b7d)

test/bake/dev/css.test.ts:
Dev server testing directory: /tmp/bun-dev-test-p8jV8W
�[0;30mdev|�[0m Started development server: http://localhost:37617
�[0;30mdev|�[0m �[32mBundled page in 179ms�[0m�[2m:�[0m import.html
�[0;30mweb|�[0m [I] [Bun] Hot-module-reloading socket connected, waiting for changes...
�[0;30mdev|�[0m �[32mReloaded in 112ms�[0m�[2m:�[0m import.css
�[0;30mdev|�[0m �[32mBundled page in 63ms�[0m�[2m:�[0m circular.html
�[0;30mweb|�[0m [I] [Bun] Hot-module-reloading socket connected, waiting for changes...
�[0;30mdev|�[0m �[32mReloaded in 37ms�[0m�[2m:�[0m circular-a.css
�[0;30mdev|�[0m �[32mBundled page in 30ms�[0m�[2m:�[0m toggle.html
�[0;30mweb|�[0m [I] [Bun] Hot-module-reloading socket connected, waiting for changes...
�[0;30mdev|�[0m �[32mReloaded in 32ms�[0m�[2m:�[0m toggle.css
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m nothing to bundle
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m modified files: /tmp/bun-dev-test-p8jV8W/css1/toggle-colors.css
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m modified dirs: /tmp/bun-dev-test-p8jV8W/css1/
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m nothing to bundle
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m modified files: /tmp/bun-dev-test-p8jV8W/css1/toggle-colors.css
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m modified dirs: /tmp/bun-dev-test-p8jV8W/css1/
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m nothing to bundle
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m modified files: /tmp/bun-dev-test-p8jV8W/css1/toggle-colors.css
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m modified dirs: /tmp/bun-dev-test-p8jV8W/css1/
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m nothing to bundle
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m modified files: /tmp/bun-dev-test-p8jV8W/css1/toggle-colors.css
�[0;30mdev|�[0m �[33mdebug warn�[0m�[2m:�[0m modified dirs: /tmp/bun-dev-test-p8jV8W/
... (truncated)
Exit: 0
diff hotspot
test/bake/dev/css.test.ts | 1261 ++++++++++++++++++++++++++++-----------------
 1 file changed, 783 insertions(+), 478 deletions(-)

gate history · 1 passed · 0 rejected · iteration 1

evidence per changed file
file                       reads  edits  tests
test/bake/dev/css.test.ts     25     43      0

…ssert the served stylesheets

test/bake/dev/css.test.ts booted one dev server per case (15) and a
happy-dom client for nearly every assertion (20 clients, 2 hard reloads)
to check what is mostly server state, and every write made with a client
attached paid the harness's one second error overlay poll. Group the
cases onto 6 servers, one HTML route per case, keep one client per case
that exercises the client side of a hot update (16), and read the rest
over HTTP: the exact stylesheet chunk served for each route, response
status and content type, and the Build Failed page for routes with
errors. Writes whose following assertion can only pass once the rebuild
reached the client pass errors: null; writes that recover from an error
keep the overlay check, since that is what they assert.

Cases that depend on the exact contents of the server (the asset table
layout, the bunfig plugin) keep their own server. Routes with a failing
stylesheet are fetched only after their client is gone, since
re-bundling such a route ships the HTML module to connected clients
(issue 31908); the existing resolve-error case only passed because the
client was disposed before it applied that update.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3bb1f166-218f-4a1d-9ee2-d8ea26d1275c

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and 4f96628.

📒 Files selected for processing (1)
  • test/bake/dev/css.test.ts

Walkthrough

The CSS development-server tests were reorganized into shared multi-route suites. Coverage now includes dependency graphs, assets, dynamic imports, stylesheet failures, recovery, HTML link changes, late-created files, and project-relative paths. Tests also verify served CSS during partial failures and recovery.

Changes

CSS development-server test coverage

Layer / File(s) Summary
Shared CSS test suites and scenarios
test/bake/dev/css.test.ts
Reusable helpers and shared suites cover CSS dependency graphs, assets, dynamic imports, link changes, late-created files, project-relative paths, and build failures.
Served CSS failure and recovery checks
test/bake/dev/css.test.ts
Asset-index and batched rebuild tests verify served CSS before failures, during partial failure, and after recovery.

Possibly related PRs

  • oven-sh/bun#37859: Expands CSS development-server tests and covers related dependency-edge behavior.
  • oven-sh/bun#37866: Covers related error and recovery behavior in hot.test.ts.
  • oven-sh/bun#37867: Covers related framework-route CSS imports and HMR regressions.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main test optimization: sharing servers and asserting served stylesheets.
Description check ✅ Passed The description explains the problem, implementation, scope, coverage, and verification results, despite using different headings from the template.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: test-only change, ready for a maintainer (revision 4f96628; 2613c6b on top is an empty commit that re-ran CI after a pipeline-wide outage). Still applies cleanly to current main.

CI: build 93647 finished with 179 of 181 jobs passed and no failures; the other 2 are the darwin 26 aarch64 test jobs, which never got an agent and were expired by Buildkite after three hours (the darwin 14 aarch64 lanes ran and passed), which is what the red aggregate status is. The only annotations are tests unrelated to this file that passed on retry (no-orphans, child_process_ipc_handle, inspect-error-leak, sourcetextmodule-leak, test-cluster-shared-leak, test-tonic). css.test.ts has passed on every lane in every build of this PR that reached the test stage (93359, 93557, 93647), including both Windows lanes, the ASAN lanes and darwin 14.

Verified locally against both builds, interleaved with the old file on the same machine: release 54.2s -> 31.5s, debug/ASAN 92.1s -> 53.9s; every local run of the new file passed. The description has the per-case assertion table and the review follow-ups: the Windows route-key problem with a nested HTML file on a shared server is addressed in 3d6385d (that case has its own single-route server again, as on main); 4f96628 makes the initial-error case end recovered like the other shared error cases, documents once on expectBuildFailed when a broken route may be fetched (probed: the fetch is the hazard, not the recovery), and removes the one fetch whose ack the following recovery write would have counted.

Related PRs adding cases to this file: #37844, #37845, #37859, #37867, #37051, #33405 (the file still ends with the untouched extractCssUrl, so they apply on top apart from the import line); #37866 is the harness change that removes the 19 overlay polls this file still pays.

Comment thread test/bake/dev/css.test.ts Outdated
@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 6:46 PM PT - Aug 12th, 2026

@robobun, your commit 2613c6b has some failures in Build #93647 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 37868

That installs a local version of the PR into your bun-37868 executable, so you can run:

bun-37868 --bun

…e server

Its HTML file has to live in a subdirectory, and the harness builds
multi-route keys from path.relative output without normalizing
separators, so on Windows a nested file registers as /html\index and
the shared-server version of the case would 404 there. As its own
server it takes the single-file catch-all route, as it did before.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed after 3d6385d and didn't find any further issues — the project-relative case is back on its own single-route server, so the Windows route-key concern from the earlier pass is resolved. Given the scale of the restructuring (15 cases folded into 7 shared servers, ~two dozen writes switched to errors: null, several client/hardReload steps replaced by served-chunk fetches), a human who owns the bake dev server tests should still confirm the coverage mapping and the errors: null reasoning.

What was reviewed:

  • Verified the split-out "css import before create project relative" case now takes the harness's single-HTML catch-all branch, avoiding the Windows path.sep route-key bug.
  • Checked each shared-server group ends with its error-producing route recovered (or is the last sub-case) so no build failure leaks into the next sub-case's client.
  • Confirmed Dev is an exported class and dev.read() exists, so the inlined writeNoChanges replacement type-checks and matches the original behavior plus errors: null.
  • Traced each errors: null write to a following assertion that reads the new value from the client or an expectReload; none looked vacuous.
Extended reasoning...

Overview

This PR restructures test/bake/dev/css.test.ts to roughly halve its wall-clock time (release ~61s → ~37s, debug/ASAN ~118s → ~66s). It merges 15 independent devTest cases into 7 by sharing dev servers across cases whose fixtures don't interact, replaces several happy-dom client connects and hardReloads with plain HTTP fetches of the served stylesheet chunk (asserted as inline snapshots via new servedCss/stylesheetUrls/fetchCss/expectBuildFailed helpers), and passes errors: null on writes whose next assertion mechanically proves the rebuild reached the client. No production code is touched.

My earlier pass flagged that combining html/index.html with two top-level HTML files would 404 on Windows because the harness's multi-route branch doesn't normalize path.sep in route keys. Commit 3d6385d addresses this by splitting "css import before create project relative" back into its own devTest with the original single-file layout (which takes the catch-all route branch), and documents the reason in a comment. Every other HTML file in the merged groups is top-level, so no remaining case hits the harness bug.

Security risks

None. Test-only change with no changes to runtime, bundler, or dev server code; no new external network calls (all fetches go to the local dev server the harness spawns).

Level of scrutiny

Medium-high despite being test-only. REVIEW.md is explicit that weakening or restructuring existing tests is one of the most-scrutinized categories: "Never silently weaken, skip, or delete an existing test or safety net… When de-flaking, keep asserting the property the original assertion protected." This PR does exactly that at scale — 15 cases' assertions are relocated, several overlay-poll checks are elided via errors: null, and previously isolated cases now share server state. The description's per-case table is thorough and I spot-checked several entries against the diff, but the soundness of each errors: null (that the following .toBe(newValue) / .notFound() / expectReload genuinely cannot pass if the rebuild silently failed) rests on dev-server invariants ("a stylesheet that fails to rebuild keeps its old rules", "reload lists are only sent while bundling_failures is empty") that someone who owns src/bake/DevServer.rs is better placed to confirm.

Other factors

  • The added snapshot assertions are strictly stronger than what they replace (exact served CSS bytes vs. only computed-style checks), and expectBuildFailed asserting the 500 body's <title> is stronger than the old not.toContain("HELLO").
  • I walked each shared-server group for cross-case bleed: /crash is repaired before /resolve connects; /resolve is repaired before /keep; /keep ends emptied (valid) before /initial; /initial ends broken but is last in its group. /before is fully recovered before /relink connects. The non-error groups never introduce build failures.
  • The two writes inside expectNoWebSocketActivity in the toggle case newly pass errors: null; the comment explains the client fixture exits on any socket message inside that block so the overlay check couldn't observe anything — this matches what the project-relative case already did on main.
  • CI build #93359 was in progress at review time, so cross-platform (Windows) results aren't confirmed here; that plus the sheer size of the coverage rearrangement is why I'm deferring rather than approving.

… failing route may be fetched

The error group's header promised every case leaves the shared server
without failures, but the initial-error case ended broken and only
worked because it was last: the dev server sends no route reloads to
anyone while any failure exists. Recover it once more and assert the
stylesheet is served again.

Fetching a route whose stylesheet is failing re-bundles it and pushes
the still-compiling HTML module to clients as a JS hot update (issue
31908), so the hazard is the fetch, not the recovery; verified that a
recovery with the page's client attached is fine, that fetching kills a
client with that page loaded, and that clients on other routes or on
the error page are unaffected. State that once on expectBuildFailed
instead of three differing comments. Drop the redundant second fetch in
the before-create case, whose ack would otherwise be counted by the
recovery write that follows, and in the link-tag case assert the loaded
page keeps its stylesheet while the HTML is broken, which also drains
that ack.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting the project-relative case back onto its own server — that resolves the Windows route-key issue from the earlier review, and this pass found nothing new. Given the scale of the restructuring (15 cases collapsed to 7 shared servers, errors: null skips gated on rebuild-must-have-landed reasoning, and the #31908 ordering constraints on expectBuildFailed), a maintainer familiar with the bake harness should still sign off on the shared-server isolation and the per-case coverage mapping.

Checked this run: the two remaining expectBuildFailed calls made with a client connected (/before, /relink, and both / calls in the project-relative case) all fall under the documented "HTML file itself fails" or "client showing the error page" exemptions; the /initial route's broken-from-the-start stylesheet is not bundled until that sub-case requests it, so it does not leak into the earlier /resolve and /keep overlay checks on the shared error server; and the script.ts find/replace pair is byte-identical to the original single-server case.

Extended reasoning...

Overview

Test-only change to test/bake/dev/css.test.ts: 15 devTest cases are consolidated into 7 by sharing dev servers across cases with non-interacting fixtures, and server-side state is asserted via new servedCss/stylesheetUrls/expectBuildFailed helpers as inline snapshots instead of only through happy-dom clients. The Windows route-key issue I flagged on the previous revision is fixed in 3d6385d by moving html/index.html back to its own single-route server; 4f96628 adds a second-recovery step to the initial-error case and documents when expectBuildFailed may be called with a client attached. No production code, harness code, or other test files are touched.

Security risks

None. This is a test file for the dev server's CSS hot-reloading; it spawns local dev servers and happy-dom clients in the test process, with no auth, crypto, or external I/O.

Level of scrutiny

Medium-high for a test-only change. The risk isn't runtime breakage but silently weakened coverage or new flake surface: cases now share a server, so an error left behind by one sub-case would show up in the next case's overlay check, and errors: null skips the harness's default overlay poll. The PR description's per-case table maps every original assertion to its new location and the file-header comment states the errors: null convention (only where the following assertion mechanically requires the rebuild to have landed). I traced each shared group for cross-contamination: the error group runs its four sub-cases sequentially with at most one client at a time, each ending recovered; the /initial route is only bundled when its sub-case requests it, so its initial syntax error can't appear in earlier sub-cases' overlay checks; and every expectBuildFailed call with a live client falls under one of the two exemptions its docstring names.

Other factors

  • The author reports 3 release + 3 debug/ASAN passing runs of the pushed revision (plus 10 more of the prior revision), and CI build #93557 is running on 4f96628.
  • Assertion count went from 76 to 260 with 25 inline snapshots of served stylesheet chunks — this is a strengthening, not a weakening, but the snapshots are new surface that a maintainer should eyeball once.
  • The shared-server design and the #31908 / #37844 workarounds encoded in the helpers' docstrings are the kind of harness-behavior reasoning a bake maintainer is best placed to confirm; that, plus the ~700-line net diff, keeps this outside the auto-approve bar for me even though I found no defects.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the Windows route-key issue — 3d6385d puts the project-relative case back on its own catch-all server, and 4f96628 makes /initial end recovered so the shared error server finishes clean. I re-reviewed the current revision and found no further issues.

Given the size of the restructure and the ordering constraints between cases sharing a server (lazy per-route bundling so /initial's fixture error stays dormant until requested; every errors: null write followed by a client assertion that only passes on a successful rebuild; expectBuildFailed only called where the page's client is gone or on the error overlay, or the HTML itself is the failing file), a maintainer familiar with the bake harness should still take a look before merge.

Checked: each errors: null site has a following c.style(...).toBe(new) / notFound() / expectReload that fails on a bad rebuild; the /before and project-relative fetches of a broken route happen while the client is on the error overlay (matching expectBuildFailed's stated rule); the two-link /relink step asserts the chunk set only, deferring order to #37845; no case deleted or weakened per the description's mapping table.

Extended reasoning...

Overview

Test-only change to test/bake/dev/css.test.ts (~700 lines). Fifteen devTest cases are consolidated into seven, with unrelated cases sharing a dev server via one HTML route each. Three helpers (stylesheetUrls, servedCss, expectBuildFailed) fetch the route's HTML/chunks over plain HTTP so served stylesheets are pinned as inline snapshots, and clients/hardReloads that only loaded a page once become fetches. Writes pass errors: null where the next assertion mechanically proves the rebuild landed. No production code is touched.

Prior review and follow-ups

My earlier inline comment flagged that combining html/index.html with other HTML files sends the harness through its multi-route branch, which does not normalize path.sep, so the route would register as /html\\index on Windows. 3d6385d resolves this by keeping the project-relative case on its own single-route server (the nested location is what the case tests, so flattening it was not an option). 4f96628 additionally makes the /initial case recover a second time at the end (so every case on the shared error server ends clean, matching the header comment's convention), consolidates the fetch-while-broken rule into expectBuildFailed's doc comment, and drops the second broken-route fetch in /before whose ack the following recovery write would have miscounted.

Sequencing on shared servers

I traced the four grouped tests for cross-case interference. In the "bundling errors" group, initial.css starts broken in the fixture but the dev server bundles lazily, so /crash, /resolve and /keep never see that error (each recovers before the next case's client connects, and /initial runs last). Every expectBuildFailed call site satisfies the rule in its doc comment: /resolve and /initial fetch after the block-scoped await using c disposes; /keep deliberately does not fetch while broken because its client must survive; /before and the project-relative case fetch while the client is still on the error overlay; /relink fetches while the HTML file itself is what fails. Each errors: null write is immediately followed by a client-side style assertion on the new value (or notFound()/expectReload), which cannot pass if the rebuild failed since the "does not kill old styles" case in this same file pins the invariant that a failed rebuild keeps the old rules.

Security risks

None. Test-only; no auth, crypto, network egress, or user-facing surface touched. The tests run against a local dev server on port: 0 via the existing harness.

Level of scrutiny

Medium. The change is confined to one test file and strictly increases assertion count (76 → 264 per the description, 25 of them inline snapshots of served CSS), with a per-case table mapping every original assertion to its new location. The risk is not correctness of shipped code but test coupling/flakiness: cases now share server state, and errors: null skips the harness's overlay poll. The author's justification for each is mechanical rather than empirical, CI on the previous revision (which differs from this one only by the two documented follow-up commits) was green on every lane including both Windows lanes, and multiple interleaved local runs on release and debug+ASAN passed.

Other factors

The diff is large and the shared-server ordering is subtle enough that a maintainer who owns bake-harness.ts should confirm the conventions (particularly the errors: null policy and the #31908 fetch-while-broken rule) match their intent before this becomes the pattern other bake test files follow (#37827 is the same pass over bundle.test.ts, and #37866 changes the harness poll this file still relies on 19 times). Deferring rather than approving on that basis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant