-
-
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?
Conversation
- added error handling when using the --file flag to do it the way --require does - added a test to assert that we throw the same type of error
- require.resolve() by Node.js follows a Node.js module resolution algo which includes checking if the resolved path actually exists on the file system.
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.
Cool! 😎
Requesting changes on adding a bit more testing. But also let's talk about the direction a bit - I feel nervous adding in process.exit
/throw
s deep within code. Thanks for sending!
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.
@JoshuaKGoldberg tagging for re-review just in case this disappeared into the backlog abyss 😅
Oh! It did, sorry and thank you! |
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.
This looks great to me, thanks! I have no gripes about the code changes themselves and appreciate the clarity of the new unmatchedSpecFiles
name. 😄
Will wait for another maintainer to review. In the interim, just leaving a few small nitpicks, nothing I'm passionate about.
lib/cli/collect-files.js
Outdated
@@ -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 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?
* @typedef {Object} UnmatchedFile - Diagnostic object containing unmatched files | |
*/ | |
/** | |
* @typedef {Object} UnmatchedFile - Diagnostic object containing unmatched files |
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.
TIL, thanks! Made changes to have separate tags for each object declaration.
lib/cli/run-helpers.js
Outdated
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)); | ||
} |
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.
[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.
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.
Yeah that makes sense to me 👍🏻. I extracted the common code to a function handleUnmatchedFiles
that is now called both by singleRun
and parallelRun
instead of the previous duplication of code.
it('should provide warning for nonexistent cjs file extensions', function (done) { | ||
const nonexistentCjsArg = 'nonexistent.test.js'; |
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.
[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 ...
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'; |
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.
Yep just general testing of common JS files. Changed!
}); | ||
}); | ||
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming]
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) { |
🔎 Overview
Fixes #4047
--file
option by resolving the argument to an absolute path and check for its existence. We now log a warning and exit if the file does not exist.--require
bubble up to the middleware inlib/cli/run.js
. See here. This specific error occurs inlib/cli/run-helpers.js
in thesingleRun
method when we load files asynchronously. Since we were lacking error handling there, the errors appeared the way they previously did before these changes.