-
-
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
Git support for --changedFilesWithAncestor #5189
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5189 +/- ##
==========================================
+ Coverage 60.54% 60.59% +0.04%
==========================================
Files 201 201
Lines 6707 6712 +5
Branches 4 4
==========================================
+ Hits 4061 4067 +6
+ Misses 2645 2644 -1
Partials 1 1
Continue to review full report at Codecov.
|
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.
Thanks
We don't seem to document it at all |
Would you mind adding a note in the docs about this option? https://github.com/facebook/jest/blob/master/docs/CLI.md |
'Will run all tests affected by file changes in the last ' + | ||
'commit made.', | ||
'When used together with `--onlyChanged`, it will run all tests ' + | ||
'affected by file changes in the last commit made.', |
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 noticed that both --lastCommit and --changedFilesWithAncestor are ignored if you don't also pass --onlyChanged or --watch. I thought about writing something in the check()
function (line 15 in this file) explode if you pass an argument that will be ignored, but then I realised that someone might set --onlyChanged or --watch in their configs, or someone might add another arg that has the same effect, so I just decided to document it a bit harder.
I also decided against talking about --watch
in the docs for --lastCommit
, because it sounds like a pointless thing to do.
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 noticed that both --lastCommit and --changedFilesWithAncestor are ignored if you don't also pass --onlyChanged or --watch.
That doesn't seem right. @cpojer WDYT?
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.
$ yarn jest --listTests | wc -l
213
$ yarn jest --listTests --lastCommit | wc -l
213
$ yarn jest --listTests --lastCommit --onlyChanged | wc -l
4
@SimenB are you suggesting that:
a) we should make --lastCommit
and --changedFilesWithAncestor
imply --onlyChanged
(may cause previously valid configurations to test fewer things than they used to)
b) we should make --lastCommit
and --changedFilesWithAncestor
explode if it's not combined with --onlyChanged
or --watch
(may cause previously valid configurations to explode, and means that the next person to add an --onlyChanged
synonym needs to update the check)
or are you just after a second reviewer?
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.
or are you just after a second reviewer?
This 🙂
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.
Oh, interesting. I think the reason for that is because the default at FB is that onlyChanged is true and people must pass explicitly --all
to run all tests. I guess we just never noticed the inconsistency. I think --lastCommit --all
doesn't make any sense, so I'm in favor of the proposed change a).
This is awesome, thanks so much for adding this feature! |
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
as requested in #5188, this PR adds git support for the
--changedFilesWithAncestor
flag.Test plan
If this is working, you should be able to do this in the jest repo:
yarn jest -- --onlyChanged --changedFilesWithAncestor