-
-
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!
lib/cli/collect-files.js
Outdated
`${symbols.warning} Warning: Cannot find test file "${file}" at the path "${fileAbsolutePath}"` | ||
); | ||
console.warn(`${warningMessage}\n`); | ||
process.exit(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] I don't love having a process.exit
deep within code. n/no-process-exit
has good reasoning around it.
It's also not really a "warning" yellow if the whole thing exits. At that point it's an error (red).
I think a more future-facing, approach would be either:
- (less work, but still partially flawed) have this function throw a special error that any higher-up handler(s) know how to log nicely
- (more work, but more understandable long-term) have this function return an object explaining its results, and have higher-up handler(s) work nicely with it
I haven't looked deeply enough to know whether either or both is feasible. Before we go into a whole deep dive, what do you think about this direction?
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.
Good to learn about the no-process-exit
, thanks! I think the latter where we return an object and having the higher-up handler(s) deal with it sounds like the approach here. The error wasn't handled in run-helpers
so maybe we can add something so that bubbles up to run.js
like the other methods?
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 approach sounds like a good one to try!
@@ -30,7 +32,7 @@ module.exports = ({ | |||
sort, | |||
spec | |||
} = {}) => { | |||
const unmatched = []; | |||
const unmatchedSpecFiles = []; |
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
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.
@@ -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 |
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.
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'; |
}); | ||
}); | ||
|
||
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.