3363: Throw error if unable to parse Mocha options file

[plroebuck]

Description of the Change

Code now throws an error for any problems loading the Mocha options file (except for nonexistent default "test/mocha.opts").

Builds off of previous discussions and work done in PR #2716 and PR #3352 .

Why should this be in core?

Silently ignoring problems loading this file is just wrong.

Benefits

Users will now be told if the file failed to load.

Possible Drawbacks

Users with invalid Mocha options files (or permissions problems, etc.) will need to correct the problem.

Applicable issues

Fixes #3363
Fixes #2576
(server-major) [since Mocha could throw an error when it didn't previously]

[coveralls]

Coverage decreased (-0.01%) to 89.995% when pulling 4d0aaa1 on plroebuck:issues/3363 into 7613521 on mochajs:master.

[plroebuck]

BTW, anyone run across this ChromeHeadless problem before? Same code ran file for all test jobs except this last one. Restarting made no difference. MochaJS PR run afterwards was fine.

[outsideris]

LGTM

[outsideris]

@plroebuck You didn't set saucelab in your travisci, seePlease set SAUCE_USERNAME and SAUCE_ACCESS_KEY env variables.

[plroebuck]

@outsideris, add "server-major" tag, pls!

[plroebuck]

@boneskull ping

[boneskull]

will review some stuff tomorrow

[Bamieh]

why is this change semver major?

[outsideris]

@Bamieh I think this is a breaking change because users will get the errors when there is no opts file. Before this change, users don't care about the existence of the opts file.

[Bamieh]

@outsideris makes sense, but looking at the code, it will only throw if there is an error in the existing file. if it does not exist, no errors will be throws. So i have a feeling that this is more of a bug fix rather than a breaking change.

• Users with no opt file will not be affected.
• Users with working opt files will not be affected.
• Users with broken opt files will have a new exception thrown that did not throw in the past.

Do you think there are people running mocha with a broken opt file? I believe its not that case because mocha would not be running as they expect anyways.

[plroebuck]

Not exactly, @outsideris... It's a breaking change because an Error could now occur where it does not in the current version. Even though the existing code is broken (due to masking), I consider it major since it might disrupt the CI process by causing run failures.

Mocha would now throw when user-specified --opt options file doesn't exist, cannot be read, or if a parsing error occurs due to its content. The default "test/mocha.opts" not existing will not trigger an error, but any other problem will.

[plroebuck]

| Do you think there are people running mocha with a broken opt file? I believe its not that case
| because mocha would not be running as they expect anyways.

Point being that they may not even know it's broken yet...
#2576

"confirmed-bug" label was correct.

[outsideris]

If so, it seems semver-minor, not semver-major like what @Bamieh said.

[plroebuck]

it's server-major if upgrading can break something.
https://blog.greenkeeper.io/introduction-to-semver-d272990c44f2

[boneskull]

This would be a bug if Mocha had intended to throw an error upon an invalid opts file, but failed to do so correctly.

Since Mocha's current behavior is intended (which is to say, doing nothing) this isn't a bug fix. But it is a breaking change.

[boneskull]

It should be possible to write an integration test for this. You can create a dummy invalid mocha.opts file somewhere under test/integration/fixtures/ and use the runMocha helper with args --opts /path/to/bad/mocha.opts, and assert the output contains an error.

This will also appease Coveralls.

[plroebuck]

Had problems getting this branch updated post-UnexpectedJS. Opening new PR instead.