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

Run cargo nextest in addition to cargo test in CI #788

Closed
wants to merge 17 commits into from

Conversation

david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Mar 18, 2022

cargo-nextest is a new Rust test runner. Imagine a world where you can actually tell what tests failed by looking at the CI output. That world is... this world.

As discussed in the comment below, we want to compare nextest and cargo test results for a while without committing to making cargo nextest results authoritative, so this adds a job that is not required to pass for PRs to get merged. Most of the time the two runners should have the same result, but we want to see if that's really true.

Closes #787.

Advantages for CI

  • failure-output = "immediate-final" causes failures to be printed when they happen and again at the end so you can see what failed without scrolling
  • JUnit output, which can be archived (done here) and analyzed over time (not done here) and also picked up by another GH action that displays it nicely on the CI results page (not done here)

In theory it should run the test suite a bit faster too, but I haven't be able to tell whether we're getting a speedup because the times vary so much.

Risks

  • The library is on the new side and we don't have a lot of experience with it
  • Most people are not using it in local dev, so it's possible we could see surprising behavior that fails to repro with cargo test

My sense is these are small risks and the project is pretty reliable and stable, but I would want input from people who have a better feel for the ecosystem and what people are using. I believe some of us are using it locally already.

What this PR does

Add a job build-and-nextest that does the following:

  • Use a prebuilt action to install cargo-nextest
  • Use cargo-nextest as the test runner for the CI test job
  • Have cargo-nextest produce JUnit output
    • This requires a config file because this option is not in the CLI
  • Archive said JUnit output whether tests pass or fail

Example failure output

Note that the failure is at the bottom of the output.

image

@david-crespo
Copy link
Contributor Author

david-crespo commented Mar 18, 2022

Ok cargo install nextest took 3-5 minutes, so building from source is clearly not tenable. I'll look at pulling the binary.

@david-crespo
Copy link
Contributor Author

Output is an improvement — the failure is only 60 lines up from the bottom. I have a feeling there's a flag we can pass to make it not print these PASSes at the bottom.

image

@david-crespo
Copy link
Contributor Author

Tried with --no-capture and it was way worse, so I took that commit back off and force pushed.

--no-capture output image

@sunshowers
Copy link
Contributor

sunshowers commented Mar 25, 2022

Output is an improvement — the failure is only 60 lines up from the bottom.

Hi there, author of nextest here! I think your problem can be solved with nextest by passing in --failure-output immediate-final: see https://nexte.st/book/other-options.html#reporter-options. You can also define a CI profile in config with this setting: https://nexte.st/book/configuration.html#profiles

Using pre-built binaries in GHA is also super easy these days if it helps: https://nexte.st/book/pre-built-binaries.html#using-pre-built-binaries-in-ci

Happy to answer any other questions you might have -- glad you're trying it out :)

@david-crespo
Copy link
Contributor Author

@sunshowers thank you! A bunch of us have already been using nextest locally and love it.

@david-crespo
Copy link
Contributor Author

Using the GH action to get the nextest binary takes 0s. So that's good.

@david-crespo
Copy link
Contributor Author

Output looks good! I'm going to try it with some other failures sprinkled around.

image

@david-crespo david-crespo marked this pull request as ready for review March 25, 2022 23:57
@david-crespo david-crespo changed the title Try cargo-nextest in CI Replace cargo test with cargo-nextest in CI Mar 25, 2022
failure-output = "immediate-final"

[profile.ci.junit]
path = "junit.xml"

This comment was marked as resolved.

This comment was marked as resolved.

@@ -100,7 +100,7 @@ async fn test_sessions(cptestctx: &ControlPlaneTestContext) {
.await
.expect("failed to get 401 for unauthed API request");

RequestBuilder::new(&testctx, Method::GET, "/orgs/whatever")
RequestBuilder::new(&testctx, Method::GET, "/orgs/nonexistent")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

forcing tests to run. the run deduper thing is driving me nuts

@david-crespo
Copy link
Contributor Author

Some issues came up in chat. @davepacheco points out that fully changing over to nextest in CI effectively means we are deciding nextest is authoritative, i.e., if cargo test and cargo nextest give different results in a give case, we listen to nextest. Nearly everyone is likely using cargo test locally.

For this reason, I'm updating the PR to add separate jobs (well, matrix options on the same job) that run the tests with nextest. This lets us see how often the two test runners give different results, as well as compare output in the case of failures. We can use the generated name of the jobs to keep only cargo test required for now.

The required checks would have to be changed in the repo settings to include

build-and-test (macos-11, cargo-test)
build-and-test (ubuntu-18.04, cargo-test)

image

@david-crespo david-crespo changed the title Replace cargo test with cargo-nextest in CI Run cargo nextest in addition to cargo test in CI Apr 18, 2022
# end without also grabbing random other temporary files.
run: |
TMPDIR=$OMICRON_TMP PATH="$PATH:$PWD/out/cockroachdb/bin:$PWD/out/clickhouse" RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings" \
cargo build --locked --all-targets --verbose
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 think we could skip the cargo build step here, but that might make it harder to tell at a glance when the results of these two jobs diverge

@jclulow
Copy link
Collaborator

jclulow commented Apr 19, 2022

Can we add an analogous clone of the buildomat job to run nextest there also?

@david-crespo
Copy link
Contributor Author

Amazingly, it worked on the first try.

@david-crespo
Copy link
Contributor Author

The files in question have changed enough that we'll want to start this from scratch. I still think it would be nice to get dramatically better failure output from CI.

@david-crespo david-crespo deleted the cargo-nextest branch January 12, 2023 17:41
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.

Try cargo-nextest in CI for better failure reporting
3 participants