fix: correct precedence of configuration extends and defaults

[manuth]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions.

Description of the Change

As described in issue #4979, due to mocha first loading its default settings and after doing so processing the arguments using args, there are some settings which cannot be inherited by specifying an extends file:

• First, default values get loaded:
  

  mocha/lib/cli/options.js

  Line 246 in 37deed2

  args = parse(args._, mocharc, args, rcConfig || {}, pkgConfig || {});

• Sometime later, the configuration gets processed:
  

  mocha/lib/cli/cli.js

  Line 77 in 37deed2

  .config(args)

Changes made in this PR processes configuration files using yargs right after they have been parsed. Thus, the order of events is now the following:

• RC files are parsed
  • RC files are processed using yargs
• package.json settings are loaded
• Collected configs and argv get merged
• mochas default values (from lib/mocharc.json) get applied

Alternate Designs

Configuration files could be processed right before mochas defaults get applied. However, the context of the file path would get lost thus causing relative extends paths to possibly fail.

Applying mochas default values could be removed from here:
https://github.com/mochajs/mocha/blob/37deed262d4bc0788d32c66636495d10038ad398/lib/cli
And instead be done somewhere around here (after yargs has been executed):

mocha/lib/cli/cli.js

Line 47 in 37deed2

var args = mochaArgs || loadOptions(argv);

Why should this be in core?

Currently, the inheritance will not work for a number of settings while other settings do. This might cause confusion and unexpected results. Thus, I'd recommend to have this change in core.

Benefits

All settings will be able to be inherited while mochas defaults still get applied if necessary.

Possible Drawbacks

None that I have heard of.
If people already used extends for their own project, there is a chance that they relied on the inheritance of ui to not work, so there is a small possibility they will have to perform a one-time change to their config file.

Applicable issues

• Enter any applicable Issues here.

  • 🐛 Bug: Some Settings Cannot Be Inherited (extends keyword) #4979
  • 🐛 Bug: Configurations don't seem to support 'extends' for some settings #3916
  • Original PR: fix: fully support 'extends' for configurations #4407

• Mocha follows semantic versioning: http://semver.org

• Is this a breaking change (major release)?

• Is it an enhancement (minor release)? (Maybe?)

• Is it a bug fix, or does it not impact production code (patch release)?

Or probably a minor as there is a small possibility people have to adjust their configuration if they used extends before

Fixes #4979
Fixes #3916

Co-authored-by: Pat Herlihy @mf-pherlihy

[github-actions]

This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!

[manuth]

Just pinging to have the stale label removed
I'd really love to see this implemented

[JoshuaKGoldberg]

Note: #4407 also touches this area. We should review them together in case one is a dup.

[JoshuaKGoldberg]

Seems good and reasonable, nice! 🔥

Marking as requesting changes just so we can chat about the two possible implementations a bit.

[JoshuaKGoldberg]

Marking as semver-major because some projects might accidentally be relying on the current, arguably broken behavior.

Conversations resolved.

[JoshuaKGoldberg]

👋 just checking in @manuth, anything I can help with?

[manuth]

Alright, so I made a little digging.
Of course, I might be wrong with my conclusions, so I'm open to feedback.

As described in issue #4979, the underlying issue that causes Mocha to ignore certain settings from extends-files is, that Mocha first applies its defaults to the provided configuration and, only after that, processes the config using yargs. Let's call this behaviour "Late Yargs Processing", which is explained in detail in it's own section.

In order to fix this, yargs must be run early or more precisely:
yargs must run before Mocha's global defaults are applied.

My solution was to run yargs in the function used by Mocha to load config files:

mocha/lib/cli/config.js

Lines 81 to 83 in f131e9a

	config = yargs(undefined, path.dirname(filepath))
	.parserConfiguration(require('./options').YARGS_PARSER_CONFIG)
	.config(config);

The solution proposed by @mf-pherlihy is to process the config using yargs not while loading the configuration but rather after loading the configuration, in the parse function, which is still before Mocha applies its global defaults:

mocha/lib/cli/options.js

Lines 130 to 142 in f07d427

	configObjects: configObjects.map(configObject => {
	// '_configPath' gets set by config.loadConfig()
	const config = configObject || {};
	const configPath =
	typeof config._configPath === 'string'
	? path.dirname(config._configPath)
	: cwd();
	// Remove the internal property
	delete config._configPath;
	return applyExtends(config, configPath, true);
	}),

Conclusion

Of course, I might be biased as former solution is written by me while latter isn't.
I consider my solution more clean and future-proof. Especially considering that loadRc is an exported function and using it might re-introduce the issue if my solution weren't implemented.

