Skip to content

test: run the --bail JUnit regression tests concurrently and assert the full report - #41423

Merged
Jarred-Sumner merged 3 commits into
mainfrom
robobun/c08fe17c/speed-up-26851-test
Sep 6, 2026
Merged

Jarred-Sumner merged 3 commits into
mainfrom
robobun/c08fe17c/speed-up-26851-test

Conversation

@robobun

@robobun robobun commented Sep 5, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • test/regression/issue/26851.test.ts takes up to 12s on the debian 13 x64-asan lane. It is in the slowest 5 percent of test files. Both tests spawn a bun test --bail child, and they ran one after the other.
  • On that lane each child exits 134, not 1. bun test --bail exits with a bare exit(1), so the BUN_DESTRUCT_VM_ON_EXIT teardown is skipped and LeakSanitizer aborts the child after several seconds of report symbolization (bun test exits after failures are not LSan-clean (bail and failing runs abort under detect_leaks=1) #32183, fix open in bun test: exit through the VM teardown path when --bail stops a run #39010). Locally each child takes 0.3s. Under the lane environment it takes 3.8s. The old expect(exitCode).not.toBe(0) hid the abort.
  • The old assertions only checked that the JUnit file exists and contains four substrings. They did not check the counters, the failure element, the bail message, or the exit code.

Fix

  • Add the file to test/no-validate-leaksan.txt, next to test/regression/issue/12250.test.ts, which is on the list for the same bail exit. The entry is to be removed when bun test: exit through the VM teardown path when --bail stops a run #39010 lands. Without LeakSanitizer the children exit 1 in well under a second.
  • Run both tests with test.concurrent. Each test owns its tempDir and its outfile, so the two children overlap. A shared runBailWithJUnit helper does the spawn and reads the report.
  • Assert the full JUnit document with toMatchInlineSnapshot: the testsuites root with tests, assertions, failures and skipped, one testsuite per file that ran, the testcase names and lines, and the failure element with its type and message. Only the time and hostname attributes are normalized. Assert the child output before the exit code: stdout is the version banner, stderr has the (pass)/(fail) lines, the Ran N tests across M files. summary and Bailed out after 1 failure. The exit code is exactly 1.
  • Add a second test after the failing one, and a third test file c_never.test.ts. Neither appears in stderr or in the report. This proves that --bail stopped the run and that the report holds only what ran. Discovery order of the root directory is sorted by name, so a_pass runs before b_fail.
  • Verified: bun bd test test/regression/issue/26851.test.ts. Plain, 3 runs: before 2.66s to 3.14s wall, after 2.27s to 2.44s wall. With the ASAN lane environment (BUN_DESTRUCT_VM_ON_EXIT=1, detect_leaks=1): before 9.53s with both children exiting 134, after the list entry that environment is not applied. Also passes with USE_SYSTEM_BUN=1.

Background

  • --bail makes bun test stop after the first failing test. bun test should produce reporter outfile when --bail is specified #26851 was about the JUnit outfile not being written in that case.
  • test/no-validate-leaksan.txt lists test files for which scripts/runner.node.mjs does not set BUN_DESTRUCT_VM_ON_EXIT and detect_leaks=1 on the ASAN lane.
  • make more tests faster #33704 also adds test.concurrent to this file as part of a broad speed pass. This PR is scoped to the one file and also strengthens the assertions.
Notes

The first CI run (build 110365) failed on the x64-asan lane with exit code 134 on both tests and 8.7s per test. That is the LeakSanitizer abort described above. The fix for the bail exit itself is in #39010 and out of scope here.

The stderr checks use toContain rather than a snapshot of the whole stream. Open PRs #39010, #39276, #38974 and #38265 change parts of the bail and reporter output, and a whole-stream snapshot would conflict with them for no gain. The XML snapshot is the thing under test.

The failure body in the XML contains at fail.test.ts:2:40. The fixture sources are built from fixed strings, not indented template literals, so the column is stable. test/js/junit-reporter/junit.test.js snapshots the same shape on every platform.


[auto-merge] gate passed · iteration 1 · 2 files touched

passes on PR (with fix)
Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/regression/issue/26851.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "test/regression/issue/26851.test.ts"
bun test v1.4.3 (e0a2b82fd)

test/regression/issue/26851.test.ts:
(pass) --bail writes JUnit reporter outfile [356.62ms]
(pass) --bail writes JUnit reporter outfile with multiple files [349.11ms]

 2 pass
 0 fail
 2 snapshots, 15 expect() calls
Ran 2 tests across 1 file. [2.42s]
Exit: 0
diff hotspot
test/no-validate-leaksan.txt        |   3 +
 test/regression/issue/26851.test.ts | 140 +++++++++++++++++++++---------------
 2 files changed, 87 insertions(+), 56 deletions(-)

gate history · 3 passed · 0 rejected · iteration 1

evidence per changed file
file                                 reads  edits  tests
test/no-validate-leaksan.txt             1      1     25
test/regression/issue/26851.test.ts      3      5     19

root cause · written by the author bot

The regression test for issue #26851 was slow because each of its five tests spawned a separate bun test child one after another, and under ASAN every child paid several seconds of startup, while the assertions only checked that the JUnit outfile existed. The fix runs the independent cases concurrently through a shared runner that captures stdout, stderr, the exit code and the report once per fixture set, then asserts the full normalized JUnit document shape, the bail message, the summary line and the exact exit code. A follow-up strips the CI environment variables that make the reporter …

…he full report

Both tests in test/regression/issue/26851.test.ts spawn their own bun test
child in their own tempDir. Run them with test.concurrent so the two
children overlap.

Assert the whole JUnit document with an inline snapshot (time and hostname
normalized), the bail line and the summary line on stderr, and exit code 1.
Add a test after the failure and a third test file to show that --bail
stops the run and that the report only holds what ran.
@github-actions github-actions Bot added the claude label Sep 5, 2026
@coderabbitai

coderabbitai Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 111fa4ee-fac9-470f-9ac5-9ff0a04faf13

📥 Commits

Reviewing files that changed from the base of the PR and between c01965f and 797408f.

📒 Files selected for processing (1)
  • test/regression/issue/26851.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


Walkthrough

Changes

JUnit bail regression coverage

Layer / File(s) Summary
Shared bail test runner and JUnit normalization
test/regression/issue/26851.test.ts
Adds reusable test generators, report normalization, and a process runner that captures output, exit status, and the JUnit report.
Single-file and multi-file bail assertions
test/regression/issue/26851.test.ts
Verifies bail execution, exit code 1, bail messaging, skipped tests, discovery order, and normalized JUnit contents.

Merge Risk: ⚪ Minimal · up to 79740

This updates regression coverage for JUnit bail output, exit behavior, and skipped tests without changing runtime implementation behavior. No merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly describes the main changes: concurrent --bail JUnit regression tests and complete report assertions.
Description check ✅ Passed The description explains the problem, implementation, verification steps, scope, and expected behavior. It provides the information required by the template, although it uses different headings.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Sep 5, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: the first CI run showed both children exit 134 on the x64-asan lane. That is the LeakSanitizer abort at the bare bail exit (#32183, fix in #39010), which the old not.toBe(0) assertion hid and which is where the 12s went. The file is now on test/no-validate-leaksan.txt next to 12250.test.ts until #39010 lands. deaf4a7 also strips the CI commit and job env vars from the child, so the snapshot holds when GITHUB_SHA or CI_JOB_URL is set. The PR body has the details and timings.

… the VM teardown

Under the ASAN lane environment, bun test --bail exits with a bare
exit(1). The VM teardown is skipped and LeakSanitizer aborts the child
with exit code 134 after several seconds (#32183). The old assertions
hid this with not.toBe(0). Put the file on the same list as
test/regression/issue/12250.test.ts until #39010 lands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/regression/issue/26851.test.ts Resolved
…hild

The JUnit reporter adds a <properties> block to each testsuite when
GITHUB_SHA, CI_JOB_URL or a similar variable is set. The inline
snapshot does not expect it.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the CI env-var stripping in deaf4a7 addresses the <properties> snapshot concern I raised earlier.

What was reviewed: the rewritten runBailWithJUnit helper drains stdout/stderr/exited concurrently and asserts output before exit code; the env-var deletion list matches the keys JunitReporter::generate_properties_list reads; the no-validate-leaksan.txt entry carries a stated reason and tracked issue (#32183) alongside the existing 12250.test.ts entry for the same bail-exit mechanism; the fixture sources are fixed strings so the :2:40/:2:48 column offsets in the snapshot are stable.

Extended reasoning...

Overview

This PR touches two files: it rewrites test/regression/issue/26851.test.ts (a pre-existing regression test for bun test --bail + JUnit reporter) to use test.concurrent, shared helpers, full inline-snapshot assertions on normalized XML, and stricter stderr/exit-code checks; and it adds one entry to test/no-validate-leaksan.txt so the ASAN lane doesn't force BUN_DESTRUCT_VM_ON_EXIT=1 on a child that exits via bare exit(1). No production code is changed.

Security risks

None. This is a test-only refactor of an existing regression test plus a CI-runner allowlist entry. No auth, crypto, network, or user-input parsing is involved.

Level of scrutiny

Low-to-moderate. The one item that would normally warrant scrutiny — opting a test out of LeakSanitizer — is handled per REVIEW.md's requirement that every weakened safety net carry a stated reason: the entry has a two-line comment naming the mechanism (bare exit(1) skipping VM teardown → LSan abort 134) and the tracking issue (#32183), and sits directly beside 12250.test.ts which is on the list for the identical --bail exit path. The PR description names #39010 as the fix after which the entry is removed.

Other factors

I previously flagged that bunEnv spreads process.env without stripping GITHUB_*/CI_* vars, which would make the JUnit reporter emit a <properties> block and break the new inline snapshots on CI or dev boxes with those set. Commit deaf4a7 addresses this by deleting all seven keys the reporter reads before spawning. The rest of the diff follows harness conventions cleanly: tempDir with using, Promise.all on the three pipes, normalizeBunSnapshot for stdout, output assertions before exitCode, test.concurrent for independent subprocess spawns, and .not.toBe(0) tightened to .toBe(1). The added c_never.test.ts fixture and after the failure test give the assertions a negative contract (they must NOT appear), which is a genuine strengthening. No CODEOWNERS entries cover the changed paths.

@robobun

robobun commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 3:41 AM PT - Sep 5th, 2026

✅ @robobun, your commit deaf4a71bb02cabb71aed16f9070524ce4971f78 passed in Build #110380! 🎉


🧪   To try this PR locally:

bunx bun-pr 41423

That installs a local version of the PR into your bun-41423 executable, so you can run:

bun-41423 --bun

@Jarred-Sumner
Jarred-Sumner merged commit 9711c25 into main Sep 6, 2026
6 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the robobun/c08fe17c/speed-up-26851-test branch September 6, 2026 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants