Verify passed test path is a file before resolving #5317
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
| const validTestPaths = paths && paths.filter(fs.existsSync); | ||
| const validTestPaths = | ||
| paths && | ||
| paths.filter(fs.existsSync).filter(name => fs.lstatSync(name).isFile()); |
There was a problem hiding this comment.
What about
const validTestPaths =
paths &&
paths.filter(name => {
try {
return fs.lstatSync(name).isFile();
} catch (e) {
return false;
}
});to at least save a single FS operation?
Codecov Report
@@ Coverage Diff @@
## master #5317 +/- ##
==========================================
- Coverage 61.27% 61.25% -0.03%
==========================================
Files 205 205
Lines 6893 6896 +3
Branches 4 4
==========================================
Hits 4224 4224
- Misses 2668 2671 +3
Partials 1 1
Continue to review full report at Codecov.
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
CHANGELOG.md
Outdated
| @@ -1,5 +1,12 @@ | |||
| ## master | |||
|
|
|||
| ## jest 22.1.1 | |||
There was a problem hiding this comment.
Just remove this line, we'll add version number on publish
|
Ahm, is this already in Looking at the file itself, it doesn't seem to have the changes: But |
|
This changeset isn't included in the v22.1.1 release. It will probably be included in another patch release soon. |
|
Oh I found my confusion, on the Release page it says "x commits to master since this tag" and I clicked it without reading, thinking those are the commits of this tag. Looking forward for the next patch. |
|
@LINKIWI @alexilyaev patch released |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This PR fixes #5272 by fixing the regression introduced in #3882.
#3882 allows directly specifying file names as arguments to the
jestCLI but this unintentionally breaks existing behavior when a test directory is passed as the sole argument tojest.Fix details
Symptom: When
*.test.jsfiles are stored in a directory e.g.test, invokingjest teston v22.0.5+ results in the following:Root cause: The change allowing directly running tests by file name does not ensure that the passed argument is not a directory (or, more generally, that the passed argument is a file).
Remediation: Filter out all valid test paths that aren't files before continuing existing logic.
Test plan
The test added by #3882 was updated to also pass a directory as an argument to the CLI. We observe that no errors are thrown; without this change, the test would fatally throw with
EISDIRas shown in the example symptom above.Discussion
Unclear if the performance impact by invoking
fs.lstatSyncon each valid test path is significant enough to explore alternatives.After poking around with this for a bit, I actually believe this regression creates a desired interface change. If the client wants to recursively search a directory for tests to run, he/she should explicitly pass option
--testPathPatternas stated here as a workaround for the bug. Based on the changeset in Allow unmatched test paths from CLI #3882 it seems like the fact that invoking the CLI asjest dir-with-testsworks seems to be a coincidental side effect.But anyway, that is a breaking interface change, so this PR just tries to fix the regression as-is.