test: stop quarantining whole files for one broken case - #33952
Conversation
An entry in test/expectations.txt removes the FILE from the run — the kind
(FAIL/SKIP/CRASH/FLAKY) is parsed but never read, so there is no
"expect failure" mode. Quarantining one case deletes the file's whole
coverage, and nobody finds out, because a skipped file reports nothing.
Verified with the debug (ASAN) build, per file:
native-plugin.test.ts 18 tests dark for 1 real failure. That failure
is ASAN-only: the plugin null-derefs on purpose
and ASAN traps the SEGV before the crash handler
can print the name. Skip that case under ASAN
(it already skips on Windows) -> 18 pass, 1 skip.
run-crash-handler.test.ts 9 pass, 0 fail. Entry was stale.
reportError.test.ts 2 pass under ASAN. Entry was stale.
child_process.test.ts 43 pass under ASAN. Entry was stale.
bunshell.test.ts 388 pass under ASAN. Entry was stale.
bundler/cli.test.ts no debug-log failures under ASAN. Entry was stale.
create-jsx.test.ts 7 pass locally; the entry names a CI-only build
failure that no longer reproduces. Removed - if
CI disagrees it will say so, which is the point.
Also drop 5 entries naming files that do not exist, and rewrite the header
to say what the runner actually does, so the next person quarantines the
broken TEST rather than the file.
spawn-maxbuf.test.ts stays quarantined: it is not flaky, it fails
deterministically, and the cause is a real behavior question (below).
|
Updated 9:54 PM PT - Jul 10th, 2026
@Jarred-Sumner, your commit 4e7cef3 is building: |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 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)
WalkthroughChangesThe PR updates platform-specific test skipping, documents a native-plugin validation exclusion, and revises expectation guidance while removing obsolete broken-test, ASAN, and HTTP/2 skip entries. Test stability
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I verified: the 6 removed entries (5× test/js/node/test/sequential/test-http2-* + test-inspector-enabled.js) point to files that do not exist on disk; the new header's claim that getRelevantTests never reads the expectation kind is accurate (scripts/runner.node.mjs:2086-2091 destructures expectations but uses only modifiers); and the "match by SUBSTRING" claim matches isMatch at line 2052. Worth waiting on build #71732 before merge — the 7 un-quarantined files were verified on one local ASAN build, not the full platform matrix.
Extended reasoning...
This PR un-quarantines 7 test files and removes 6 stale entries, based on local debug/ASAN verification. The header rewrite and runner-behavior claims check out against the source. The only risk is a re-quarantined file failing on a platform the author did not run (Windows, macOS, musl); that is answerable only by the pending CI build, not code inspection. The inline nit about the skip comment misdescribing raise(SIGSEGV) as a null-deref is minor and non-blocking. No production code is touched.
Build 71732 ran these files for the first time. What the blanket entries
were hiding, and what each needs:
native-plugin.test.ts
ASAN Its napi addon's Init() calls napi_create_function and then
napi_set_named_property without checking the status in between,
like most real addons. validateExceptionChecks simulates a throw
in every scope, so the second call trips at the napi boundary.
Added to no-validate-exceptions.txt, which keeps the file RUNNING
on the ASAN lane instead of skipped.
musl Every test passes (19/19). The job fails only because the
"plugin crashes" test segfaults on purpose and the agent writes a
core, which the runner counts as a failure. Skip that one test.
Windows beforeAll builds the addon with node-gyp and the agents have no
ClangCL toolset (MSB8020), so nothing in the file can run. This
one is a real whole-file skip, with the reason written down.
create-jsx.test.ts
Windows 12 of 13 tests are already todo there; the last one asserts on an
"--only-missing install" line `bun create` never prints on
Windows. Marked todoIf(isWindows).
Net: native-plugin runs on darwin, glibc linux, and ASAN (18 pass, 1 skip);
create-jsx runs everywhere.
There was a problem hiding this comment.
LGTM — test-infra only, restores coverage that stale quarantine entries were hiding. The follow-up commit tightened the native-plugin skip comment to "segfaults on purpose" (accurate) and folded in what CI surfaced (musl core-dump, Windows ClangCL, validateExceptionChecks). Verified the six dropped expectations.txt entries (5× sequential/test-http2-*, test-inspector-enabled.js) reference files that do not exist on disk, and isASAN/isMusl are real harness.ts exports.
Extended reasoning...
Overview
Test-infrastructure cleanup touching four files, zero production code:
test/expectations.txt— rewrites the header to document that entries remove the whole file regardless of kind, drops ~12 stale/nonexistent entries, and narrows thenative-plugin.test.tsquarantine from all-platforms to[ WINDOWS ]only.test/bundler/native-plugin.test.ts— one test'sskipIfwidened fromwin32towin32 || isASAN || isMuslwith a per-platform comment.test/cli/create/create-jsx.test.ts— one test gatedtodoIf(isWindows).test/no-validate-exceptions.txt— addsnative-plugin.test.ts(napi fixture's uncheckednapi_*status between calls trips the simulated-throw checker).
Net effect: ~460 previously-dark test cases across 7 files re-enter the CI matrix.
Security risks
None. No runtime, parser, network, crypto, or install code touched. Changes are limited to CI test-selection metadata and skipIf/todoIf guards in test files.
Level of scrutiny
Low. The failure mode of getting this wrong is bounded and self-correcting: an incorrectly un-quarantined file turns CI red on the affected platform, which is immediately visible. There is no silent-corruption risk. The PR already went through one CI round — commit aaace0db exists specifically to fold in what build #71761's predecessor surfaced (musl agent core-dump accounting, Windows node-gyp toolchain, exception-check validation), which is evidence the loop was closed.
Other factors
- I spot-checked the "files that do not exist" claim: none of
test/js/node/test/sequential/test-http2-{timeout-large-write,timeout-large-write-file,max-session-memory,ping-flood,settings-flood}.jsortest/js/node/test/parallel/test-inspector-enabled.jsexist on disk, so removing those entries is a no-op for the runner and pure hygiene. isASANandisMuslare both exported fromtest/harness.ts, so the new imports resolve.- My earlier nit about the skip comment's "dereferences null" wording was addressed — it now reads "segfaults on purpose", which matches
raise(SIGSEGV)innative_plugin.cc. The alternative.env(bunEnv)fix I floated was explicitly speculative for CI; keeping the skip is the conservative choice and still nets +18 tests in that file. - The rewritten
expectations.txtheader is factually consistent withgetRelevantTestsinscripts/runner.node.mjs(substring match, kind ignored) and gives future contributors the right guidance.
With the exception-check exemption in place the file now passes on the ASAN lane (18 pass, 0 fail) and then aborts at exit: LeakSanitizer reports a direct 8-byte leak from Bun's own dlopen path. BunProcess.cpp allocates a NapiModuleMeta and hands it to a NapiExternal with no finalizer, right under a comment saying "TODO: think about the finalizer here / currently we do not dealloc napi modules". So the meta is never freed. That is a real leak, but fixing it means changing napi module lifetime, which does not belong in a test-quarantine cleanup. Exempt the file from the leak check so its 18 tests run on ASAN, and leave the leak reported.
There was a problem hiding this comment.
LGTM — test-infra only, net effect is restoring coverage that stale quarantine entries had silently disabled. The comment wording nit from my earlier pass was addressed (now "segfaults on purpose").
What was reviewed:
- Verified the 5 removed
sequential/test-http2-*entries andtest-inspector-enabled.jsreference files that no longer exist on disk. - Confirmed
isMuslis exported fromharness.ts; the widenedskipIfandtodoIf(isWindows)still net-gain coverage vs. the prior whole-file quarantines. - Checked the new
no-validate-exceptions.txt/no-validate-leaksan.txtentries only relax sanitizer modes for a file that was previously not running at all.
Extended reasoning...
Overview
This PR touches only test-infrastructure files: test/expectations.txt (removes ~12 stale/nonexistent entries, rewrites the header to document that entries remove whole files, adds a Windows-scoped entry for native-plugin), two test files (narrows whole-file quarantines to per-test skipIf/todoIf), and adds native-plugin.test.ts to the two no-validate lists with commented rationale. No runtime, native, or bundler code is modified.
Security risks
None. The diff is confined to CI test-selection metadata and test skip conditions. No auth, crypto, network, or user-facing code paths are touched.
Level of scrutiny
Low-to-moderate. The change is mechanically simple (deleting text-file lines, adding || isASAN || isMusl to a skip condition, wrapping one test in todoIf(isWindows)), and the blast radius is bounded to CI green/red. The real verification is the CI matrix itself, and the commit history shows three rounds of CI-driven fixups already applied (aaace0db, 54c2a15d) after the initial un-quarantine, so the author has already iterated against the full matrix. I spot-checked that the removed sequential/test-http2-* and test-inspector-enabled.js entries reference files that do not exist on disk, matching the PR's "5 nonexistent entries" claim.
Other factors
My earlier review left one nit — the skip comment inaccurately said "dereferences null" when native_plugin.cc actually calls raise(SIGSEGV). The thread is resolved and the comment now reads "segfaults on purpose", which is accurate. The author did not take the speculative .env(bunEnv)-instead-of-skip suggestion, but that was explicitly flagged as unproven for CI and non-blocking; skipIf(isASAN) on one test is strictly better than the prior whole-file quarantine, so there is no regression. The new no-validate-* entries only relax sanitizer modes for a file that previously wasn't running at all, so they cannot mask a regression. Every change here moves in the direction of more coverage, and any residual platform-specific failure will surface as a red CI job rather than silent breakage.
#33952 rewrote the expectations.txt policy: quarantining drops the whole file from the run, so the entry must either name a file that cannot run at all or carry its coverage elsewhere. test-crypto-dh.js can run; it fails on one verifyError === 0 assert that cannot be skipped without editing a verbatim upstream mirror. Port its two-party exchange and setPublicKey/setPrivateKey round-trip into node-crypto.test.js so the quarantine does not silently drop that coverage, and reword the entry to spell that out.
2 participants
The mechanism
An entry in
test/expectations.txtremoves the whole file from the run.getRelevantTests(scripts/runner.node.mjs) filters on the platform modifier and never reads the kind —[ FAIL ],[ SKIP ],[ CRASH ],[ FLAKY ],[ LEAK ],[ TIMEOUT ]are all the same thing. There is no "run it and expect failure" mode.So quarantining one broken case silently deletes that file's entire coverage, and nothing ever reports it. Several entries turned out to be stale — the thing they quarantined had been fixed, and no one could tell, because the file wasn't running.
Restored
Verified with the debug (ASAN) build.
test/bundler/native-plugin.test.tstest/js/node/child_process/child_process.test.tstest/js/bun/shell/bunshell.test.tstest/cli/run/run-crash-handler.test.tstest/bundler/cli.test.tstest/js/bun/util/reportError.test.tstest/cli/create/create-jsx.test.tsnative-plugin's one real failure is ASAN-only: the plugin dereferences null on purpose, and ASAN traps the SEGV and aborts before Bun's crash handler can print the plugin name. It is nowskipIf(win32 || isASAN)— the broken test is quarantined, not the file.Also
Not fixed here
test/js/bun/spawn/spawn-maxbuf.test.tsstays quarantined. It is not flaky — it fails deterministically (5/5 runs). Two causes:toBeLessThan(100)) that a debug/ASAN build cannot meet — it measured 108ms.Bun.spawnwithtimeoutkills the direct child, but readingproc.stdoutafterwards blocks for the child's full lifetime.bun exec sleep 5→ the shell spawnssleep, which inherits the stdout pipe's write end. Killing the shell leavessleepholding it, so the stream never EOFs.proc.exitedresolves; the stream does not. WhetherkillSignalshould reach the process group is a product decision, so I left it alone.