Conversation
Passing --coverage-reporter=lcov without --coverage silently produced no coverage output. Same for --coverage-dir. Both flags now enable coverage when passed on the CLI. bunfig.toml keys (coverageReporter, coverageDir) are intentionally left alone so a preconfigured reporter doesn't force coverage on every test run; the explicit coverage = true key already exists for that. Fixes #17502
|
Updated 3:40 PM PT - Jun 16th, 2026
❌ @autofix-ci[bot], your commit 4f2c2f0 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32420That installs a local version of the PR into your bun-32420 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesImplicit Coverage Activation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
CI is currently blocked on agent provisioning ( |
There was a problem hiding this comment.
LGTM — two-line flag-implication fix with matching integration tests; the help-text note is a non-blocking nit.
Extended reasoning...
Overview
This PR adds two lines to parse_test_command_options in src/runtime/cli/Arguments.rs, setting ctx.test_options.coverage.enabled = true inside the existing --coverage-reporter and --coverage-dir handlers. It also adds four integration tests in test/cli/test/coverage.test.ts (plus an existsSync import) verifying that each flag now produces coverage output without an explicit --coverage. Fixes #17502.
Security risks
None. This is a CLI ergonomics change in the test runner's argument parser — no auth, crypto, filesystem-permission, or network surface is touched. The only behavioral effect is enabling coverage instrumentation when the user has already signaled coverage intent via a related flag.
Level of scrutiny
Low. The change is mechanical and follows established precedent in the same function: --seed already sets randomize = true, and --parallel already sets isolate = true. The new assignments sit inside pre-existing if blocks, so no new control flow is introduced. The PR description explicitly justifies leaving the bunfig equivalents (coverageReporter, coverageDir) untouched, which is the right call — a preconfigured reporter shouldn't force coverage on every bun test.
Other factors
- Neither file is covered by CODEOWNERS.
- The new tests mirror existing patterns in
coverage.test.ts(tempDirWithFiles,bunExe(),bunEnv, spawnSync) and assert on both lcov-file and text-table outputs. - CI failures are infrastructure-only ("Failed to create agent" / image-not-found across all lanes including main); the author reports the full
coverage.test.tssuite (16 tests) passes locally. - The only bug-hunter finding is a documentation nit suggesting "Implies --coverage." be appended to the help strings — worth doing but not a blocker.
There was a problem hiding this comment.
LGTM — thanks for adding the "Implies --coverage." help text.
Extended reasoning...
Overview
Two-line functional change in src/runtime/cli/Arguments.rs: when --coverage-reporter or --coverage-dir is parsed, set ctx.test_options.coverage.enabled = true. Accompanying help-string updates in TEST_ONLY_PARAMS, a matching docs snippet update in docs/snippets/cli/test.mdx, and four new integration tests in test/cli/test/coverage.test.ts covering both = and space-separated forms plus the --coverage-dir case.
Security risks
None. This is CLI flag parsing for the test runner's coverage feature; no auth, crypto, filesystem-path, or untrusted-input handling is altered. The new assignments only flip a boolean that was already settable via the existing --coverage flag.
Level of scrutiny
Low. The functional diff is two unconditional boolean assignments inside existing if blocks that already guard on the flag being present. There is no --no-coverage flag to conflict with, and the existing --coverage handling at line 1561-1562 already only writes when not previously enabled, so ordering/override concerns don't apply. The bunfig path is intentionally untouched per the PR description, which is the right call. None of the touched files are CODEOWNER-protected.
Other factors
My earlier nit (document the implication in help text) was addressed in c67cec2 and the inline thread is resolved. The bug-hunting pass found nothing. New tests follow the established tempDirWithFiles + Bun.spawnSync pattern from the same file and assert on observable behavior (lcov.info on disk, % Funcs/% Lines in stderr). The CI failure is fleet-wide agent provisioning (Image not found: linux-*-v37), unrelated to this diff, and the suite was verified green locally per the author.
|
Stale PR review: keep open, rework. The The Wanted shape:
|
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-16, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#17502) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Fixes #17502 ### Problem - `bun test --coverage-reporter=lcov` runs the tests, exits 0, and writes no report. It prints no warning. - The `--coverage-reporter` block of `parse_test_command_options` (`src/runtime/cli/Arguments.rs:1798`) sets `coverage.reporters` and never `coverage.enabled`. ### Fix - That block now sets `coverage.enabled = true`. The help text and `docs/snippets/cli/test.mdx` say `Implies --coverage`. - `--coverage-dir` and the bunfig key `coverageReporter` do not change (see Notes). - Verified: `test/cli/test/coverage.test.ts`, 5 new tests (lcov, the separate-argument form, text, both reporters, `--parallel=2`). The released build fails 5 of 5. A debug build with the change passes them, and the whole file passes (32 tests). ### Background - `bun test` collects coverage only when `coverage.enabled` is true. `--coverage` or `coverage = true` in `bunfig.toml` sets it. - A `--parallel` coordinator starts its workers with `--coverage` when `coverage.enabled` is true (`src/runtime/cli/test/parallel/runner.rs:422`), so the same switch covers that mode. - No other place was weighed: one block parses the flag. #20736 and #32420 made the same change, and a review of #32420 asked for the reporter half only. ### Downsides - A run with `--coverage-reporter` and no `--coverage` now collects coverage. It is slower, it prints the table or writes `coverage/lcov.info`, and a `coverageThreshold` in `bunfig.toml` can now fail it. - `--coverage-dir` with neither flag is still dropped with no message. - Other runs pay nothing: one assignment at argument parsing, only when the flag is present. The help text grows by 20 bytes. <details><summary>Notes</summary> **Rows, before and after.** A directory with `f.ts` and `f.test.ts`: | command | released build | with the change | | --- | --- | --- | | `bun test --coverage` | text table | text table | | `bun test --coverage --coverage-reporter=lcov` | `coverage/lcov.info` | `coverage/lcov.info` | | `bun test --coverage-reporter=lcov` | nothing | `coverage/lcov.info` | | `bun test --coverage-reporter=text` | nothing | text table | | `bun test --coverage-dir=cov2 --coverage-reporter=lcov` | nothing | `cov2/lcov.info` | | `bun test` | nothing | nothing | **Why `--coverage-dir` stays.** Only the lcov writer reads the directory (`src/runtime/cli/test_command.rs:1631-1681`). `bun test --coverage-dir=out` alone would print the text table and never create `out/`. The comparable flags `--cpu-prof-dir` and `--heap-prof-dir` do not turn their switch on. They fail with `must be used with --cpu-prof` (`Arguments.rs:1392`, `:1449`). Whether `--coverage-dir` alone must be an error is an open question. **Why the bunfig key stays.** A reporter that is set in `bunfig.toml` must not turn coverage on for each `bun test` run. `coverage = true` is the key for that. **`bunfig.toml` wins over the flags today.** With `coverage = false` in `bunfig.toml`, `--coverage` does not turn coverage on, and `--coverage-reporter` does not either. With `coverageReporter = "lcov"` there, `--coverage-reporter=text` writes `lcov.info`. #40348 (open) changes that order and edits the same function. **Threshold.** With `coverageThreshold = 1.0` and one function that no test calls: `bun test` exits 0, `bun test --coverage` exits 1, and `bun test --coverage-reporter=lcov` now exits 1 too. **Earlier pull requests.** #17737, #20736, #32420. The last one was closed by a cleanup of stale pull requests, with no objection to the change. </details>
Fixes #17502.
Repro
No coverage output is produced unless
--coverageis also passed.Cause
src/runtime/cli/Arguments.rsparses--coverage-reporterand--coverage-dirintoctx.test_options.coverage.{reporters,reports_directory}but never setscoverage.enabled, so the test runner skips coverage collection entirely.Fix
Set
ctx.test_options.coverage.enabled = truewhen either--coverage-reporteror--coverage-diris passed on the CLI.The bunfig keys (
coverageReporter,coverageDir) are intentionally left as-is: a preconfigured reporter in bunfig.toml shouldn't force coverage on everybun testrun. Users who want that already havecoverage = true.Verification
New tests in
test/cli/test/coverage.test.ts:--coverage-reporter=lcov/--coverage-reporter lcovalone writescoverage/lcov.info--coverage-reporter=textalone prints the coverage table--coverage-dir=outalone prints the coverage table (default text reporter)All four fail on main and pass with this change. Full
coverage.test.tssuite (16 tests) passes.