Repository navigation
fix: --reporter-option on CLI overrides values from config (#5532) - #5986
devareddy05 wants to merge 1 commit into
Conversation
275a63d to
2e71d62
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5986 +/- ##
==========================================
- Coverage 78.12% 77.92% -0.21%
==========================================
Files 63 63
Lines 4590 4602 +12
Branches 997 984 -13
==========================================
Hits 3586 3586
- Misses 1004 1016 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Test failure is known issue #6036 |
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Thanks for the contribution! And sorry it took so long to review. But:
- We've since refactored this area of logic in Mocha significantly. See #6164 & co.
- This was clearly authored with AI in a way that runs counter to the AI-Generated Code section of our contributing guidelines
For just one of those I'd request changes, but for both I'm going to go ahead and close this out. Feel free to send another PR if you'd like though - I left a couple of thoughts on this one. Cheers!
| * @alias module:lib/cli.loadOptions | ||
| * @returns {external:yargsParser.Arguments} Parsed args from everything | ||
| */ | ||
| // Reads `reporter-option` from a config source under any of its known | ||
| // aliases. Keeps `loadOptions` agnostic of how each source was authored. | ||
| const readReporterOption = (source) => |
There was a problem hiding this comment.
[Comments] This // comment conflicts with the existing JSDoc for the function. This should add to the JSDoc instead.
| ].reduce((acc, src) => (src ? acc.concat(list(src)) : acc), []); | ||
| if (orderedReporterOption.length) { | ||
| args["reporter-option"] = orderedReporterOption; | ||
| } |
There was a problem hiding this comment.
[Refactor] This is quite a lot of complexity for a pretty small area of bug. And, we actually refactored away yargs & yargs-parser on main in the meantime. The mergeConfigObjects we added on main would be a good place for this kind of thing.
PR Checklist
status: accepting prsOverview
--reporter-option(--reporter-options,-O) given on the CLI did not override the same key declared in.mocharc.*orpackage.json. The config-file value won instead. Regression observed since v8.3.0, still present in v11.7.5+. The maintainer comment on the issue narrowed it to "Mocha not recognizing the clashing options as 'clashing'".Root cause
In
lib/cli/options.mjs, the second call toparse(...)passes CLI args, env, RC, andpackage.jsonasconfigObjectsin priority-HIGH→LOW order.yargs-parser'scombine-arrays: trueconcatenates thereporter-optionarray from every source in that order:[CLI, env, rc, pkg]. The downstream coerce inlib/cli/run.js(around L223-L243) then reduces this array left-to-right with last-writer-wins — so the lowest-priority source ends up taking precedence, which is the inverse of the intended priority order.Fix
After the second parse, re-emit
reporter-optionin priority-LOW→HIGH order (pkg, rc, env, CLI). The coerce's existing last-writer-wins reduction now yields the correct result: CLI overrides env overrides rc overrides pkg. Per-key merge semantics are preserved — CLI overrides only the clashing keys; non-clashing keys from the config still flow through.A small helper (
readReporterOption) reads the value under any of the supported source-side aliases (reporter-option,reporter-options,reporterOption,reporterOptions,O) so config files written with either casing/spelling are respected.Behavior matrix (verified locally)
output=cli.xml, configoutput=config.xmlcli.xmloutput=cli.xml, configoutput=config.xml,suiteName=FromConfig{output: 'cli.xml', suiteName: 'FromConfig'}foo=bar foo=baz, no config{foo: 'baz'}(last wins, unchanged)foo=fromEnv, configfoo=fromConfigTests
test/integration/options/reporter-option.spec.js: "should allow the CLI to override clashing keys from the config", driving Mocha via--config <fixture-yaml>+--reporter-option foo=fromCliand asserting the custom reporter prints{"foo":"fromCli","extra":"keepMe"}.test/integration/fixtures/options/reporter-option-mocharc.yml.Validation
npm run format:check,npm run tsc— cleannpm run test-smoke— 1/1 passingnpm run -s test-node-run -- "test/integration/options/reporter-option.spec.js"— 4/4 passing (3 pre-existing + 1 new)npm run -s test-node-run -- "test/integration/options/*.spec.js"— 158/158 passingnpm run test-node:unit— 1008 passing, 3 pendingCloses #5532