bake test harness: derive multi-HTML route keys from posix paths - #37892
bake test harness: derive multi-HTML route keys from posix paths#37892robobun wants to merge 2 commits into
Conversation
indexHtmlScript builds one static route per HTML file when a test has more than one. The route key was derived from the path.relative() result, which uses backslashes on Windows, so a nested file such as docs/index.html was registered as "/docs\\index" and every request for it hit the harness's 404 fallback. The same line also left a top-level index.html at "/index" instead of "/". Normalize the separators once and derive the key per path segment, giving the same routes as "bun ./index.html ./docs/index.html ./docs/guide.html".
|
Status: reproduced on a Windows x64 box against the unmodified harness (a multi-route fixture with The diff is ready from my side. Both CI runs so far (builds 93405 and 93452) failed before any linux x64 tests ran because |
|
Warning Review limit reached
Next review available in: 8 minutes 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 (2)
Comment |
There was a problem hiding this comment.
LGTM — test-harness-only fix with clear before/after semantics.
What was reviewed:
- Traced
htmlFileRouteagainstindex.html,about.html,docs/index.html,docs/guide.html→/,/about,/docs,/docs/guide; the old non-anchored.replace("/index", "")and Windows-separator issues are both gone. - Confirmed the only existing multi-HTML fixtures (three in
css.test.ts, flatfirst.html/second.html) produce identical keys under the new derivation, and thehtmlFiles: ["index.html"]/htmlFiles: []callers still take their original branches. - New test asserts each route's distinct body plus a 404 for
/docs/index, so it can't pass via the single-file catch-all.
Extended reasoning...
Overview
This PR touches only test infrastructure: test/bake/bake-harness.ts (the indexHtmlScript generator that writes bun.app.ts for bake dev/prod fixtures) and adds one devTest to test/bake/dev/html.test.ts. The harness change normalizes path.sep → / once up front (previously only the import line did this, so route keys on Windows contained backslashes) and replaces the ad-hoc .replace(/\.html$/, "").replace("/index", "").replace(/\/$/,"") chain with a segment-based htmlFileRoute that strips .html and drops a trailing index segment. A doc comment on htmlFiles now states the single-file vs multi-file routing behavior.
Security risks
None. This is test-harness code that generates a fixture bun.app.ts inside a temp directory during test runs; it does not ship in the runtime and processes no untrusted input.
Level of scrutiny
Low. No runtime code paths are touched. The blast radius is the bake test suite, and the PR body enumerates every existing consumer of indexHtmlScript and shows they are unchanged (flat filenames → same keys) or take a different branch. I spot-checked that claim against test/bake/dev/ — the three css.test.ts multi-HTML fixtures use flat first.html/second.html (identical output), bundle.test.ts passes htmlFiles: ["index.html"] (single-file /* branch), and the htmlFiles: [] cases skip the generator entirely.
Other factors
The new logic is easy to verify by hand: "index.html" → segments ["index"] → pop → "/", "docs/index.html" → ["docs"] → "/docs", "docs/guide.html" → "/docs/guide". It also fixes a latent bug the old code had (non-anchored "/index" replace would mangle docs/index-page.html → /docs-page). The added test exercises the multi-route branch with nested and top-level index.html, asserts distinct bodies per route, and checks /docs/index 404s to prove these are exact static routes rather than the single-file catch-all — so it fails for the right reason without the fix. The PR body reports it was verified on both linux and windows. No outstanding reviewer comments.
|
Updated 3:06 PM PT - Aug 12th, 2026
❌ @robobun, your commit d71215a has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37892That installs a local version of the PR into your bun-37892 --bun |
There was a problem hiding this comment.
LGTM — test-harness-only fix for Windows path separators and index.html route keys in multi-HTML bake fixtures.
What was reviewed:
htmlFileRoutederivation traced forindex.html,about.html,docs/index.html,docs/guide.html— matches the routesbun ./*.htmlwould produce.- Confirmed the only existing multi-HTML fixtures (three in
css.test.ts, flatfirst.html/second.html) still map to/first//second;indexHtmlScripthas no callers outside the harness. - Checked the
mainDircase (../web/index.html) still hits the single-file/*branch, so the..segment never reacheshtmlFileRoute.
Extended reasoning...
Overview
This PR fixes route-key derivation in the bake test harness's indexHtmlScript generator (test/bake/bake-harness.ts). Previously, when a fixture had multiple HTML files, the generated bun.app.ts derived static-route keys from path.relative(mainDir, file) without normalizing path.sep, so nested files on Windows produced keys like "/docs\\index". It also mapped a top-level index.html to /index and used a non-anchored .replace("/index", "") that would corrupt paths like docs/index-page.html. The fix normalizes separators once, then derives the key per-segment (strip .html, drop a trailing index segment). A new test in test/bake/dev/html.test.ts covers /, /about, /docs, /docs/guide, plus a 404 for /docs/index.
Security risks
None. This is test-harness code that generates a fixture file inside a temp directory; no production/runtime code is touched, and no untrusted input is involved.
Level of scrutiny
Low. The change is confined to ~15 lines of a test-support helper plus a doc-comment and a new test. indexHtmlScript is only called from within bake-harness.ts itself. I verified the three existing multi-HTML fixtures in css.test.ts use flat first.html/second.html, so their keys are unchanged; the nested html/index.html fixture at css.test.ts:645 is single-file and still takes the /* catch-all branch. The mainDir caller (which can produce ../web/index.html) is also single-file per the PR body and repo grep, so the .. segment never reaches htmlFileRoute.
Other factors
The PR description is thorough: it shows the before/after generated output, cites the reference behavior in src/js/internal/html.ts, enumerates every existing consumer and confirms they were re-run on both Linux and Windows, and demonstrates the new test fails without the harness change (/ returned 404 because index.html was keyed as /index). The bug hunting system found no issues. This is a clean, well-scoped harness improvement that unblocks #37868.
Problem
GET /docs,GET /docs/guide).index.htmlin such a fixture was served at/index, not/."/docs\\index"; the/indexstrip was a first-occurrence substring replace, so it missed a top-levelindex.html.Fix
.html, drop a trailingindexsegment.bun ./index.html ./docs/index.html ./docs/guide.htmlitself serves (/,/docs,/docs/guide), which is what this branch imitates.maindepends on the old keys: existing multi-HTML fixtures use flat files, whose routes are unchanged./,/about,/docs,/docs/guideand expects 404 at/docs/index. It fails without the harness change on every platform and passes on linux-x64 and windows-x64; the other tests using this code path were re-run on both and pass.Background
devTest/prodTestwrites abun.app.tsper fixture that imports each HTML file and exports a server config; this PR only changes that generated file.staticmaps a route string to an imported HTML module. String keys match exactly, so a key with a backslash in it is never hit./*catch-all; several get one exact route each, imitatingbun ./a.html ./b.html, which serves a file at its path minus.htmland anindex.htmlat its directory (docs/bundler/html-static.mdx).path.relative, which uses the platform separator, so on Windows nested paths carry backslashes until converted.Original description
What
test/bake/bake-harness.tsgenerates abun.app.tsfor everydevTest/prodTestfixture that contains HTML files. With one HTML file it registers a/*catch-all; with several,indexHtmlScriptregisters one static route per file and derives each key from the result ofpath.relative(mainDir, file). Only theimportline normalizedpath.sep; the key derivation did not, so on Windows any nested HTML file on a multi-route server was registered under a key containing a backslash and was unreachable. The same line also mapped a top-levelindex.htmlto/index.Generated
bun.app.tsfor a fixture withabout.html,docs/index.htmlanddocs/guide.htmlon the Windows box, before this change (GET /docsandGET /docs/guideboth returned the harness's 404 fallback):After:
"/about","/docs","/docs/guide".Fix
Normalize the separators once for both the import specifiers and the route keys, and derive the key per path segment (strip
.html, drop a trailingindexsegment). That gives the routesbun ./index.html ./docs/index.html ./docs/guide.htmlitself would serve (/,/docs,/docs/guide, seedocs/bundler/html-static.mdxandsrc/js/internal/html.ts), which is what the multi-route branch was imitating. The previous.replace("/index", "")was also a non-anchored first-occurrence replace, so it would have turned e.g.docs/index-page.htmlinto/docs-page; the segment version does not have that problem.Nothing on
maindepends on the old keys: the only multi-HTML fixtures are three tests intest/bake/dev/css.test.tsusing flatfirst.html/second.html, which still map to/first//second. The one fixture that has a top-levelindex.htmlnext to another HTML file (bundle.test.ts, "importing html file with text loader") passeshtmlFiles: ["index.html"]and therefore still takes the/*branch. ThehtmlFilesoption doc now states both behaviors so the next multi-route test does not have to readindexHtmlScriptto find out which routes it gets.This surfaced while restructuring the css tests in #37868, which currently keeps the
html/index.htmlcase on its own server because of this; with this change it can join a shared server.Test
test/bake/dev/html.test.ts, "multiple html files are served at the routes bun derives from their paths":index.html,about.html,docs/index.html,docs/guide.html, each with a distinct body, fetched at/,/about,/docs,/docs/guide, plus a 404 for/docs/indexto show these are exact routes rather than the single-file catch-all.Without the harness change it fails on every platform at
/(index.htmlwas registered as/index); a variant of the same fixture withoutindex.htmlfails only on Windows, at/docs(the output above). With the change it passes on linux-x64 (bun bd test, andUSE_SYSTEM_BUN=1) and on windows-x64 (USE_SYSTEM_BUN=1, canary 9a543cc).Also re-ran the other consumers of
indexHtmlScriptwith the change, on both linux (bun bd test) and windows: the three multi-route css tests, "css import before create project relative" (nested single file), bundle.test.ts "directory cache bust case #17576" (mainDir, so the import specifier is../web/index.html) and "importing html file with text loader" (explicithtmlFiles), and react-spa.test.ts (fixture directory branch). All pass.