-
Notifications
You must be signed in to change notification settings - Fork 5k
test(napi): check the experimental-finalizer wrapper's output before its exit code #37214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
f5ccd47
940630e
9efd638
741c556
7f62e76
7027604
880da00
51f1c7e
2dcf453
e5946fb
9db94de
c0e09cd
d59a943
a2ee2f3
a51f860
107543a
b8743ea
56d5010
04afe87
0dff607
f103e21
2aaae6a
f07e513
0656ed1
011d990
5860983
85de86a
bb4430b
c6ceb9a
9a48440
c17e393
2f31865
04c4180
cb60a8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1329,12 +1329,38 @@ | |
| bunProc.exited, | ||
| ]); | ||
|
|
||
| // The wrapper script should exit with 0 if the test passed | ||
| expect(bunExitCode).toBe(0); | ||
| // Diagnostics: if the child left heap snapshots behind (it only does so | ||
| // when GC #1 failed to finalize), upload them as build artifacts. | ||
| const diagDir = join(__dirname, "napi-app"); | ||
| const snapshots = readdirSync(diagDir).filter(f => f.startsWith("napi-diag-") && f.endsWith(".heapsnapshot")); | ||
| if (snapshots.length) { | ||
| console.error( | ||
| "napi diagnostics: heap snapshots:", | ||
| snapshots.map(f => `${f} (${statSync(join(diagDir, f)).size} bytes)`), | ||
| ); | ||
| if (process.env.BUILDKITE) { | ||
| const up = spawnSync({ | ||
| cmd: ["buildkite-agent", "artifact", "upload", "napi-diag-*.heapsnapshot"], | ||
| cwd: diagDir, | ||
| env: process.env, | ||
| stdout: "inherit", | ||
| stderr: "inherit", | ||
| }); | ||
| console.error("napi diagnostics: artifact upload exit", up.exitCode); | ||
| } | ||
|
Check failure on line 1350 in test/napi/napi.test.ts
|
||
|
dylan-conway marked this conversation as resolved.
|
||
| } | ||
| // Checked first so a failure prints everything the wrapper and child wrote. | ||
| // Diagnostics: also fail (and so print everything, including the GC log) | ||
| // when the crash only happened on the second, event-loop-turn GC. | ||
| expect(bunStdout + "\n---- stderr ----\n" + bunStderr).not.toContain( | ||
| "GC #1 (synchronous, same stack) returned without crashing", | ||
| ); | ||
| expect(bunStdout + "\n---- stderr ----\n" + bunStderr).toContain("TEST PASSED: Process crashed as expected"); | ||
| expect(bunStdout + bunStderr).toContain("Loading experimental module"); | ||
| expect(bunStdout + bunStderr).toContain("Created"); | ||
| expect(bunStderr).toContain("FATAL ERROR"); | ||
| expect(bunStdout + bunStderr).toContain("TEST PASSED: Process crashed as expected"); | ||
| // The wrapper script should exit with 0 if the test passed | ||
| expect(bunExitCode).toBe(0); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Comment on lines
+1404
to
+1405
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The wrapper went from a single async spawn (with a 5s SIGKILL fallback) to 7 variants × 3 runs = 21 sequential Extended reasoning...What the bug isThe variant-matrix rewrite of test_experimental_with_timeout.js increased the wrapper's workload roughly 21× without adjusting the outer per-test timeout at napi.test.ts:1408, which remains The old wrapper spawned one child asynchronously, forwarded its output, and sent
Additionally, V1 run 0 writes Where it can biteThe test is gated only by The per-child Why existing code doesn't prevent it
Step-by-step proof
ImpactUnrelated timeout noise on non-darwin lanes while iterating on this diagnostics branch. That's the exact opposite of what the branch wants: clean CI signal so a genuine How to fixEither is fine (the second is closer to REVIEW.md's "shrink the workload" preference):
|
||
|
|
||
| // The marker must NOT have actually been printed. Only check stdout: the | ||
| // fixture prints the marker via console.log (stdout), while stderr contains | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.