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

Improve hg revset used by jest-changed-files #7880

Merged
merged 2 commits into from
Feb 13, 2019
Merged

Conversation

quark-zju
Copy link
Contributor

Summary

Changed the default hg status --rev parameter used by jest-changed-files so it's more correct in corner cases, and avoids the (incorrect) usage of branch() which will be removed internally at Facebook.

Test plan

I'm not sure how to test this exactly. But there are no new test breakage introduced by this change. Commit 64485f7 introduced the code but it didn't come with a test. Aside from that, I've made sure the revset works as advertised.


The old revset is ancestor(.^, min(branch(.) and not min(branch(default)), max(public())).
It can select public commits unintentionally in some cases.

In the following two examples, commit A would be selected by the old code path
as the "from revision". The new revset would select commit C instead, which
is better because the developer won't want to test commits in the B::C range.

Example 1 - Multiple named branches:

  E default (named branch), public
  :
  | D stable (named branch), draft (only commit the developer cases about)
  | |
  | C stable (named branch), public
  | :
  | |
  | B stable (named branch), public
  |/
  A

Example 2 - Multiple heads in the "default" named branch:

  E default (named branch), public, has a bigger revision number than C.
  :
  | D default (named branch), draft (only commit the developer cases about)
  | |
  | C default (named branch), public
  | :
  | |
  | B default (named branch), public
  |/
  A

Explanation of the min(!public() & ::.)^ revset:

With modern Mercurial, !public() is the way to select local commits that
do not exist on other developer's repos. & ::. limits commits to only the
current "feature branch" (branch in terms of the commit DAG, not hg named
branches). min selects the first commit in the non-public feature branch,
and ^ selects its immediate parent. Note: it's !public() & ::. instead
of ::. & !public() intentionally, because the former has a fast path.

The old revset is `ancestor(.^, min(branch(.) and not min(branch(default)), max(public()))`.
It can select public commits unintentionally in some cases.

In the following two examples, commit A would be selected by the old code path
as the "from revision". The new revset would select commit C instead, which
is better because the developer won't want to test commits in the `B::C` range.

Example 1 - Multiple named branches:

    E default (named branch), public
    :
    | D stable (named branch), draft (only commit the developer cases about)
    | |
    | C stable (named branch), public
    | :
    | |
    | B stable (named branch), public
    |/
    A

Example 2 - Multiple heads in the "default" named branch:

    E default (named branch), public, has a bigger revision number than C.
    :
    | D default (named branch), draft (only commit the developer cases about)
    | |
    | C default (named branch), public
    | :
    | |
    | B default (named branch), public
    |/
    A


Explanation of the `min(!public() & ::.)^` revset:

With modern Mercurial, `!public()` is the way to select local commits that
do not exist on other developer's repos. `& ::.` limits commits to only the
current "feature branch" (branch in terms of the commit DAG, not hg named
branches).  `min` selects the first commit in the non-public feature branch,
and `^` selects its immediate parent. Note: it's `!public() & ::.` instead
of `::. & !public()` intentionally, because the former has a fast path [1].

[1]: See https://www.mercurial-scm.org/repo/hg/rev/c6c8a52e28c9077580dd2f0552eb2bd6d5e0d13c
@codecov-io
Copy link

Codecov Report

Merging #7880 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7880   +/-   ##
=======================================
  Coverage   58.46%   58.46%           
=======================================
  Files         178      178           
  Lines        6638     6638           
  Branches        5        6    +1     
=======================================
  Hits         3881     3881           
  Misses       2755     2755           
  Partials        2        2

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 9ba62ad...bff81f5. Read the comment docs.

@quark-zju
Copy link
Contributor Author

The windows test failure seems unrelated to this change.

@SimenB
Copy link
Member

SimenB commented Feb 13, 2019

I've never used mercurial, so not really able to contribute any meaningful review. The PR (#5476) seems to have changed it as an aside. Maybe @mjesun can comment on this?

We have an e2e test for the basic functionality already: https://github.com/facebook/jest/blob/9ba62ada66f731880229073eb193e8cb60863ae3/e2e/__tests__/jestChangedFiles.test.js#L320-L322, so it seems not to be broken with your changes at least


Windows failure not related (that test is a bit flaky 😞)

@mjesun
Copy link
Contributor

mjesun commented Feb 13, 2019

I wasn't aware of the !public() when I did this, and it looks like a really nice improvement! I've done a bit of testing and it seems to effectively be selecting the right commits, and I'm happy to see a fast path was also used to optimize it (in fact, I tested it and works super fast).

@SimenB SimenB merged commit f27d1c1 into jestjs:master Feb 13, 2019
@mjesun
Copy link
Contributor

mjesun commented Feb 28, 2019

@quark-zju I just found an issue when executing jest in a public commit, as it reports empty revision range. Any guesses on how to make it work for these use cases too?

@quark-zju
Copy link
Contributor Author

@mjesun Sorry, I didn't think about that. I think min((!public() & ::.)+.)^ can be used.

@SimenB
Copy link
Member

SimenB commented Mar 5, 2019

@quark-zju I'm getting test failures after this PR.

    Command failed: hg status -amnu --rev min(!public() & ::.)^ /var/folders/gj/0mygpdfn6598xh34njlyrqzc0000gn/T/jest-changed-files-test-dir /var/folders/gj/0mygpdfn6598xh34njlyrqzc0000gn/T/jest-changed-files-test-dir/nested-dir /var/folders/gj/0mygpdfn6598xh34njlyrqzc0000gn/T/jest-changed-files-test-dir/nested-dir/second-nested-dir
    abort: empty revision range

      at makeError (node_modules/execa/index.js:174:9)

Which version of mercurial should I use?

$ hg --version
Mercurial Distributed SCM (version 4.9)
(see https://mercurial-scm.org for more information)

Copyright (C) 2005-2019 Matt Mackall and others
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@jeysal
Copy link
Contributor

jeysal commented Mar 5, 2019

Same error as @SimenB on Linux, same Mercurial version. But I'm also getting a second one.

 FAIL  e2e/__tests__/jestChangedFiles.test.ts (5.995s)                                                                                                                                                                                          
  ● gets changed files for hg                                                                                                                                                                                                                   
                                                                                                                                                                                                                                                
    Command failed: hg status -amnu --rev min(!public() & ::.)^ /tmp/jest-changed-files-test-dir /tmp/jest-changed-files-test-dir/nested-dir /tmp/jest-changed-files-test-dir/nested-dir/second-nested-dir                                      
    abort: empty revision range                                                                                                                                                                                                                 
                                                                                                                                                                                                                                                
      at makeError (node_modules/execa/index.js:174:9)                                                                                                                                                                                          
                                                                                                                                                                                                                                                
 FAIL  e2e/__tests__/onlyChanged.test.ts (20.086s)                                                                                                                                                                                              
  ● gets changed files for hg                                                                                                                                                                                                                   
                                                                                                                                                                                                                                                
    expect(received).toMatch(expected)                                                                                                                                                                                                          
                                                                                                                                                                                                                                                
    Expected pattern: /PASS __tests__(\/|\\)file2.test.js/                                                                                                                                                                                      
    Received string:  "                                                                                                                                                                                                                         
                                                                                                                                                                                                                                                
      ● Test suite failed to run                                                                                                                                                                                                                
                                                                                                                                                                                                                                                
        abort: empty revision range                                                                                                                                                                                                             
                                                                                                                                                                                                                                                
    "                                                                                                                                                                                                                                           
                                                                                                                                                                                                                                                
      273 |                                                                                                                                                                                                                                     
      274 |   ({stdout, stderr} = runJest(DIR, ['-o', '--changedFilesWithAncestor']));                                                                                                                                                          
    > 275 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);                                                                                                                                                                     
          |                  ^                                                                                                                                                                                                                  
      276 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);                                                                                                                                                                     
      277 | });                                                                                                                                                                                                                                 
      278 |                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                                
      at Object.toMatch (e2e/__tests__/onlyChanged.test.ts:275:18)                                                                                                                                                                              
      at asyncGeneratorStep (e2e/__tests__/onlyChanged.test.ts:13:103)                                                                                                                                                                          
      at _next (e2e/__tests__/onlyChanged.test.ts:15:194)                                                                                                                                                                                       
      at e2e/__tests__/onlyChanged.test.ts:15:364                                                                                                                                                                                               
      at Object.<anonymous> (e2e/__tests__/onlyChanged.test.ts:15:97)

@mjesun
Copy link
Contributor

mjesun commented Mar 6, 2019

@SimenB, didn't you forget the +..

Edit: I think you commented on the PR not noticing @quark-zju's reply with changing the HG pattern. I'm making a PR with the updated pattern as we speak, since I verified it works.

@SimenB
Copy link
Member

SimenB commented Mar 6, 2019

I just ran the integration test, not passing anything anywhere :D

@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 11, 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.

6 participants