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

ci: Run tests with nextest #4740

Merged
merged 4 commits into from
Apr 20, 2023

Conversation

Joining7943
Copy link
Contributor

@Joining7943 Joining7943 commented Apr 13, 2023

This pr installs cargo-nextest as test runner in ci workflows and jobs previously running tests with cargo test.

nextest comes with a better execution model making it less error prone, so that test binaries exiting with error still report the test which caused the error. It also comes with other features like

  • test retries and marking intermittently failing tests as flaky
  • showing per test execution times and particularly slow tests
  • ... see the docs

Note there's no performance improvement in the ci because of the lack of a multi-core test environment. Together with the installation step of nextest the test jobs even take around 5-10 seconds longer.

In case of the android workflow, the nextest binary has to be built from source because none of the pre-built binaries works on x86 android targets. The binary is stored in the emulator image, so that step isn't needed often. The caching strategy for the avd cache changed, so that a successfully built image is cached before running the build and test step or else the upload depends on the build and tests running successfully.

I added a nextest target to the GNUMakefile instead of replacing the existing test target.

Before merging this pr it's needed to clear cached emulator images/snapshots from the github caches or else the android workflow fails due the missing nextest binary in the currently restored images. We could also change the cache key or maybe there's another possibility?

Closes #4737

@Joining7943 Joining7943 force-pushed the ci-use-nextest-as-test-runner branch 21 times, most recently from 6adc112 to a97f203 Compare April 14, 2023 17:33
@Joining7943 Joining7943 marked this pull request as ready for review April 14, 2023 18:10
@sylvestre
Copy link
Contributor

test retries and marking intermittently failing tests as flaky

I love this one!
just for that, moving is clearly super interesting :)

Maybe remove d41591f in a new commit

@sylvestre
Copy link
Contributor

@Joining7943
Copy link
Contributor Author

Maybe remove d41591f in a new commit

The change doesn't affect the gnu tests, only the rust tests. It's a replacement for cargo test.

please update
https://github.com/uutils/coreutils/blob/main/CONTRIBUTING.md#testing

Ok, no problem

@Joining7943 Joining7943 force-pushed the ci-use-nextest-as-test-runner branch from 484496b to c02099f Compare April 15, 2023 08:47
@Joining7943
Copy link
Contributor Author

How to proceed with the avd cache in the github ci? New cache key? I'm asking because this image has a size of more than 2GB. Having two images around is unnecessary and it would take half of the space github makes available per default.

@Joining7943 Joining7943 force-pushed the ci-use-nextest-as-test-runner branch from c02099f to 370d4e9 Compare April 15, 2023 10:13
@Joining7943 Joining7943 marked this pull request as draft April 15, 2023 11:25
@Joining7943 Joining7943 marked this pull request as ready for review April 15, 2023 13:41
@Joining7943
Copy link
Contributor Author

This pr is ready for review. The problem with the emulator image seems to have solved itself and the current image in the cache is the one with the nextest dependency in it. Note the date fuzzer went nuts and produced around 250000 lines of error messages, but this also happened in other prs, so I'm pretty sure it has nothing to do with this one. The android workflow fails because the test step fails with 3 failing tests and 1 flaky test.

@Joining7943 Joining7943 force-pushed the ci-use-nextest-as-test-runner branch from c22c153 to 3f61f33 Compare April 16, 2023 13:17
@Joining7943
Copy link
Contributor Author

Update: I checked the situation around the avd cache in the github cache again, and I think the safest way to make the caching in the android workflow with the nextest binary work is to change the cache key.

@uutils uutils deleted a comment from github-actions bot Apr 17, 2023
@sylvestre
Copy link
Contributor

is it ready for review?

@sylvestre sylvestre force-pushed the ci-use-nextest-as-test-runner branch from 3f61f33 to dc775d9 Compare April 18, 2023 09:06
@uutils uutils deleted a comment from github-actions bot Apr 18, 2023
@uutils uutils deleted a comment from github-actions bot Apr 18, 2023
@Joining7943
Copy link
Contributor Author

is it ready for review?

Sure :)

@uutils uutils deleted a comment from github-actions bot Apr 19, 2023
@uutils uutils deleted a comment from github-actions bot Apr 19, 2023
@@ -0,0 +1,6 @@
[profile.ci]
retries = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, i prefer 2, sorry. if a test fails twice, in a row, it is a big deal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,6 @@
[profile.ci]
retries = 3
status-level = "all"
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please document these options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would placing a link to the official docs above [profile.ci] do the job, too?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@sylvestre
Copy link
Contributor

@tertsdiepraam @cakebaker I think we should take it, WDYT ?

@cakebaker
Copy link
Contributor

@sylvestre +1 to give it a try. In the worst case we can easily return to cargo test.

env:
RUST_BACKTRACE: "1"
CARGO_TERM_COLOR: "always"
Copy link
Member

@tertsdiepraam tertsdiepraam Apr 19, 2023

Choose a reason for hiding this comment

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

@sylvestre you previously objected to this change in #4165. Have you changed your mind?

In any case we can specify it just once, like I did in that PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i missed that change
could you please remove 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.

In any case we can specify it just once, like I did in that PR.

I activated the colors only for tests, for now. Sorry, I missed the discussion in #4165. Having colors in the ci would be really great, though. @sylvestre Are you looking at the raw logs just locally? Removing the color codes would be a oneliner sed -Ei 's/\x1B\[([0-9]{1,2}(;[0-9]{1,2})?)?[mGK]//g' PATTERN.

Copy link
Contributor

Choose a reason for hiding this comment

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

i am looking at logs in github action too

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Apr 19, 2023

Apart from the comment above I agree that this can be merged.

env:
RUST_BACKTRACE: "1"
CARGO_TERM_COLOR: "always"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CARGO_TERM_COLOR: "always"

@Joining7943 Joining7943 force-pushed the ci-use-nextest-as-test-runner branch from dc775d9 to 3495b82 Compare April 19, 2023 21:46
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/misc/timeout is no longer failing!
Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@sylvestre
Copy link
Contributor

seems that more people like logs with color than I was expecting. let's do take it then :)

@sylvestre sylvestre merged commit 7445293 into uutils:main Apr 20, 2023
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.

ci: Use nextest as test runner instead of cargo test
4 participants