ci: improve test resilience and run style checks within `cargo nextest` #9608

nagisa · 2023-09-28T07:48:43Z

I got absolutely fed up with waiting for prerequisite infrastructure work for #9290 (why does it have to be that hard?)

However the PR in question had some other important improvements that do not necessarily rely on changes to said infrastructure to work. In particular:

nextest now retries failing tests a few times before giving up, to make sure they aren't spuriously failing;
- This should help some timing out integration tests in particular, as those often fail because of some deadlockish situation in my experience.
style checks still run with cargo nextest – unfortunately that means that in CI this will run these checks multiple times, but that doesn’t sound particularly terrible of a tradeoff (especially if the other changes mean we won't be retrying entire test suites as often anymore;)
This should allow for a greater portion of the test suite to run on Macs – unfortunately not verified by the CI, but people do complain and this should make the situation better.

cc #9367

This way running `cargo nextest` locally is the only thing you need to do to figure out if there are going to be any issues with landing your code. One other reason is that running the tools locally the naive way will clobber the build cache which can make iteration times slower.

When run on an m1 these can result in a very significant logging!

Checking clippy can take quite some time, and locally it is better to spend some disk space for quick reruns over having to wait 2 minutes or so before the test concludes.

It seems like we end up blocking on some directories sometimes, and

In particular a CI-specific profile that retries individual failing tests a couple times before calling the entire CI run failing is a big one as that will save on time for everybody even in presence of some spurious tests that we have. But also threads-required seems like a nice option to add to these specific time intensive tests to hopefully reduce the duration of these individual tests. I wonder if it also makes those heavy tests start in the beginning of the run?

Land the remaining improvements while waiting for the infrastructure setup to get completed.

This will mean that some tests will run multiple times until infrastructure can be changed...

.config/nextest.toml

Cause CI scripts can't be changed…

I anticipate that changing the script will not let the CI run…

nagisa · 2023-10-02T08:56:39Z

@nikurt @Longarithm could I request a review here?

nikurt · 2023-10-02T10:03:21Z

test-utils/style/src/lib.rs

+}
+
+#[test]
+fn clippy() {


This function duplicates scripts/run_clippy.sh. I see that you plan to delete that file in #9290.
Should be fine, but maybe add a TODO to remove the duplicated files.

nikurt · 2023-10-02T10:03:41Z

core/o11y/src/testonly.rs

@@ -31,24 +31,24 @@ fn setup_subscriber_from_filter(mut env_filter: EnvFilter) {
 }

 pub fn init_test_logger() {
-    let env_filter = EnvFilter::new("tokio_reactor=info,tokio_core=info,hyper=info,debug");
+    let env_filter = EnvFilter::new("cranelift=warn,wasmtime=warn,h2=warn,tower=warn,trust_dns=warn,tokio_reactor=info,tokio_core=info,hyper=info,debug");


nikurt · 2023-10-02T10:05:39Z

core/o11y/src/io_tracer.rs

@@ -117,7 +117,7 @@ impl<S: Subscriber + for<'span> LookupSpan<'span>> Layer<S> for IoTraceLayer {
        }
    }

-    #[allow(clippy::integer_arithmetic)]
+    #[allow(clippy::arithmetic_side_effects)]


Please also update docs/practices/style.md

285:`clippy::integer_arithmetic` lints.

…t` (#9608) I got absolutely fed up with waiting for prerequisite infrastructure work for #9290 (why does it have to be _that_ hard?) However the PR in question had some other important improvements that do not necessarily *rely* on changes to said infrastructure to work. In particular: * `nextest` now retries failing tests a few times before giving up, to make sure they aren't spuriously failing; * This should help some timing out integration tests in particular, as those often fail because of some deadlockish situation in my experience. * style checks still run with `cargo nextest` – unfortunately that means that in CI this will run these checks multiple times, but that doesn’t sound particularly terrible of a tradeoff (especially if the other changes mean we won't be retrying entire test suites as often anymore;) * This should allow for a greater portion of the test suite to run on Macs – unfortunately not verified by the CI, but people do complain and this should make the situation better. cc #9367

nagisa added 14 commits September 21, 2023 11:04

Replace buildkite with GHA for Rust tests?

c951582

Filter out cranelift and wasmtime from test logs

b6bd5bc

When run on an m1 these can result in a very significant logging!

Try this???

216acf7

Allow reuse of target directory for style tests

15111f8

Checking clippy can take quite some time, and locally it is better to spend some disk space for quick reruns over having to wait 2 minutes or so before the test concludes.

increase clippy timeout further

26d4ebe

It seems like we end up blocking on some directories sometimes, and

ignore near-vm-test-api tests on aarch64 which does not support these

e67cc06

Move more style checks over to nextest

71a394b

Add nightly vs non-nightly test suites

4b56709

Rebase

7d72555

Switch to large github runner?

b2f700e

Revert just the CI runner changes for the time being

920a63e

Land the remaining improvements while waiting for the infrastructure setup to get completed.

revert changes to scripts

c7f0522

This will mean that some tests will run multiple times until infrastructure can be changed...

nagisa requested review from wacban and akhi3030 September 28, 2023 07:48

nagisa requested a review from a team as a code owner September 28, 2023 07:48

Rename deprecated lint

97696dd

nagisa commented Sep 28, 2023

View reviewed changes

.config/nextest.toml Show resolved Hide resolved

nagisa added 2 commits September 28, 2023 11:12

apply retry logic to the default profile

abbc32b

Cause CI scripts can't be changed…

Fix broken script by adapting stuff it looks at

3c992e5

I anticipate that changing the script will not let the CI run…

nagisa enabled auto-merge September 28, 2023 10:09

nagisa mentioned this pull request Sep 28, 2023

🔷 [2023-09] throughout which it will hopefully become more pleasant to wake up on workdays #9510

Closed

nagisa requested review from Longarithm and nikurt October 2, 2023 08:56

nikurt approved these changes Oct 2, 2023

View reviewed changes

nagisa added this pull request to the merge queue Oct 2, 2023

Merged via the queue into master with commit 90fb333 Oct 2, 2023

nagisa deleted the nagisa/cargo-tests-style-excerpt branch October 2, 2023 10:26

wacban mentioned this pull request Oct 4, 2023

ci: move a chunk of the Rust CI over to GHA #9290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: improve test resilience and run style checks within `cargo nextest` #9608

ci: improve test resilience and run style checks within `cargo nextest` #9608

nagisa commented Sep 28, 2023

nagisa commented Oct 2, 2023

nikurt Oct 2, 2023

nikurt Oct 2, 2023

nikurt Oct 2, 2023

ci: improve test resilience and run style checks within cargo nextest #9608

ci: improve test resilience and run style checks within cargo nextest #9608

Conversation

nagisa commented Sep 28, 2023

nagisa commented Oct 2, 2023

nikurt Oct 2, 2023

Choose a reason for hiding this comment

nikurt Oct 2, 2023

Choose a reason for hiding this comment

nikurt Oct 2, 2023

Choose a reason for hiding this comment

ci: improve test resilience and run style checks within `cargo nextest` #9608

ci: improve test resilience and run style checks within `cargo nextest` #9608