fix(jest-changed-files): improve `changedFilesWithAncestor` command pattern for Mercurial SCM #12322

mrazauskas · 2022-02-07T18:56:08Z

Summary

A changed files test for Mercurial SCM is failing locally and on CI with "abort: empty revision range" error. It may be a good idea to improve the command pattern implementing the suggestion by @quark-zju from #8653 (comment)

Also it seems like that test can run on CI. No need to keep it disabled. Nice!

One of mercurial related tests is from time to time failing in CI. I got curious to look at it. Interestingly all other hg tests in the same test file are disabled on CI with rather convincing comment. Would it make sense to skip the last one too?

~~(Hm.. The comment if rather outdated. It also might sense to enable all tests on GitHub and to see what happens.)~~

~~It isn’t best idea to disable tests, of course. There must be a good reason. More green CI runs, perhaps? Is this one good enough? Not sure..~~

~~Just thinking out loud.~~

Test plan

Green CI

SimenB · 2022-02-07T19:25:59Z

If it's flaky instead of failing we should add jest.retryTimes or something

SimenB · 2022-02-07T19:32:58Z

might need to skip if jasmine or something, it doesn't support retries

mrazauskas · 2022-02-07T19:35:52Z

If it's flaky instead of failing we should add jest.retryTimes or something

Worth to try. Let’s see if it helps.

By the way, I was comparing versions of OS and mercurial which were installed on passing / failing CI runs. No difference, nothing suspicious.

e2e/__tests__/jestChangedFiles.test.ts

codecov-commenter · 2022-02-07T20:38:01Z

Codecov Report

Merging #12322 (3f43c84) into main (2f5a93d) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #12322      +/-   ##
==========================================
- Coverage   67.26%   67.26%   -0.01%     
==========================================
  Files         330      330              
  Lines       17352    17352              
  Branches     5071     5071              
==========================================
- Hits        11672    11671       -1     
- Misses       5648     5649       +1     
  Partials       32       32

Impacted Files	Coverage Δ
packages/jest-changed-files/src/hg.ts	`0.00% <0.00%> (ø)`
packages/expect/src/utils.ts	`96.09% <0.00%> (-0.49%)`	⬇️

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 2f5a93d...3f43c84. Read the comment docs.

SimenB · 2022-02-08T07:01:28Z

packages/jest-changed-files/src/hg.ts

