Conversation
…package's postinstall The tailwind and shadcn templates depend on bun-plugin-tailwind, whose bun peer dependency installs the npm bun package. That package is in the default trusted list, so every init of those templates ran its postinstall, which on musl shells out to npm install for a glibc build it cannot execute and on Windows x64 compiles a PowerShell snippet to probe for AVX2. The file ran it five times per run. Point the nested bun install at a global bunfig with ignoreScripts, initialize each template at most once per run and share the result between the "works" case and the typecheck/build case, use the shadcn init itself as the cache prime, and run the cases concurrently on Windows as well. The react cases now compare every scaffolded file against the template source, assert the exact file set and that every declared dependency was installed, and check that the build produced dist/index.html. The error case asserts the exact message and exit code.
|
Warning Review limit reached
Next review available in: 28 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 (1)
Comment |
|
Status: ready for review (head 7f8f5d3). Reproduced how: per-lane timelines extracted from the raw test-bun job logs of build 95391 (table in the PR body), plus a cold-cache run of the unmodified file on a windows aarch64 machine (43.9s). With this branch the same machine runs it in 16.6s cold and 11-13s warm, 7 out of 7 runs green; on this PR's first CI build the file went from 20-29s to 8-13s on the alpine and windows lanes, passing everywhere (numbers in the body). Found while investigating, handled separately: #38474 ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a substantial test-file restructure (memoized template inits shared across concurrent tests, describe.concurrent re-enabled on Windows, new exact-listing snapshots), so a human look would still be worthwhile.
What was reviewed:
- The memoized
initTemplate+ shared-directory pattern:init.filesis captured synchronously before any consumer runsbun run build, andexpectReactTemplateonly reads template files / README / package.json, so the concurrentdist/write doesn't race with the snapshot or the "works" assertions. XDG_CONFIG_HOMEglobal-bunfig mechanism matches the pattern intest/cli/install/minimum-release-age.test.ts;bunEnvdoesn't already set it.- Template source paths (
src/runtime/cli/init/react-{app,tailwind,shadcn}) exist; the source-dirbun.lock/node_modulesfilter matches what's actually checked in. readdirSync(temp)on theDisposableStringfromtempDir— itextends String, and the same call shape already exists elsewhere in the file.
Extended reasoning...
Overview
This PR restructures test/cli/init/init.test.ts to cut CI time on the slow lanes (alpine, windows) from 20-29s down to ~12-17s. The mechanism: (1) point the nested bun install at a global bunfig with ignoreScripts = true so the npm bun package's postinstall never runs, (2) memoize each template's bun init so the "works" and "typechecks/builds" cases share one install instead of running two, (3) replace the throwaway cache-prime with the shadcn init itself, and (4) enable describe.concurrent on Windows. Along the way the assertions are tightened: exact file-listing snapshots replace existsSync spot-checks, template files are byte-compared against src/runtime/cli/init/<template>/, the ENOTDIR error case asserts the exact message and exit code, and the build case now asserts dist/index.html exists so it can't silently become a no-op.
Security risks
None. This touches only a test file; no production code, no auth/crypto/permissions surface.
Level of scrutiny
Medium. It's test-only, so there is no user-facing risk, but it is not a mechanical change: it introduces a shared-directory-across-concurrent-tests pattern (two tests read from and one writes dist/ into the same temp dir), removes a platform exemption (Windows was serial before), and adds several inline snapshots that pin the exact scaffold layout for three templates. The reasoning in the PR body is thorough and I verified the load-bearing assumptions (file capture happens before any build writes; the XDG_CONFIG_HOME override pattern is already used in minimum-release-age.test.ts; DisposableString extends String so readdirSync(temp) is fine; the template source directories and their bun.lock filter match the repo). Still, the concurrent-shared-directory design and the Windows re-enablement are the kind of test-infrastructure decisions a maintainer typically signs off on — if the build script for one of the templates ever starts touching files outside dist/, or if Windows concurrency turns out to have been serial for a reason the author's 7 runs didn't surface, this file will flake.
Other factors
The PR description notes five open PRs touch this same file (#36607, #35164, #38419, #38474, #35445) with rebase implications, which is another reason a human should be in the loop on merge order. No prior reviews from me or other humans; only bot noise in the timeline. The bug-hunting pass found nothing.
|
On the two points worth spelling out from the review above:
|
…l fails The react cases were switched from inherited stdio to pipes, so when the nested bun install fails the error lines no longer reach the log, and the first assertion to notice was a file-listing diff or an ENOENT. Check the installed packages first, with the captured stdout/stderr in the assertion, and drop the blanket stderr error/warn match, which also fired on registry state such as peer dependency warnings during a publish window.
|
Pushed 7f8f5d3 after a self-review pass over the diff. Two things it turned up, both in the new react cases:
No change to the mechanism or the timings; PR body updated accordingly. This push also re-runs CI, which matters because the previous build's only non-flaky problem was both darwin 14 aarch64 test shards expiring without ever getting an agent (the neighbouring build 95945 had the same thing happen). |
There was a problem hiding this comment.
LGTM — test-only refactor that strengthens assertions and cuts CI time on the slow lanes.
What was reviewed:
- Shared-directory concurrency:
init.filesis captured synchronously inside the memoized promise before any consumer sees it, and the only post-init writer (bun run build) targetsdist/, which none of theworksassertions read. initTemplatememoization underdescribe.concurrent: the check-and-set is synchronous, so two concurrent callers can't both miss the cache.XDG_CONFIG_HOMEoverride:bunEnvdoesn't set it, andtest/cli/install/minimum-release-age.test.tsalready uses the same mechanism to configure the nested install.- Verified
src/runtime/cli/init/{react-app,react-tailwind,react-shadcn}exist andtempDirreturns aDisposableString, soreaddirSync(temp)is fine.
Extended reasoning...
Overview
This PR refactors test/cli/init/init.test.ts to reduce its wall-clock time on alpine and Windows CI lanes (25-29s → 8-13s) without weakening coverage. The changes are: (1) point XDG_CONFIG_HOME at a global bunfig with ignoreScripts = true so the nested bun install skips the npm bun package's postinstall; (2) memoize each template's init in initTemplate() so the works and typechecks/builds cases share one directory per template; (3) replace the throwaway cache-prime installs with the shadcn init itself; (4) drop the Windows-only serial-describe exemption; (5) tighten assertions across the board — exact file-listing snapshots, byte-for-byte comparison of every scaffolded file against src/runtime/cli/init/<template>/, exact error message and exit code for the ENOTDIR case, and an expectInstalled helper that surfaces the nested install's stdout/stderr when a declared dependency is missing (working around #38474). One file changed, no production code touched.
Security risks
None. This is a test file that spawns bun init and bun install in temp directories. The new XDG_CONFIG_HOME points at a test-created temp dir and is scoped to the spawned processes' env.
Level of scrutiny
Medium — it is test-only, but it shares mutable temp directories across concurrently running test cases and removes a platform-specific concurrency exemption, both of which are classic flake sources. I traced both:
- Shared directories: the
InitResult.fileslisting is captured inside the memoized async IIFE before the promise resolves, so no consumer can observe it afterbun run buildwritesdist/.expectReactTemplatereads only the template source files, README, package.json andnode_modules/<pkg>/package.json— none of which the build step touches.tsc --noEmitwrites nothing to the project. - Memoization race:
templateInits.get/.setruns synchronously before anyawait, so underdescribe.concurrenttwo callers with the same flag cannot both miss the cache. - Windows concurrency: the exemption came from a bulk conversion (#22823) with no recorded reason; the author ran 7 cold/warm runs on Windows aarch64 all green, and this PR's CI build (95949 per the body) passed all 15 cases on every lane including both Windows targets.
Other factors
- No CODEOWNERS entry for this path.
- Assertions are strictly stronger than before (exact listings and content comparison replace
existsSync/toHaveProperty;not.toBe(0)becomes exact stderr +toBe(1)), matching REVIEW.md's "assert the strongest invariant" guidance. - Cleanup: shared directories and the global config dir are removed in
afterAllviaPromise.allSettled+fs.promises.rm({force:true}), so a failed init doesn't block cleanup of the others. - I confirmed
src/runtime/cli/init/containsreact-app,react-tailwind,react-shadcnand thatreact-app/has abun.lock(excluded by the filter inexpectReactTemplate), and that the harness'stempDirreturns aDisposableStringsoreaddirSync(temp)works.
|
Nothing further has changed since 7f8f5d3. On that head's build the file is green on every lane that has run so far (the only annotations are retry-passed flakes in unrelated files); the darwin 14 aarch64 test shards are the ones still waiting for an agent, as in the previous build. |
This branch has not been deployed
1 participant
Problem
test/cli/init/init.test.tsis a serial-phase file that took 25s on alpine x64, 20s on alpine aarch64, 23s on windows x64 and 29s on windows aarch64 in build 95391, against 3.5-6s on the debian, ubuntu and darwin lanes.bunpackage.--react=tailwindand--react=shadcndepend onbun-plugin-tailwind, whosebunpeer dependency installs that package, andbunis insrc/install/default-trusted-dependencies.txt, so every init of those templates ran itsinstall.js. The file ran it five times per run (cache prime, twoworkscases, two typecheck/build cases).bun@1.3.14postinstall tries the glibc build first (@oven/bun-linux-x64), fails to execute it, runsnpm installof that same package against registry.npmjs.org, fails again, and only then tries the musl build. In the alpine x64 log the tailwind and shadcn installs took 5.9s and 7.4s after the cache was primed, against 0.2s for the blank template on the same lane and 0.17s / 0.65s for the same two installs on debian; the prime itself took 13.4s against 2.0s on debian.packages/bun-releasesince npm(bun-release): prefer musl package first on musl hosts #36283, but the test installs whatever is published, and nothing in the file asserts on lifecycle scripts.)describeinstead ofdescribe.concurrenton Windows, and the shadcn template (60 packages, 5555 files, 4090 of them in lucide-react) was installed three times per run at about 6s per warm install on that lane.Fix
initEnvpointsXDG_CONFIG_HOMEat a global bunfig with[install] ignoreScripts = true, so thebun installthatbun initspawns skips thebunpackage's postinstall.peer = falsewas tried first and rejected: it still downloads the@oven/*tarballs and it also drops the project's owntypescriptpeer dependency, which the TypeScript 6 cases exist to check.initTemplate, memoized per flag). Theworkscase and the "installs TypeScript 6, typechecks, and builds" case for a template share the result; the file listing is captured right after init, before the build case writesdist/. Shared directories are removed inafterAll.beforeAllprime is now the shadcn init itself rather than two throwaway installs. shadcn's closure is a superset of every other template's, including the blank one: after a shadcn-only init into a fresh cache,-y,--react,--react=tailwindand--react=shadcneach reportResolved, downloaded and extracted [0].describe.concurrenton every platform. The Windows exemption came from the bulktest.concurrentconversion in Start using test.concurrent in our tests #22823 with no recorded reason; 7 runs on a windows aarch64 machine (one cold cache, six warm) were all green.workscases now snapshot the exact set of filesbun initcreated, compare every file fromsrc/runtime/cli/init/<template>/against what was written, check the per-template README title and bun version, and check that every dependency, devDependency and peerDependency declared in the written package.json is present in node_modules. This replaces thetoHavePropertyandexistsSyncchecks.expectInstalled): the react inits now use pipes instead of inherited stdio, andbun initexits 0 when the nested install fails (init: exit 1 when the nested bun install fails #38474), so this is what puts the install's error lines in the failure output instead of a bare listing diff or ENOENT. Verified with a dead registry (BUN_CONFIG_REGISTRY=http://127.0.0.1:1/): all seven template cases fail at that assertion with theerror: ConnectionRefused ...lines shown. There is deliberately no blanket "stderr has no error/warn lines" check, sincewarn:lines there depend on registry state (peer ranges during a publish window, optional dependency fetch failures).buildscript, so the build branch cannot silently stop running, and that the build produceddist/index.html.Failed to change directory to mydir: ENOTDIR, identical on Windows), empty stdout and exit code 1 instead ofnot.toBe(0).existsSynccalls.beforeAlland about 7s for the concurrent phase (fourtscruns and three builds).bun test: 43.9s before, 16.6s after (11.2-12.7s with a warm cache). Of the 16.6s, about 11s is the one cold shadcn install inbeforeAll.bun bd test test/cli/init/init.test.ts: 35.2s before, 34.4s and 34.9s after. Flat as expected: glibc never took the postinstall detour, and under a debug+ASAN binary the file is dominated by the three templatebun run buildsteps (5-24s each), which are unchanged. Release binary on the same box: 4.9s before, 4.4-5.0s after.bunpackage still pulls one to four@oven/bun-*platform tarballs into the cache once per run (four on x64 linux, since bun install does not filter optional dependencies by libc yet, install: filter optionalDependencies by libc (glibc/musl) #31123). The only way to avoid that ispeer = false, rejected above.test/cli/install/bunx.test.ts(test(bunx): install fixture packages from a local registry instead of real ones #38467): that file does not installbun-plugin-tailwind. One observation from the same logs that does apply to any install-heavy file: even fully cached, the 5-package blank installs took 200-250ms on alpine and windows aarch64 against 14-30ms on debian.--react workscase, whose body changed). bun init: put typescript in devDependencies, not peerDependencies #35445's edits to the three reactworkscases are superseded, since those now compare package.json against the template source.Background
bun initwrites the template files and then spawnsbun installin the new directory;bun installreads$XDG_CONFIG_HOME/.bunfig.toml(falling back to$HOME/.bunfig.toml) on every platform before the project's bunfig, which is how a test can configure the nested install without adding files to the scaffold it is asserting on.test/cli/install/minimum-release-age.test.tsuses the same mechanism.trustedDependenciesor in bun's built-in default trusted list;bunis on that list.ignoreScriptsdisables them for the whole install.BUN_INSTALL_CACHE_DIR(scripts/runner.node.mjs), so every run of this file starts from a cold cache; bun dedupes downloads within one install but not across concurrently running installs, which is why one install has to run alone before the rest start.Per-lane timeline of the file in build 95391 (from the raw job logs)
[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file