-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Fix bug: MPR executes tests multiple times #5335
Conversation
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.
Needs a test, should be straightforward with an integration test.
Also changelog :)
return false; | ||
} | ||
|
||
// It exists and it is a file; return true if it's in the project. | ||
return allFiles.includes(path.resolve(name)); |
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.
@mjesun have you measured perf impact of this? I suspect allFiles
to be quite a big array, no?
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.
I can change it to a Set
so that the search becomes O(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.
I mean, for the typical use-case (jest some-path
or jest test1 test2
) it should be negligible, but hypothetically it might hit the user harder when they provide a glob with tens of paths.
I'm good with a set.
|
Codecov Report
@@ Coverage Diff @@
## master #5335 +/- ##
==========================================
- Coverage 61.21% 61.19% -0.03%
==========================================
Files 205 205
Lines 6900 6903 +3
Branches 4 4
==========================================
Hits 4224 4224
- Misses 2675 2678 +3
Partials 1 1
Continue to review full report at Codecov.
|
const validTestPaths = | ||
paths && | ||
paths.filter(name => { | ||
try { | ||
return fs.lstatSync(name).isFile(); | ||
if (!fs.lstatSync(name).isFile()) { | ||
// It exists, but it is not a file; return false. |
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.
can you remove the "return false" here? It's literally the next line.
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.
huh, I didn't spot that :D
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. |
When executing a test by path (not sure why this is a thing?) we were returning
true
for each call in a project configured. This is because as soon as a test is a file, we returnedtrue
, not checking if the file actually belongs to the project being tested.This has popped up multiple times (e.g. #5332).