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

[mrazauskas]

Summary

Closes #8653

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]

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

[SimenB]

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

[mrazauskas]

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.

[codecov-commenter]

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]

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 🙈

[mrazauskas]

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]

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

[mrazauskas]

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

[SimenB]

@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 😅

[mrazauskas]

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]

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

[mrazauskas]

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]

Yep!

[mrazauskas]

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]

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]

@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]

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]

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

[github-actions]

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.