Skip to content
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

Merged
merged 8 commits into from
Jan 4, 2018

Conversation

alsuren
Copy link
Contributor

@alsuren alsuren commented Dec 27, 2017

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

@codecov-io
Copy link

codecov-io commented Dec 27, 2017

Codecov Report

Merging #5189 into master will increase coverage by 0.04%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/jest-changed-files/src/git.js 93.93% <95.65%> (+4.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97ee7c9...4b15f86. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@SimenB
Copy link
Member

SimenB commented Dec 27, 2017

Missing docs update, but we still don't have versioned docs, so not sure how to handle it

We don't seem to document it at all

@SimenB
Copy link
Member

SimenB commented Dec 28, 2017

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.',
Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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 🙂

Copy link
Member

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).

@cpojer cpojer merged commit 1a039f9 into jestjs:master Jan 4, 2018
@cpojer
Copy link
Member

cpojer commented Jan 4, 2018

This is awesome, thanks so much for adding this feature!

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants