Repository navigation
test(aot): run the compiled runs of errors-below-callbacks-that-may-get-a-display-name-in-a-test-run.test.ts concurrently and state every line - #47843
Conversation
…rrent test and state every line test/aot/errors-below-callbacks-that-may-get-a-display-name-in-a-test-run.test.ts ran its six `bun test --aot` runs one after another in one test. Each run is now a test of its own, and the tests are concurrent, so `bun test --max-concurrency` says how many of them build at once. Every run must print the 28 stated lines and the summary of two passing test files, and exit with 0. A run that asks to be one program must say that it is one. A debug build of Bun lists a private helper of flatMap in the stack of plain Bun, so the file failed there. It now runs the plain run and the two runs as one program on a debug build. It skips the four runs in layers there and says why.
|
Status: ready for review. This is a test-only change to How this was checked:
Both builds are red for the same 41 other test files, and none is this file. 40 of them are red in build 137162, which is main's own build of the base commit. The other is The description has every run and how it was taken. |
|
No actionable comments were generated in the recent review. WalkthroughThe test compares complete stack output and run results between plain Bun and AOT runs. It covers one-program, one-layer, and two-layer configurations, with and without runtime validation. ChangesAOT stack trace tests
Possibly related issues
Suggested reviewers: Merge Risk:
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline note, I also checked two things and ruled them out: the 28 literal expected lines follow the fixture's naming sequence in a.test.js (each shownAs/asWritten step and the "others" re-run with fresh each/inThePackage objects but the still-named probe), and the builtAsOneProgram: false expectation for layered forms cannot be tripped by the non-Linux "one program: this Bun loads one program a realm" line, since scripts/runner.node.ts runs this file only on the Debian and ASAN lanes.
Extended reasoning...
The PR rewrites one test file under test/aot, splitting a serial seven-run test into one plain-Bun test plus six concurrent --aot form tests, each in its own temp dir, with the full 28-line stdout, the bun test summary counts, the build form and the exit code asserted as one object. It touches no production code and no security-sensitive surface. The expected output is a hand-written literal that I could not execute here, and the only inline finding is a pre-existing per-test timeout nit, so a human familiar with the AOT test lanes should still glance at the concurrency and memory profile on the ASAN runners.
…acks to the suite Each test holds one run now, so it needs no limit of its own. The suite gives a test 120 s, and 270 s on a build with a sanitizer or assertions. In CI the whole file took 89 s on the lane with a sanitizer, 53 s on x64 and 34 s on aarch64, with all its runs at once. With a limit of 480 s on each of six tests, runs that hang two at a time held the file for three times as long as the one test of main did.
There was a problem hiding this comment.
LGTM — the follow-up commit drops the per-test 480 s ceiling, and the remaining change is a self-contained, test-only restructuring of one file.
What was reviewed:
- The 28 hard-coded
SEENlines and the2 pass / 0 fail / 2 expect() calls / Ran 2 tests across 2 files.summary against the unchanged fixture infiles(a.test.js probes,Error.stackTraceLimitloop, b.test.js) — they match what the fixture prints. builtAsOneProgramagainst src/runtime/cli/run_compiled.rs:AOT test run | one programis printed forTestRunLayers:0in both spellings, andrun()in test/aot/harness/run.ts still enforcesexpectLayersfor the layered forms.- The ruled-out 120 s timeout concern on the ASAN lane: test/aot/harness/default-timeout.ts raises the default to 270 s when
isASAN || isDebug, so each concurrent layered run has headroom beyond the measured 62–66 s. test.skipIf(...).concurrent(...)chaining and{ ...bunEnv, KEY: undefined }follow existing patterns in test/ (e.g. test/bundler/bundler_splitting.test.ts, test/aot/supported.ts'salwaysSkippedproxy handles the chain when--aotis unsupported).
Extended reasoning...
The PR changes only test/aot/errors-below-callbacks-that-may-get-a-display-name-in-a-test-run.test.ts (+74/-22), splitting one serial 480 s test into seven concurrent tests with exact expected output, a stderr summary check, and a one-program check, while leaving the fixture project byte-identical. It touches no production code and no security-sensitive surface. The second commit (3a2283c) removed the per-test timeout that my earlier inline comment flagged, and the bug hunt ran dry with no findings. The file has no CODEOWNER, and the timeline shows no CHANGES_REQUESTED review or unresolved human objection, so a human look is not required.
Problem
test/aot/errors-below-callbacks-that-may-get-a-display-name-in-a-test-run.test.tstakes 349 s on debian 13 x64-asan (build #137158).bun test --aotruns go one by one in one test. Four build in layers, and each compiles Bun's own library again.Fix
bun test --max-concurrencysays how many build at once.bun bd testfails on main and runs 3 tests in 23 to 30 s here.Background
bun test --aotcompiles a test run as one program or in layers: the package with Bun's own code, the application above it, each test file above that.bun test --aotfollows #44833's mocks #47605 also makes the six forms tests and runs two before a merge. Its filter fits this table.Notes
What the file asks, and what stays as it was. The project (
files) is the same text, byte for byte. The seven commands are the same: plainbun test --isolate, andbun test --isolate --aotwithTestRunLayers:0,TestRunLayers:1,TestRunPartFiles:1andTestRunLayers:2,TestRunPartFiles:1, each without and withBUN_JSC_validateAOTRuntime=1.run()ofharness/run.tsstill fails a run that was not built in the layers it asked for. Each run has a folder of its own now.One variable is new for every run:
BUN_JSC_showPrivateScriptsInStackTraces=0. It is the default of every build but a debug build, andtest/js/bun/util/reportError.test.tsand others pass it for the same reason.The limit of a test. The one test of main has 480 s for its seven runs, and takes 306 to 354 s of them on x64-asan (eleven recent builds, 137158 to 137216). No test has a limit of its own now. A test holds one run and has the limit of the suite: 120 s, and 270 s on a build with a sanitizer or assertions. What the slowest test took:
The rows with one and two CPUs stand for a full batch of CI, where the file has about that much of the machine. With two CPUs and AddressSanitizer this PR's file takes 231 s. Main's file takes 306 to 354 s in its batch.
Assertions, before and after.
2 pass,0 fail,2 expect() calls,Ran 2 tests across 2 files.for every runrun()checkslayers 1andlayers 2. Nothing checksTestRunLayers:0TestRunLayers:0must sayAOT test run | one program, and no other run maytoEqualwith the stated lines, nottoMatchInlineSnapshot: bun:test refuses snapshot matchers in concurrent tests.Four faults, put into both versions of the file. Release build.
a.test.jsprints the line "one of the package too" before it namesinThePackage, in every runb.test.jsskips its test1 pass,1 skip,1 expect() callsTestRunLayers:O, a letter for the digit, and are built in layersbuiltAsOneProgram: falseeachanother nameWhere the time goes. One run of each form alone, release build, 12 CPUs.
The project is 5 KB. In layers the lowest program holds Bun's own code for whatever the programs above may ask: 249 units. With
BUN_JSC_logAOTPhaseTimes=1a run in two layers says that this layer takes 4.7 s of 5.3 s, 25.1 s of user time. The application layer takes 0.18 s and the two test files 0.14 and 0.25 s. So the file is four builds of Bun's library, and its processor time does not change: 100 s of user time before and after (release, 12 CPUs).All local runs. Linux x64, 12 vCPUs, 15.6 GB, builds of 3bf1a06.
tasksetfor fewer CPUs. Each row is one round, taken in turn.Release (
bun run build:release test <file>), 12 CPUs:Release, 4 CPUs, in the environment that
scripts/runner.node.tsgives a file:--max-concurrency=4--max-concurrency=2Release with AddressSanitizer (
bun run build:asan), 8 CPUs, in the environment of the lane with a sanitizer:--max-concurrency=8--max-concurrency=2--max-concurrency=8(4 on the lanes without a sanitizer) is what the runner passes when the file runs in a process of its own.--max-concurrency=2is what a parallel batch gets.Debug (
bun bd test <file>):Received: "SEEN made, as written | made < visit < step < each < inThePackage < flatIntoArrayWithCallback < forEachOf < work < <anonymous>". A debug build setsshowPrivateScriptsInStackTraces(src/jsc/bindings/ZigGlobalObject.cpp, underBUN_DEBUG), so plain Bun lists the private helper thatflatMapcalls. No compiled run starts.SKIPPED:line on stderr names the runs in layers and the reason.CI, before. Build #137158, the only lanes that run the file: 349.29 s on x64-asan, 150.46 s on x64, 97.04 s on aarch64. On x64-asan it ran inside a batch of 36 files,
bun test --parallel=7 --max-concurrency=2, which took 413 s. This file was the longest of the batch. On x64 and aarch64 the batch has 40 files, 3 at a time. Other builds: 306 to 354 s on x64-asan (eleven builds of pull requests), 130 to 167 s on x64 and 83 to 89 s on aarch64 (four builds of main).CI, this PR. The runner starts a file that a PR changes in a process of its own, in front of the other files that build executables: with
--max-concurrency=8beside two other files on x64-asan, with--max-concurrency=4beside one on x64 and aarch64. So all seven tests ran at once. Build #137237 ran the first commit, which had a limit of 480 s on each compiled run. Build #137247 ran the second commit.--max-concurrency=2is the nearest measure of that: the same time as with all at once where processors are free.test/aot/servers/node.test.tstook 131 and 133 s on x64-asan (89 s in build #137158).test/aot/tools/same-code-wherever-the-text-stands.test.tstook 53 and 51 s on x64 (29 s) and 34 and 35 s on aarch64 (21 s).test/js/web/request/request-clone-leak.test.tson x64-asan, a lane that builds of main do not run. It is also red in builds #137193 and #137212 of other pull requests.On top of #47605. That PR leaves this file out of the list that CI runs in front of a merge, makes each form a test behind a
beforeAll, and runs two of the six forms unlessAOT_WHOLE=1. I put this PR's file on itstest/aot/harnessandtest/aot/supported.ts(22621ef), with its filter on the table, and ran it with main's build of Bun: 3 pass without the variable, 7 pass withAOT_WHOLE=1, 1 skip withAOT_WHOLE=0.Designs that were not taken.
bun test --aotwrites the program to an unnamed file and starts itself again on it (start_second_processinsrc/runtime/cli/run_compiled.rs). Nothing is left for a second run.new Function("f", "return f"). The package hands each callback through it, so that the compiler cannot follow the function that gets the name. That is what the file is about.Promise.all. The runner gives a file of a batch two tests at once, so that no more builds run than there are processors. Tests of their own obey it.[auto-merge] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file