-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
base: master
Are you sure you want to change the base?
Changes from all commits
bd47d79
4aad184
8983b84
b6d6dc2
9b3d5d2
645e0b0
0e1e84a
9685b82
f8aedff
a6bdef4
909834d
cf69df1
b905d21
ea092be
6d4170e
bad240d
21e566d
dce26f4
4c9f318
1a9f155
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'); | ||||||||||||
|
@@ -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 = ({ | ||||||||||||
|
@@ -30,7 +31,7 @@ module.exports = ({ | |||||||||||
sort, | ||||||||||||
spec | ||||||||||||
} = {}) => { | ||||||||||||
const unmatched = []; | ||||||||||||
const unmatchedSpecFiles = []; | ||||||||||||
const specFiles = spec.reduce((specFiles, arg) => { | ||||||||||||
try { | ||||||||||||
const moreSpecFiles = castArray(lookupFiles(arg, extension, recursive)) | ||||||||||||
|
@@ -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(); | ||||||||||||
|
@@ -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 | ||||||||||||
}; | ||||||||||||
}; | ||||||||||||
|
||||||||||||
/** | ||||||||||||
|
@@ -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 | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||
* @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. | ||||||||||||
*/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
@@ -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(); | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
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'}); | ||
}); | ||
}); |
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 = []; | ||||||||||
|
@@ -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) { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Naming]
Suggested change
|
||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Naming] This confused me:
Is the intent that it's generally testing CommonJS files (
Suggested change
|
||||||||||
|
||||||||||
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'} | ||||||||||
); | ||||||||||
}); | ||||||||||
}); |
There was a problem hiding this comment.
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