Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add error handling for nonexistent file case with --file option #5086

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 39 additions & 7 deletions lib/cli/collect-files.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

const fs = require('fs');
const path = require('path');
const ansi = require('ansi-colors');
const debug = require('debug')('mocha:cli:run:helpers');
Expand All @@ -19,7 +20,7 @@ const {castArray} = require('../utils');
/**
* Smash together an array of test files in the correct order
* @param {FileCollectionOptions} [opts] - Options
* @returns {string[]} List of files to test
* @returns {FileCollectionResponse} An object containing a list of files to test and unmatched files.
* @private
*/
module.exports = ({
Expand All @@ -30,7 +31,7 @@ module.exports = ({
sort,
spec
} = {}) => {
const unmatched = [];
const unmatchedSpecFiles = [];
Copy link
Author

Choose a reason for hiding this comment

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

Renamed variable for semantic clarity

const specFiles = spec.reduce((specFiles, arg) => {
try {
const moreSpecFiles = castArray(lookupFiles(arg, extension, recursive))
Expand All @@ -44,14 +45,34 @@ module.exports = ({
return [...specFiles, ...moreSpecFiles];
} catch (err) {
if (err.code === NO_FILES_MATCH_PATTERN) {
unmatched.push({message: err.message, pattern: err.pattern});
unmatchedSpecFiles.push({message: err.message, pattern: err.pattern});
return specFiles;
}

throw err;
}
}, []);

// check that each file passed in to --file exists

const unmatchedFiles = [];
fileArgs.forEach(file => {
const fileAbsolutePath = path.resolve(file);
try {
require.resolve(fileAbsolutePath);
} catch (err) {
if (err.code === 'MODULE_NOT_FOUND') {
unmatchedFiles.push({
pattern: file,
absolutePath: fileAbsolutePath
});
return;
}

throw err;
}
});

// ensure we don't sort the stuff from fileArgs; order is important!
if (sort) {
specFiles.sort();
Expand All @@ -67,19 +88,24 @@ module.exports = ({
if (!files.length) {
// give full message details when only 1 file is missing
const noneFoundMsg =
unmatched.length === 1
? `Error: No test files found: ${JSON.stringify(unmatched[0].pattern)}` // stringify to print escaped characters raw
unmatchedSpecFiles.length === 1
? `Error: No test files found: ${JSON.stringify(
unmatchedSpecFiles[0].pattern
)}` // stringify to print escaped characters raw
: 'Error: No test files found';
console.error(ansi.red(noneFoundMsg));
process.exit(1);
} else {
// print messages as a warning
unmatched.forEach(warning => {
unmatchedSpecFiles.forEach(warning => {
console.warn(ansi.yellow(`Warning: ${warning.message}`));
});
}

return files;
return {
files,
unmatchedFiles
};
};

/**
Expand All @@ -92,4 +118,10 @@ module.exports = ({
* @property {string[]} file - List of additional files to include
* @property {boolean} recursive - Find files recursively
* @property {boolean} sort - Sort test files
* @typedef {Object} UnmatchedFile - Diagnostic object containing unmatched files
Copy link
Member

Choose a reason for hiding this comment

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

[Docs] I believe it's standard to have a separate JSDoc tag for each object declaration?

Suggested change
* @typedef {Object} UnmatchedFile - Diagnostic object containing unmatched files
*/
/**
* @typedef {Object} UnmatchedFile - Diagnostic object containing unmatched files

* @property {string} absolutePath - A list of unmatched files derived from the file arguments passed in.
* @property {string} pattern - A list of unmatched files derived from the file arguments passed in.
* @typedef {Object} FileCollectionResponse
* @property {string[]} files - A list of files to test
* @property {UnmatchedFile[]} unmatchedFiles - A list of unmatched files derived from the file arguments passed in.
*/
47 changes: 41 additions & 6 deletions lib/cli/run-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

const fs = require('fs');
const path = require('path');
const ansi = require('ansi-colors');
const debug = require('debug')('mocha:cli:run:helpers');
const {watchRun, watchParallelRun} = require('./watch-run');
const collectFiles = require('./collect-files');
Expand Down Expand Up @@ -117,9 +118,25 @@ exports.handleRequires = async (requires = [], {ignoredPlugins = []} = {}) => {
* @private
*/
const singleRun = async (mocha, {exit}, fileCollectParams) => {
const files = collectFiles(fileCollectParams);
debug('single run with %d file(s)', files.length);
mocha.files = files;
const fileCollectionObj = collectFiles(fileCollectParams);

if (fileCollectionObj.unmatchedFiles.length > 0) {
fileCollectionObj.unmatchedFiles.forEach(({pattern, absolutePath}) => {
console.error(
ansi.yellow(
`Warning: Cannot find any files matching pattern "${pattern}" at the absolute path "${absolutePath}"`
)
);
});
console.log(
'No test file(s) found with the given pattern, exiting with code 1'
);

return mocha.run(exitMocha(1));
}

debug('single run with %d file(s)', fileCollectionObj.files.length);
mocha.files = fileCollectionObj.files;

// handles ESM modules
await mocha.loadFilesAsync();
Expand All @@ -140,9 +157,27 @@ const singleRun = async (mocha, {exit}, fileCollectParams) => {
* @private
*/
const parallelRun = async (mocha, options, fileCollectParams) => {
const files = collectFiles(fileCollectParams);
debug('executing %d test file(s) in parallel mode', files.length);
mocha.files = files;
const fileCollectionObj = collectFiles(fileCollectParams);

if (fileCollectionObj.unmatchedFiles.length > 0) {
fileCollectionObj.unmatchedFiles.forEach(({pattern, absolutePath}) => {
console.error(
ansi.yellow(
`Warning: Cannot find any files matching pattern "${pattern}" at the absolute path "${absolutePath}"`
)
);
});
console.log(
'No test file(s) found with the given pattern, exiting with code 1'
);
return mocha.run(exitMocha(1));
}
Comment on lines +160 to +174
Copy link
Member

Choose a reason for hiding this comment

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

[Refactor] This is about the same as the previous block. Up for extracting out a function?

Having two areas of code that are roughly the same isn't the worst thing in the world, but given how they're pretty much exactly identical - I think it'd make the code more readable & easy to understand. Plus if we have to change it later, it'll be easy to forget one.


debug(
'executing %d test file(s) in parallel mode',
fileCollectionObj.files.length
);
mocha.files = fileCollectionObj.files;

// note that we DO NOT load any files here; this is handled by the worker
return mocha.run(options.exit ? exitMocha : exitMochaLater);
Expand Down
4 changes: 2 additions & 2 deletions lib/cli/watch-run.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ exports.watchParallelRun = (
newMocha.suite.ctx = new Context();

// reset the list of files
newMocha.files = collectFiles(fileCollectParams);
newMocha.files = collectFiles(fileCollectParams).files;

// because we've swapped out the root suite (see the `run` inner function
// in `createRerunner`), we need to call `mocha.ui()` again to set up the context/globals.
Expand Down Expand Up @@ -120,7 +120,7 @@ exports.watchRun = (mocha, {watchFiles, watchIgnore}, fileCollectParams) => {
newMocha.suite.ctx = new Context();

// reset the list of files
newMocha.files = collectFiles(fileCollectParams);
newMocha.files = collectFiles(fileCollectParams).files;

// because we've swapped out the root suite (see the `run` inner function
// in `createRerunner`), we need to call `mocha.ui()` again to set up the context/globals.
Expand Down
7 changes: 7 additions & 0 deletions test/integration/fixtures/collect-files.fixture.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
var obj = {foo: 'bar'};

describe('mjs', function () {
it('should work', function () {
expect(obj, 'to equal', {foo: 'bar'});
});
});
86 changes: 83 additions & 3 deletions test/integration/options/file.spec.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
'use strict';

var path = require('path').posix;
var helpers = require('../helpers');
var runMochaJSON = helpers.runMochaJSON;
var resolvePath = helpers.resolveFixturePath;
const {
runMochaJSON,
resolveFixturePath: resolvePath,
runMocha
} = require('../helpers');

describe('--file', function () {
var args = [];
Expand Down Expand Up @@ -64,4 +66,82 @@ describe('--file', function () {
done();
});
});

it('should run esm tests passed via file', function (done) {
const esmFile = 'collect-files.fixture.mjs';
const testArgs = ['--file', resolvePath(esmFile)];

runMochaJSON(esmFile, testArgs, function (err, res) {
if (err) {
return done(err);
}
expect(res, 'to have passed');
done();
});
});

it('should log a warning if a nonexistent file is specified', function (done) {
Copy link
Member

Choose a reason for hiding this comment

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

[Naming]

Suggested change
it('should log a warning if a nonexistent file is specified', function (done) {
it('should log a warning if a nonexistent file with an unknown extension is specified', function (done) {

const nonexistentTestFileArg = 'nonexistent.test.ts';
runMocha(
nonexistentTestFileArg,
['--file'],
function (err, res) {
if (err) {
return done(err);
}

expect(
res.output,
'to contain',
`Warning: Cannot find any files matching pattern`
).and('to contain', nonexistentTestFileArg);
done();
},
{stdio: 'pipe'}
);
});

it('should provide warning for nonexistent cjs file extensions', function (done) {
const nonexistentCjsArg = 'nonexistent.test.js';
Comment on lines +104 to +105
Copy link
Member

Choose a reason for hiding this comment

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

[Naming] This confused me:

nonexistent cjs file extensions
.test.js

Is the intent that it's generally testing CommonJS files (.js)? I think the name could be a bit more clear. Maybe ...

Suggested change
it('should provide warning for nonexistent cjs file extensions', function (done) {
const nonexistentCjsArg = 'nonexistent.test.js';
it('should provide warning for nonexistent js file extensions', function (done) {
const nonexistentCjsArg = 'nonexistent.test.js';


runMocha(
nonexistentCjsArg,
['--file'],
function (err, res) {
if (err) {
return done(err);
}

expect(
res.output,
'to contain',
`Warning: Cannot find any files matching pattern`
).and('to contain', nonexistentCjsArg);
done();
},
{stdio: 'pipe'}
);
});

it('should provide warning for nonexistent esm file extensions', function (done) {
const nonexistentEsmArg = 'nonexistent.test.mjs';

runMocha(
nonexistentEsmArg,
['--file'],
function (err, res) {
if (err) {
return done(err);
}

expect(
res.output,
'to contain',
`Warning: Cannot find any files matching pattern`
).and('to contain', nonexistentEsmArg);
done();
},
{stdio: 'pipe'}
);
});
});