jest --changedFilesToContributeTo=origin/master#5188
jest --changedFilesToContributeTo=origin/master#5188alsuren wants to merge 3 commits intojestjs:masterfrom
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! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| child.stdout.on('data', data => (stdout += data)); | ||
| child.stderr.on('data', data => (stderr += data)); | ||
| child.on('error', e => reject(e)); | ||
| child.on('close', code => { |
There was a problem hiding this comment.
wow, we really should use execa or something...
There was a problem hiding this comment.
Is this a refactor that you want me to include in my bug fix branch, or is it okay to just move the existing code about unchanged for now, and and backlog the execa change for later?
(git grep shows it up as a dependency of os-locale and lerna in yarn.lock, but isn't used directly by jest anywhere)
There was a problem hiding this comment.
Sticking it in the backlog is no problem at all, and will keep this PR focused. So just keep the child_process stuff for now 🙂
| export type Options = {| | ||
| lastCommit?: boolean, | ||
| withAncestor?: boolean, | ||
| sinceCommit?: ?string, |
There was a problem hiding this comment.
? on both sides, does that mean anything? Ref #5176 (comment)
| description: | ||
| 'When used together with `--onlyChanged`, it runs tests ' + | ||
| 'related to the current changes and the changes made in the last commit. ' + | ||
| '(NOTE: this only works for hg repos)', |
There was a problem hiding this comment.
Can this fix (thanks :D) be in a separate PR?
There was a problem hiding this comment.
Sure. I'll have a go at extracting it tomorrow (it's getting late here though).
There was a problem hiding this comment.
... okay so it turned out to be easier than I thought (#5189) but I am genuinely off to bed now.
Codecov Report
@@ Coverage Diff @@
## master #5188 +/- ##
==========================================
- Coverage 61.23% 61.23% -0.01%
==========================================
Files 205 205
Lines 6891 6896 +5
Branches 4 3 -1
==========================================
+ Hits 4220 4223 +3
- Misses 2670 2672 +2
Partials 1 1
Continue to review full report at Codecov.
|
|
I think this is a great idea! 🎉 The todos in the OP look fine to me. Regarding mercurial support, I don't think it should be a blocker, but if you're able to add support for it, that would be great. |
|
Can you rebase this and resolve the conflicts? Bonus points if you can also make this work for Mercurial before we merge this so that we keep consistency for people :) |
alsuren
force-pushed
the
changed-files-refactor
branch
from
0b61468 to
b5d751c
Compare
| } | ||
| let args = ['status', '-amnu']; | ||
| if (options && options.withAncestor) { | ||
| args.push('--rev', 'ancestor(.^)'); |
There was a problem hiding this comment.
I've asked a friend who actually understands hg to help me with making hg work. I pity the fool that tries to use this option in a mixed hg and git environment when it's finished though.
|
I realised that actually I always want all changes that are included in HEAD and the working tree but not in the tree from $sinceCommit. This should stop users from relying on awkward constructs like I will have a go at fixing the tests tomorrow. |
alsuren
force-pushed
the
changed-files-refactor
branch
from
57a7181 to
933e33a
Compare
alsuren
changed the title
alsuren
force-pushed
the
changed-files-refactor
branch
from
c38eab0 to
b637af5
Compare
|
I fixed all the tests and conflicts, and cherry-picked #5307 (to preemptively resolve conflicts, because I wanted its functionality). I also settled on I think I'm pretty happy with the code at this point, so should be ready to review now, unless you'd prefer to review #5307 first (test failures look like permissions problems on the server, I think). |
make changedFilesToContributeTo require an arg, and imply onlyChanged
alsuren
force-pushed
the
changed-files-refactor
branch
from
b637af5 to
0ba2431
Compare
|
|
|
Closing this one in favor of the duplicate. |
|
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. |
5 participants
Summary
I often have
jest --watchrunning in a window, but sometimes I forget to look at it before committing, or I stage a bunch of changes that break tests, and it hides them from the test runner. This means that I often get surprised by CI test failures when I come to open pull requests.This change allows you to write
jest --watch --changedFilesSinceCommit=$LAST_MERGE_COMMITto test your entire feature branch, but not the whole project.LAST_MERGE_COMMITcan be written asHEAD^{/^Merge}or`git log --date-order --merges -1 --format=%H`on projects that use merge commits. On projects like jest that use linearised history, you might be able to get away with something like`git log --oneline --cherry --format=%H origin/master...HEAD | tail -n1`^but I don't work on them often enough to know.Test plan
I still need to write some integration tests for this (see TODO below) but I wanted to float the idea and get feedback before I got too invested in it.
To test this against the jest repo on my local machine, I have been running
yarn run watchin one window, andyarn jest -- --onlyChanged --changedFilesSinceCommit=HEAD^^^^^^^^^(if I add more^s then I get more test suites run in myTest Suites: n passed, n totaloutput.TODO (feel free to add more):