changedSince runs specs related to files with no diff

[finn-orsini]

🐛 Bug Report

Using the --changedSince flag, Jest will run specs related to files with no current code changes (no diff) but which were previously committed.

The description of this flag is: Runs tests related to the changes since the provided branch. I would expect changes in this context to mean code changes between the current and specified revision, not each individual commit.

To Reproduce

Steps to reproduce the behavior:

• Check out a new branch from master
• Commit a change to any file (example.js) which has a related spec (example.test.js). Adding a comment will do.
• Remove the change to this file & commit again.
• Check that there are no current changes with git diff origin/master. There should be none.
• Run jest --changedSince origin/master
• Output will show that example.test.js was run, even though example.js has no changes.

Expected behavior

example.test.js does not run

Link to repl or repo (highly encouraged)

https://github.com/finn-orsini/jest-changed-since-bug-poc

• Run yarn
• Check out branch changed_since_no_changes (See PR showing no code changes here)
• Run yarn test

Output will show that sum.test.js was run:

$ jest --changedSince origin/master
 PASS  src/__tests__/sum.test.js
  ✓ adds 1 + 2 to equal 3 (4ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        1.119s

Note that running git log --name-only --pretty=format: HEAD ^origin/master (the command run by jest-changed-files) on this branch will include sum.js since a comment was added and removed in previous commits.

envinfo

  System:
    OS: macOS Mojave 10.14.6
    CPU: (8) x64 Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
  Binaries:
    Node: 8.16.2 - ~/.nvm/versions/node/v8.16.2/bin/node
    Yarn: 1.21.1 - ~/.nvm/versions/node/v8.16.2/bin/yarn
    npm: 6.4.1 - ~/.nvm/versions/node/v8.16.2/bin/npm
  npmPackages:
    jest: ^24.9.0 => 24.9.0

[SimenB]

This is because we use git log rather than git diff, see https://github.com/facebook/jest/blob/56565cc04c7261c5a3222a0547bea58c024ec0b8/packages/jest-changed-files/src/git.ts#L53-L74

I do agree this is not expected, though. @alsuren do you remember why you went with log rather than diff?

/cc @thymikee @jeysal do you agree it's not expected behavior?

[thymikee]

Yea, seems like diff makes more sense here. I wouldn't expect running tests for files that were changed+unchanged in the meantime.

[alsuren]

The reason I use log is quite subtle when you're looking at the code, but it's also crucial to my workflow.

I use jest --watch --changedSince=origin/master as my go-to command for testing my work. If I fetch master, and don't rebase my local branch, I don't want to suddenly start testing all of the files for changes that other people have added to master.

I searched for quite a long time for a "please tell me which files would change if I merged my current branch into master" git invocation, but I couldn't work out how to express it.

Does anyone know of a good way to express this in git?

[alsuren]

hrm...

       git diff [<options>] <commit>...<commit> [--] [<path>...]
           This form is to view the changes on the branch containing and up to the second <commit>,
           starting at a common ancestor of both <commit>. "git diff A...B" is equivalent to "git diff
           $(git merge-base A B) B". You can omit any one of <commit>, which has the same effect as using
           HEAD instead.

       Just in case you are doing something exotic, it should be noted that all of the <commit> in the
       above description, except in the last two forms that use ".." notations, can be any <tree>.

       For a more complete list of ways to spell <commit>, see "SPECIFYING REVISIONS" section in
       gitrevisions(7). However, "diff" is about comparing two endpoints, not ranges, and the range
       notations ("<commit>..<commit>" and "<commit>...<commit>") do not mean a range as defined in the
       "SPECIFYING RANGES" section in gitrevisions(7).

maybe ... would do what we want?

@finn-orsini do you want to try making a fix for this, or should I pick it up? (I probably won't have time until Tuesday though)

[finn-orsini]

@alsuren yeah I can give it a shot this afternoon! Thanks!

[alsuren]

One other thing that we should be aware of when fixing this is that some people abuse the "resolve conflicts" button on github ( https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/resolving-a-merge-conflict-on-github ) rather than rebasing when there are conflicts. (this is a behaviour that I only observed in the last few months)

These people might end up with quite a lot of spurious tests being run under a diff-based changeSince implementation.

master HEAD
     | x <- conflict resolution 2
     |/|
C -> x |
     | x <- conflict resolution 1
     |/|
B -> x |
     | x <- A
     |/
     |
common ancestor

If someone is sitting at HEAD, will changedSince see changes related to A and any conflicts that happened, or will it also see unrelated changes from B and C?

What happens if people are merging master into their branch purely to fix CI issues, and there aren't even any conflicts? I think I might be seeing this behaviour on my current project too, because our CI is set up to test branch-tips without merging with master first :-( .

[edit to avoid spamming everyone with email notifications]:

I guess we could take the output of log and diff, and only test files that show up in both lists?

I would also be okay with letting people using the merge-heavy workflow break, and see if we get bug reports about it. I suspect that nobody will care, and it will make our code faster and cleaner (and more similar to the bzr case, if I remember correctly). This probably isn't my call to make though ;-) .

[/edit]

[finn-orsini]

Great point!

The current project I'm working on is extremely merge-heavy, so, unfortunately, that is something I would like to account for (and I'm fairly sure is why we noticed issues in --changedSince).

I'm going to experiment with the diff only approach as well as comparing the output of log and diff - If neither of these are satisfactory for --changedSince, would adding a different option (maybe --diffAgainst) be a decent compromise?

[edit]
Seeing good results currently using git diff --name-only <commit>...HEAD.

Seems like the workflow you described:

If I fetch master, and don't rebase my local branch, I don't want to suddenly start testing all of the files for changes that other people have added to master.

would be accounted for by using --changedSince=master instead of --changedSince=origin/master with this update.
[/edit]

[monapasan]

@finn-orsini
Is there an update on this one? Do you need any assistance?

[brapifra]

Yeah, can we keep this moving? I'm also happy to help if needed @finn-orsini @alsuren @SimenB

[finn-orsini]

Sorry for the delay here, no excuse other than just totally dropped this from my radar.

I have a fork with what I think was a working fix from when this was originally posted, will double-check it tomorrow morning & post an update here.

Thanks for bearing with me!

[finn-orsini]

🎉 PR opened: #10155

[matthew-hallsworth]

Hi @finn-orsini do you know when this might be merged and released? I have a couple of things relying upon this update that I need to push through our company backlog.

Thanks for any info you can provide :)

[finn-orsini]

👋 Hi @matthew-hallsworth, I believe @SimenB was waiting to see if anyone else wanted to weigh in on the PR. It has been approved, but I do not have access to merge 👍

[SimenB]

#10155

[SimenB]

Fix released in 26.2.0