@@ -20,7 +20,7 @@ const adapter: SCMAdapter = {

    const args = ['status', '-amnu'];
    if (options && options.withAncestor) {
-      args.push('--rev', `min((!public() & ::.)+.)^`);
+      args.push('--rev', `min((!public() & ::.)+.)`);


interesting! I've never used mercurial before, but since the test assert on this I guess the change is correct? 🙂

Was set to use ^ in #7880. I seem to have complained there about failing test, tho 🙈

My first time with Mercurial, to be honest. That caret symbol might be needed, but seems like this is the trouble maker. Possibly Execa is somehow escaping it.

SimenB · 2022-02-08T07:05:03Z

I'll land in it v28 just to be safe (although I trust the tests). Will start landing for v28 probably tomorrow

mrazauskas · 2022-02-08T07:09:16Z

packages/jest-changed-files/src/hg.ts

@@ -20,7 +20,7 @@ const adapter: SCMAdapter = {

    const args = ['status', '-amnu'];
    if (options && options.withAncestor) {
-      args.push('--rev', `min((!public() & ::.)+.)^`);
+      args.push('--rev', 'min((not public() and ancestors(.))^ or .^)');


The ^ is left in place, but not at the end. Also I replaced !, &, + with not, and, or. Might be these are less problematic.

@mjesun @quark-zju dunno if any of you are still at FB (I know Miguel is not, at least 🙂), but does this change make sense to you? Could you verify it?

Less weird syntax (and of course fixes a test, at least with mercurial 6), but I've never written a mercurial command in my life, so I have no frame of reference to know if this is the correct fix or not 😅

It might be this is not the same thing with ^ moved inside min(). But leaving ^ at the end of command throws abort: empty revision range locally and in CI. Strange.

SimenB · 2022-02-08T07:31:22Z

@mrazauskas should we enable the tests (with retry?) that passes without your change separately from changing the behaviour (even if correcting it)?

mrazauskas · 2022-02-08T07:35:07Z

You mean to split this PR? New one just to enable working tests (with retry, possibly) and to leave this PR just for that correction. Sounds good for me.

SimenB · 2022-02-08T07:36:04Z

Yep!

mrazauskas · 2022-02-08T10:45:44Z

Seems like I fixed the command, but now test is failing.

Current command min((!public() & ::.)+.)^ wasn’t working neither locally, nor on CI. It was throwing abort: empty revision range. Seemed like ending caret (^) might be the issue.

Mercurial docs: "x^ – Equivalent to x^1, the first parent of each changeset in x." I added ^1 at the end, so that command looks like this min((!public() & ::.)+.)^1. Should be the same command, but as one can see now test is failing in CI. (The passing runs do skip Mercurial tests.)

Alternatively min((not public() and ancestors(.))^ or .^) command was tried out before (edit: just noticed there should be follow instead of ancestors, like this: min((not public() and follow(.))^ or .^)). It was passing all tests. Hm.. This might be the right one.

Just to say, I have never used Mercurial. Came to all this by reading docs (this is most useful part).

mrazauskas · 2022-02-08T12:17:04Z

Seems like the solution to this problem was already suggested by @quark-zju here: #8653 (comment)

Works locally. Let’s see what CI thinks.

mrazauskas · 2022-02-08T15:21:10Z

@SimenB This PR is rebased and cleaned up. I implemented the change suggested by experienced developer. Should be safe to land it in v28.

mrazauskas · 2022-04-20T14:39:35Z

I'll land in it v28 just to be safe (although I trust the tests). Will start landing for v28 probably tomorrow

@SimenB Just to remind. I don’t need it, just wanted to fix the test. All could also stay as is, since nobody is complaining (;

SimenB · 2022-04-21T05:31:50Z

hah, forgot. should've added it to the milestone 😅

github-actions · 2022-05-22T00:08:23Z

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.

disable flaky mercurial test

d4a5679

facebook-github-bot added the cla signed label Feb 7, 2022

add jest.retryTimes

7af601b

skip one test

7f06a80

mrazauskas commented Feb 7, 2022

View reviewed changes

e2e/__tests__/jestChangedFiles.test.ts Outdated Show resolved Hide resolved

exclude failing part

879a8a5

mrazauskas changed the title ~~ci: skip flaky mercurial test on CI~~ ci(jest-changed-files): enable all mercurial related tests on CI Feb 8, 2022

SimenB reviewed Feb 8, 2022

View reviewed changes

mrazauskas commented Feb 8, 2022

View reviewed changes

mrazauskas changed the title ~~ci(jest-changed-files): enable all mercurial related tests on CI~~ fix(jest-changed-files): an attempt to fix failing mercurial related test Feb 8, 2022

mrazauskas mentioned this pull request Feb 8, 2022

ci(jest-changed-files): enable mercurial related test on CI #12327

Merged

mrazauskas added 3 commits February 8, 2022 12:01

Merge branch 'main' into ci-disable-mercurial-test

979530b

fix revset

e714e6e

uncomment test

72ba692

with first

2140f7d

mrazauskas added 2 commits February 8, 2022 14:41

increase timeout?

2b175e4

revert timeout

b4ccbe2

mrazauskas changed the title ~~fix(jest-changed-files): an attempt to fix failing mercurial related test~~ fix(jest-changed-files): improve changedFilesWithAncestor pattern for Mercurial SCM Feb 8, 2022

mrazauskas added 2 commits February 8, 2022 17:12

Merge branch 'main' into ci-disable-mercurial-test

cce3072

pattern style

79ad644

add changelog entry

c1fbe7b

prettier

3f43c84

mrazauskas changed the title ~~fix(jest-changed-files): improve changedFilesWithAncestor pattern for Mercurial SCM~~ fix(jest-changed-files): improve changedFilesWithAncestor command pattern for Mercurial SCM Feb 8, 2022

Merge branch 'main' into ci-disable-mercurial-test

01c8206

SimenB merged commit 49cc5d2 into jestjs:main Apr 21, 2022

mrazauskas deleted the ci-disable-mercurial-test branch April 21, 2022 05:42

mrazauskas mentioned this pull request Apr 21, 2022

chore(CI): enable one more Mercurial test #12704

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(jest-changed-files): improve `changedFilesWithAncestor` command pattern for Mercurial SCM #12322

fix(jest-changed-files): improve `changedFilesWithAncestor` command pattern for Mercurial SCM #12322

mrazauskas commented Feb 7, 2022 •

edited

Loading

SimenB commented Feb 7, 2022

SimenB commented Feb 7, 2022

mrazauskas commented Feb 7, 2022

codecov-commenter commented Feb 7, 2022 •

edited

Loading

SimenB Feb 8, 2022 •

edited

Loading

mrazauskas Feb 8, 2022

SimenB commented Feb 8, 2022

mrazauskas Feb 8, 2022

SimenB Feb 8, 2022 •

edited

Loading

mrazauskas Feb 8, 2022

SimenB commented Feb 8, 2022

mrazauskas commented Feb 8, 2022

SimenB commented Feb 8, 2022

mrazauskas commented Feb 8, 2022 •

edited

Loading

mrazauskas commented Feb 8, 2022

mrazauskas commented Feb 8, 2022

mrazauskas commented Apr 20, 2022

SimenB commented Apr 21, 2022

github-actions bot commented May 22, 2022

fix(jest-changed-files): improve changedFilesWithAncestor command pattern for Mercurial SCM #12322

fix(jest-changed-files): improve changedFilesWithAncestor command pattern for Mercurial SCM #12322

Conversation

mrazauskas commented Feb 7, 2022 • edited Loading

Summary

Test plan

SimenB commented Feb 7, 2022

SimenB commented Feb 7, 2022

mrazauskas commented Feb 7, 2022

codecov-commenter commented Feb 7, 2022 • edited Loading

Codecov Report

SimenB Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

mrazauskas Feb 8, 2022

Choose a reason for hiding this comment

SimenB commented Feb 8, 2022

mrazauskas Feb 8, 2022

Choose a reason for hiding this comment

SimenB Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

mrazauskas Feb 8, 2022

Choose a reason for hiding this comment

SimenB commented Feb 8, 2022

mrazauskas commented Feb 8, 2022

SimenB commented Feb 8, 2022

mrazauskas commented Feb 8, 2022 • edited Loading

mrazauskas commented Feb 8, 2022

mrazauskas commented Feb 8, 2022

mrazauskas commented Apr 20, 2022

SimenB commented Apr 21, 2022

github-actions bot commented May 22, 2022

fix(jest-changed-files): improve `changedFilesWithAncestor` command pattern for Mercurial SCM #12322

fix(jest-changed-files): improve `changedFilesWithAncestor` command pattern for Mercurial SCM #12322

mrazauskas commented Feb 7, 2022 •

edited

Loading

codecov-commenter commented Feb 7, 2022 •

edited

Loading

SimenB Feb 8, 2022 •

edited

Loading

SimenB Feb 8, 2022 •

edited

Loading

mrazauskas commented Feb 8, 2022 •

edited

Loading