Skip to content

Commit

Permalink
Print error instead of warning
Browse files Browse the repository at this point in the history
  • Loading branch information
@benedikt-roth
benedikt-roth committed Apr 25, 2018
1 parent 8b200d3 commit 2dbc42f
Showing 1 changed file with 3 additions and 5 deletions.
8 changes: 3 additions & 5 deletions bin/options.js
View file
Expand Up @@ -30,11 +30,9 @@ function getOptions() {
: 'test/mocha.opts';

if (optPathSpecified && !fs.existsSync(optsPath)) {
console.error(
`Warning: Could not find specified options file: ${optsPath}
Continuing with default configuration.\n`
);
return;
const noSuchFileError = new Error(`Specified options file does not exist: ${optsPath}`);
noSuchFileError.code = 'ENOENT';
throw noSuchFileError;
}

try {
Expand Down

2 comments on commit 2dbc42f

@plroebuck
Copy link
Contributor

@plroebuck plroebuck commented on 2dbc42f Apr 25, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Nice work, Benedikt!

@plroebuck
Copy link
Contributor

@plroebuck plroebuck commented on 2dbc42f Apr 25, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have couple things related to code following your change:

mocha/bin/options.js

Lines 38 to 51 in 2dbc42f

try {
const opts = fs
.readFileSync(optsPath, 'utf8')
.replace(/\\\s/g, '%20')
.split(/\s/)
.filter(Boolean)
.map(value => value.replace(/%20/g, ' '));
process.argv = process.argv
.slice(0, 2)
.concat(opts.concat(process.argv.slice(2)));
} catch (err) {
// ignore
}

  1. Appears if anything goes wrong reading "mocha.opts", parsing its output, or splicing it back into the process arguments -- we ignore the error and set LOADED_MOCHA_OPTS as though it was successful? WTH? Error should not be suppressed -- at least print it out...

mocha/bin/options.js

Line 53 in 2dbc42f

process.env.LOADED_MOCHA_OPTS = true;

  1. Environment variables are strings; assigning it a boolean value is code smell, even if Node converts it for you. Should be:
process.env.LOADED_MOCHA_OPTS = 'true';
  1. What does the environment variable mean anyway? If taken at its name, seems that it should be moved to be the last statement in the try block. (#L48.5).

Please sign in to comment.