Furthermore, it uses yarg's yargs-function which seems like "more public" API to me than the yargs/helpers=> applyExtends function (I guess?).

However, there are more...

Things to Consider

I have just noticed that RC config files aren't the only place where the extends option reasonably might occur. One more spot is the package.json file.

I'd recommend adding a function (like processConfig(config, filePath) - recommendations welcome, though) to lib/cli/options.js which processes expands the extends option using yargs that we can use for both RC config files (in the loadRc function) and the package.json file (in the loadPkgRc function).
For this to happen, slight changes need to be made to this PR.

One more thing is that currently, because yargs has no knowledge of the original location of configuration files, the extends-file path is relative to the working directory. After implementing either of the solutions, the extends-file will be relative to the corresponding configuration file. This should be mentioned somewhere in the changelog, I guess.

Late Yargs Processing

Mocha processing the configuration using yargs at a late stage causes certain settings from extends-files to be ignored.
I will describe a case in some greater detail here.

Let's imagine a config file:
.mocharc.json:

{
  "extends": "./base.json",
  "timeout": 1000
}

And base config:
base.json:

{ "ui": "tdd" }

First, at this line, Mocha will apply the global defaults to the config:

mocha/bin/mocha.js

Line 27 in 75dcf8c

const opts = loadOptions(process.argv.slice(2));

or, when called through _mocha:

mocha/lib/cli/cli.js

Line 51 in 75dcf8c

var args = mochaArgs || loadOptions(argv);

Making the runtime-configuration at this point look like this:

Runtime-Config After loadOptions:

{
  "_": [],
  "config": false,
  "package": false,
  "extends": "./base.json",
  "timeout": 1000,
  "diff": true,
  "extension": [
    "js",
    "cjs",
    "mjs"
  ],
  "reporter": "spec",
  "slow": 75,
  "ui": "bdd",
  "watch-ignore": [
    "node_modules",
    ".git"
  ]
}

Note:
Take especial note that the ui setting now is already set to Mocha's global default bdd loaded from the mocharc variable here:

mocha/lib/cli/options.js

Lines 315 to 322 in 75dcf8c

	args = parse(
	args._,
	mocharc,
	args,
	envConfig,
	rcConfig || {},
	pkgConfig || {}
	);

After that, yargs is executed on the config here:

mocha/lib/cli/cli.js

Line 81 in 75dcf8c

.config(args)

You will notice, that the configuration ui option remains unchanged. That's, ofc, as you might remember from the previous Runtime-Config dump, because of the fact that a few settings, including ui, are now set in the config object, which override the ui set in the extends-file. Thus, the ui option and all further global defaults set in the mocharc variable cannot be set using extends.

At time of writing, the mocharc variable is loaded from this file:
https://github.com/mochajs/mocha/blob/75dcf8c6c40ed1ce134ae5e174b6f4c4ca4d8c42/lib/mocharc.json

[manuth]

So... uh... looks like I did a little oupsie in this PR:
I forgot to load the config of the yargs result from the argv property, effecitvely causing the majority of the tests not to run at all and instead printing the mocha version number to the console.

I fixed this in commit a41006d by adding .argv to the end of the statement.

Furthermore, as discussed in my previous comment, I added a new function loadRcFile which processes the configuration of a config file or package.json-file using yargs, which takes care of the extends option.

Now, extends works properly for both package.json and config files.

I'm ready for a review, I guess

[JoshuaKGoldberg]

🔥 Whoohoo! I love how the changes in lib/cli/options.js are super straightforward now - just one extracted function for the two loading places.

I tried this out in the https://github.com/mochajs/mocha-examples packages that use a mocha config file and nothing was broken (that wasn't already broken...). So that's great.

I also think that because this is a breaking change for the next behavior-changing major, we'll have an alpha/beta/rc out in public for folks to try it out with. So I say 🚀 great! The only blocker now is waiting for us to get to being able to working on that major.

Marking as approved, with the notes that:

• Ideally if someone else in @mochajs/maintenance-crew has time, they should review too
• We're blocked from merging until the work on the next major gets going

Thanks @manuth! 🙌

[JoshuaKGoldberg]

yargs must run before Mocha's global defaults are applied.

ACK, that seems reasonable to me. As I understand it: Mocha-specific global defaults are meant to tack on if nothing is specified in Yargs-land. Having Yargs-land finish before Mocha-land starts seems right.

[manuth]

Awesome! Thank you so much for reviewing - and also thanks for the feedback, I really appreciate it 😄