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

nix: skip test_shallow_commits_lack_parents under Nix builds #4839

Closed
wants to merge 2 commits into from

Conversation

thoughtpolice
Copy link
Contributor

While this isn't a real solution to #4784 it does stem the bleeding for anyone like Mitchell (or myself) who have to install the flake from the Git repository. This patch also probably needs to be backported to upstream nixpkgs, too.

The flake.lock update is so that we can get a newer version of cargo-nextest that supports the --skip flag (there is a range of nextest versions that don't support --skip after the migration to use 'filtersets' was introduced.)

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@thoughtpolice
Copy link
Contributor Author

thoughtpolice commented Nov 12, 2024

I have spent some time on #4784 but haven't had anything conclusive and I was out of town this weekend. I hope to have more time to look at this this week if Yuya doesn't in the meantime, but otherwise this at least unbreaks people's flakes/overlays.

# https://github.com/martinvonz/jj/issues/4784
# seems to fail reliably under Nix/nix-shell only, but otherwise
# is an unknown anomaly?
"--skip" "test_shallow_commits_lack_parents"
Copy link
Contributor

Choose a reason for hiding this comment

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

The nextest native syntax would involve --filterset, and AIUI you need a -- for --skip – does this actually work to skip it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware of the filterset expression, but I chose this in part because I think --skip reads much more naturally (part of the reason it was presumably added back to nextest, along with the fact it's backwards compatible with older versions; when I originally tried diagnosing this I had to spend 5 minutes figuring out why --skip was reworked and the right way to skip tests, which involved a nice bug report.) Regarding the -- yes, it's because the actual Nix harness around cargo test adds it for you. I've never seen --skip used in any other way in any Rust+Nix project FWIW, so I've assumed this was always the default behavior.

@bnjmnt4n
Copy link
Member

This test does seem somewhat flaky even in non-Nix environments - example of a failing GitHub actions run: https://github.com/martinvonz/jj/actions/runs/11792819629/job/32847132523. Perhaps we should disable it on Rust's end?

@yuja
Copy link
Contributor

yuja commented Nov 12, 2024

This test does seem somewhat flaky even in non-Nix environments - example of a failing GitHub actions run: https://github.com/martinvonz/jj/actions/runs/11792819629/job/32847132523. Perhaps we should disable it on Rust's end?

Maybe we need to reload the repo after make_shallow()? gix holds in-memory cache which is invalidated by file mtime.

@yuja
Copy link
Contributor

yuja commented Nov 12, 2024

#4842

@thoughtpolice
Copy link
Contributor Author

#4842

Yes, I figured it was something like that just like test_gc was flaky, but I've been a bit sick and busy all weekend so I didn't get time to ensure that it was the same problem. I'll test this out.

@thoughtpolice
Copy link
Contributor Author

Yes, this looks like it fixed it for me. Thanks @yuja!

I wonder if we should try running the testsuite on some filesystems like ext3 with far coarser mtimes in order to weed out issues like that. A task for the future...

@thoughtpolice thoughtpolice deleted the aseipp/push-yxuvpwqpnvtw branch November 12, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants