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

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

Merged
merged 15 commits into from
Apr 21, 2022
Merged

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

merged 15 commits into from
Apr 21, 2022

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Feb 7, 2022

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
Copy link
Member

SimenB commented Feb 7, 2022

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

@SimenB
Copy link
Member

SimenB commented Feb 7, 2022

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

@mrazauskas
Copy link
Contributor Author

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
Copy link

codecov-commenter commented Feb 7, 2022

Codecov Report

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

Impacted file tree graph

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

@mrazauskas 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
@@ -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() & ::.)+.)`);
Copy link
Member

@SimenB SimenB Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

SimenB commented Feb 8, 2022

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

@@ -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 .^)');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@SimenB SimenB Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

SimenB commented Feb 8, 2022

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

@mrazauskas
Copy link
Contributor Author

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
Copy link
Member

SimenB commented Feb 8, 2022

Yep!

@mrazauskas 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
Copy link
Contributor Author

mrazauskas commented Feb 8, 2022

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
Copy link
Contributor Author

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 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
Copy link
Contributor Author

@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 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
@mrazauskas
Copy link
Contributor Author

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
Copy link
Member

SimenB commented Apr 21, 2022

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

@SimenB SimenB merged commit 49cc5d2 into jestjs:main Apr 21, 2022
@mrazauskas mrazauskas deleted the ci-disable-mercurial-test branch April 21, 2022 05:42
@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 22, 2022
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.

Tests failing for mercurial changed files
4 participants