Conversation
|
Status: superseded by #40348. Closing this one. #40348 moves the If the reorder in #40348 is not wanted, this PR is a rebased, green, narrow alternative and can be reopened. Reproduced with: printf '[test]\ncoverage = false\n' > bunfig.toml
bun test --coverage # no coverage table |
|
Warning Review limit reached
On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file. Or wait 20 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for addressing the three notes (Scope section, description tweak, and moving the tests into coverage.test.ts).
What was reviewed:
- The three
*_from_cliguards mirror the existingpath_ignore_patterns_from_clipattern; bunfig-only path (coverage = truewith no CLI flag) still applies since the flags default tofalse. --coverageCLI change: the removedif !ctx.test_options.coverage.enabledguard was dead (bunfig loads after CLI parsing), so switching toif args.flag(...)is behavior-preserving.- New tests cover all three flags plus the bunfig-only sanity case; each drains stdout/stderr concurrently and uses
tempDir/bunEnvper harness conventions.
Extended reasoning...
Overview
Fixes #12216: --coverage, --coverage-reporter, and --coverage-dir were being clobbered by [test] bunfig values because CLI parsing runs before bunfig loading and the bunfig loader wrote unconditionally. The fix adds three bool fields to TestOptions (coverage_from_cli, coverage_reporter_from_cli, coverage_dir_from_cli), sets them in parse_test_command_options when the corresponding flag is present, and guards the three bunfig assignments on those flags. Four tests are added to test/cli/test/coverage.test.ts.
Security risks
None. This is CLI/bunfig option-precedence plumbing for the test runner's coverage settings; no untrusted input parsing, network, or filesystem-path handling changes.
Level of scrutiny
Low-to-moderate. The change is small (~30 net lines of Rust, ~90 lines of tests), mechanical, and copies the established path_ignore_patterns_from_cli pattern already in the same file/struct. The old if !ctx.test_options.coverage.enabled guard on the CLI side was provably dead (default is false and bunfig hasn't loaded yet), so replacing it with if args.flag(b"--coverage") is behavior-preserving for the CLI-only case. --coverage is a presence-only flag (no --no-coverage), so setting coverage_from_cli only when the flag is present is correct.
Other factors
All three prior review comments are resolved: the sibling [test] keys (onlyFailures, reporter.dots, randomize, rerunEach, retry, seed) are explicitly named in the PR description's Scope section as an intentional deferral; the stale description sentence about type validation was dropped; and the tests were moved from test/regression/issue/12216.test.ts into test/cli/test/coverage.test.ts per the test-organization convention. No CODEOWNERS-gated paths are touched. Tests follow harness conventions (tempDir, bunEnv, await using, concurrent stdout/stderr/exited drain, exit-code asserted last, test.concurrent for independent subprocess spawns).
There was a problem hiding this comment.
LGTM — all three prior review nits (sibling-key scope note, description wording, test file placement) have been addressed.
What was reviewed:
- The three
*_from_cliguards mirror the existingpath_ignore_patterns_from_clipattern; bunfig type validation still runs forcoverage/coverageDirbefore the guard. - Confirmed the removed
if !ctx.test_options.coverage.enabledCLI-side guard was dead (bunfig loads after CLI parsing), so dropping it doesn't change behavior when--coverageis absent. - New tests drain all pipes concurrently, assert output before
exitCode, and include the negative control (coverage = truein bunfig with no CLI flag still enables coverage).
Extended reasoning...
Overview
The PR fixes #12216: bun test --coverage was clobbered by [test] coverage = false in bunfig because CLI flags are parsed before bunfig loads and the bunfig loader wrote unconditionally. The fix adds three boolean *_from_cli markers on TestOptions (coverage_from_cli, coverage_reporter_from_cli, coverage_dir_from_cli), sets them in parse_test_command_options when the corresponding flag is present, and gates the bunfig writes on them. Four subprocess tests are added to test/cli/test/coverage.test.ts. Total: ~30 lines of Rust across three files plus ~90 lines of tests.
Security risks
None. This is CLI/config precedence logic for the test runner's coverage reporting; no untrusted-input parsing paths, allocation sizes, or privileged operations are touched.
Level of scrutiny
Low-to-medium. The change is mechanical, follows an existing in-tree pattern (path_ignore_patterns_from_cli in the same struct/same bunfig block), and is confined to option plumbing. The one non-obvious edit — replacing if !ctx.test_options.coverage.enabled { ... = args.flag(...) } with if args.flag(...) { ... = true } — is safe because the old guard was always true at that point (bunfig hadn't loaded yet, so enabled was still its default false), so the flag-absent path is unchanged.
Other factors
I previously left three non-blocking comments on this PR; all were addressed: the PR description now has a Scope section deferring the sibling [test] keys, the inaccurate validation sentence was dropped, and the tests were moved from test/regression/issue/12216.test.ts into test/cli/test/coverage.test.ts. The new tests follow harness conventions (tempDir, bunEnv, test.concurrent, Promise.all on stdout/stderr/exited) and include a negative control confirming bunfig-only coverage = true still works. Verification section shows the tests fail under USE_SYSTEM_BUN=1 and pass under the debug build.
|
#40348 moves the |
…e bunfig CLI test options are parsed before bunfig.toml is loaded, and the bunfig loader was writing the [test] coverage/coverageReporter/coverageDir values unconditionally, so an explicit CLI flag was clobbered by the config value. Track whether each of these came from the CLI (same pattern as path_ignore_patterns_from_cli) and skip the bunfig assignment when so. Fixes #12216
Not a true regression (never worked), so per convention these belong next to the other coverage CLI tests rather than under test/regression/issue/.
0114802 to
9b5abba
Compare
There was a problem hiding this comment.
LGTM — small, focused fix that mirrors the existing path_ignore_patterns_from_cli pattern; all earlier nits have been addressed.
What was reviewed:
- Confirmed the three
*_from_cliguards match the establishedpathIgnorePatternsshape and that the removedif !coverage.enabledguard was dead (bunfig loads after CLI parse). - Checked that
--coverageremains a set-true-only flag (no behavior change when the flag is absent) and that bunfig type validation still runs forcoverage/coverageDir. - The four new tests cover CLI-overrides-bunfig for each flag plus the negative case (bunfig
coverage = truestill works with no CLI flag). - Noted the author's own heads-up about #40348; that's a possible follow-up simplification, not a blocker for this fix.
Extended reasoning...
Overview
The PR fixes #12216: --coverage, --coverage-reporter, and --coverage-dir were being clobbered by [test] bunfig values because CLI flag parsing runs before bunfig loading and the bunfig loader wrote unconditionally. The fix adds three *_from_cli booleans to TestOptions (src/options_types/context.rs), sets them in parse_test_command_options (src/runtime/cli/Arguments.rs), and guards the corresponding assignments in bunfig.rs. Four test.concurrent cases are added to test/cli/test/coverage.test.ts.
Security risks
None. This is CLI/bunfig option-precedence plumbing; no untrusted input parsing, auth, crypto, or filesystem-path handling is touched.
Level of scrutiny
Low-to-moderate. The change is ~30 src lines and mechanically copies the codebase's existing path_ignore_patterns_from_cli pattern (same struct, adjacent bunfig block). The removed if !ctx.test_options.coverage.enabled guard was provably dead — at that point in parse_test_command_options, bunfig has not loaded and coverage.enabled is always its default false, so the branch was always taken; replacing it with if args.flag(b"--coverage") is behavior-preserving for the no-flag case and correct for the flag case.
Other factors
- All three prior inline nits from earlier runs were addressed: a Scope section now names the deferred sibling keys (
onlyFailures,reporter.dots,randomize,rerunEach,retry,seed); the inaccurate description sentence aboutcoverageReportertype validation was dropped; and the tests were moved fromtest/regression/issue/12216.test.tsintotest/cli/test/coverage.test.ts. - Tests follow harness conventions (
tempDir,bunEnv, concurrent pipe-drain,using, exit-code asserted last) and include a negative case verifying bunfig still applies when no CLI flag is passed. - The PR body's evidence shows the new tests fail on main (both ASAN and release) and pass with the fix.
- No CODEOWNERS entries match the touched paths.
- The author's note about #40348 (reordering flag parse after bunfig load) is a potential future simplification; it does not change the correctness of this fix and can remove the
_from_cliflags later if it lands.
|
Closing in favor of #40348, which fixes the CLI-vs-bunfig ordering for all |
|
The three "overrides bunfig" coverage cases from this PR ( |
2 participants
Repro
Cause
parse_test_command_options(CLI flags) runs beforeload_config_with_cmd_args(bunfig). The bunfig loader then wrote[test] coverage/coverageReporter/coverageDirintoctx.test_optionsunconditionally, so the config value clobbered an explicit CLI flag. Theif !ctx.test_options.coverage.enabledguard on the CLI side was dead because bunfig had not loaded yet.Fix
Record whether each of
--coverage/--coverage-reporter/--coverage-dirwas passed on the CLI (same pattern the codebase already uses forpath_ignore_patterns_from_cli) and have the bunfig loader skip the assignment when the corresponding CLI flag was seen.Scope
This PR covers the coverage-family keys from #12216. The same clobber mechanism also applies to other
[test]keys (onlyFailures,reporter.dots,randomize,rerunEach,retry,seed); those are pre-existing and intentionally deferred to a follow-up to keep this change reviewable.Verification
Supersedes the coverage half of #28547 (stale, conflicting, bundles an unrelated fix).
Closes #12216
[review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file