[cargo-miri] support nextest

[sunshowers]

Add the ability to run cargo miri nextest list and cargo miri nextest run.

cargo-nextest is a new test runner for Rust maintained mostly by myself. It has several new features, but the most relevant to miri is the fact that it runs each test in its own process. This gives miri users better leak detection (#1481) for free, for example.

See nextest-rs/nextest#181 for discussion, including comments by @eddyb and @RalfJung.

Future work might be to have miri read the list of tests (or test binaries) generated by nextest list. @eddyb thinks that might be useful.

I tested cargo miri nextest run against smallvec, and it worked great.

Note: Running tests out of archives is currently broken, as the comment in run-test.py explains.

[RalfJung]

Thanks for the PR!

I am not sure what the best way is to do this. Given that nextest is a third-party cargo command, and given how many third-party cargo commands there are that we can impossibly all support in Miri -- I am not sure where to draw the line.

In particular I don't think we want to be on the hook for maintaining this support, i.e., it should be best-effort and nextest breaking should probably not block our CI. Instead, whoever cares about this support should have a cron job to detect regressions. I view this as being unofficially supported by Miri. @sunshowers you seem to be part of nextest upstream, would that work for you?

[bors]

☔ The latest upstream changes (presumably #2399) made this pull request unmergeable. Please resolve the merge conflicts.

[sunshowers]

Sounds good, I should just be able to rebase and simplify this. Happy to check against miri in nextest's CI rather than here.

[saethlin]

I would very much like to have each test run in its own interpreter, to protect against runaway memory usage in the main thread which I think I have seen happen with normal cargo miri test. The default test runner doesn't support this unless you compile with -Cpanic=abort (and even then, we'd need subprocess support in Miri to make it work. This... just works.). Also the default test runner's test timing/timeout logic seems very broken to me. I've tried to use it with Miri and the behavior doesn't seem to make any sense. Maybe I'm just too lazy to put in the effort to patch up the default test runner, but it seems like if people have put in the effort to write a much better test runner it would be nice to use it.

I'm concerned that the parallelism is not something we want for Miri. The interpreter generally and especially the Stacked Borrows checker are very memory-hungry. I worry that cranking up the parallelism from what I'm pretty sure is none at all to maximum would cause a lot of users to go from "Miri runs forever" to "Miri crashes" or "Miri makes my system unresponsive" which is definitely something I have seen with current Miri on my desktop Linux system, it just takes a very long time at the moment for the memory usage to hit 128 GB. Try using this on the adler crate.

Also, with this patch if I ctrl+c a test suite (which you will probably want to do with crates like adler or regex) it looks like stdout never get flushed or something. I don't get my prompt back and I have to hit enter after error: test run failed is printed.

[sunshowers]

Maybe I'm just too lazy to put in the effort to patch up the default test runner, but it seems like if people have put in the effort to write a much better test runner it would be nice to use it.

Thanks! Note that there was already an effort to try and change the default runner in rust-lang/cargo#10052 -- it was deemed to not be possible due to BC constraints. nextest doesn't have those constraints -- most projects are compatible with it but not all.

I'm concerned that the parallelism is not something we want for Miri. The interpreter generally and especially the Stacked Borrows checker are very memory-hungry. I worry that cranking up the parallelism from what I'm pretty sure is none at all to maximum would cause a lot of users to go from "Miri runs forever" to "Miri crashes" or "Miri makes my system unresponsive" which is definitely something I have seen with current Miri on my desktop Linux system

This is a really good point. I was trying to figure out how miri ensures that tests are run in a single-threaded fashion. For now, single-threaded tests can be specified by -j1 or the test-threads configuration parameter.

BTW I'm currently testing out cargo miri nextest run against the regex crate with 24 threads on my dev machine with 64GB RAM,

Also, with this patch if I ctrl+c a test suite (which you will probably want to do with crates like adler or regex) it looks like stdout never get flushed or something. I don't get my prompt back and I have to hit enter after error: test run failed is printed.

Did some debugging -- this happens because when you hit ctrl-C, a SIGINT signal is sent to the entire process group. cargo-miri doesn't capture it so it exits immediately, while nextest handles it gracefully so it exits after cargo-miri does. The prompt does get displayed by my understanding but nextest's output ends up overwriting it.

A fix would be to add a ctrl-c handler to cargo-miri that waits for the invoked subprocess to exit. (Pressing ctrl-c twice exits cargo-miri immediately.) But it's probably not worth doing given that control is returned to the shell, and it's just the prompt that's missing.

---

One thing I noticed while testing this out on regex is that the list step also invokes miri, which is quite slow compared to the run step. Wondering if there's any sort of logic within miri that would enable using a native build for the list step and the miri interpreter for the run step.

[sunshowers]

I am not sure what the best way is to do this. Given that nextest is a third-party cargo command, and given how many third-party cargo commands there are that we can impossibly all support in Miri -- I am not sure where to draw the line.

My response would be that I think nextest delivers unique value to miri users that the default test runner does not, in terms of per-test isolation. I haven't had the opportunity to use miri myself, to be honest -- I just did this as a courtesy to our shared users.

I also don't think that making nextest support miri is practicable -- it's much easier (just a few lines of code) for miri to invoke nextest in this manner.

[RalfJung]

I was trying to figure out how miri ensures that tests are run in a single-threaded fashion.

We have patched the libtest runner with a cfg(miri) to enforce not using threads.

Also, available_parallelism always returns 1 inside Miri.

@saethlin I am confused by your comment, I must be missing some context. Is this a wishlist for the nextest devs or saying we shouldn't land this until X or ... ?

[saethlin]

Missing context is probably that I'm wondering if I could use nextest to address saethlin/crater-at-home#7 where I want tests run in their own interpreter and timed out independently. The nextest runner already does the first, and I think with some adjustment it could do the per-test timeouts (which the default test runner I think is supposed to be able to do).

My concern with parallelism and memory usage is that at least when I hear about it, everyone is excited about nextest because its strategy gets all your tests run faster, and this PR keeps the highly parallel behavior as the default (or at least it did about an hour ago when I tried it out). So I'm not sure what default behavior cargo miri nextest run should have. On the one hand getting all the tests run faster is awesome, but I'm not sure that the typical user of cargo miri test has a factor of available_parallelism or num_cpus::get() free memory when running cargo miri test. I'm not sure if users would find cargo miri nextest run defaulting to a single job too surprising, because nextest has earned a reputation for parallelism.

[RalfJung]

I see. That seems to me like something that can probably be figured out on the nextest side, if needed.

[sunshowers]

Will available_parallelism always return 1 if the miri environment variables are set? If so. nextest can just use that.

[RalfJung]

Will available_parallelism always return 1 if the miri environment variables are set? If so. nextest can just use that.

I was referring to code that runs inside Miri, i.e., interpreted code. So this depends on nextest architecture details that I have no clue about.

But if I would have to guess, it seems like the parallelism is handled outside Miri, so whatever Miri does makes no difference. Rather, nextest would have to discover that it runs as cargo miri nextest and alter its behavior.

[sunshowers]

Got it. I think we'll make nextest detect the miri environment and configure itself to using 1 thread by default then.

[RalfJung]

Got it. I think we'll make nextest detect the miri environment and configure itself to using 1 thread by default then.

Sounds good!
What would be the best way for you to detect this? Miri sets a ton of env vars but they are generally considered private. Probably the best one to check would be MIRI_SYSROOT.

[sunshowers]

Checking for MIRI_SYSROOT sounds good.

[sunshowers]

@saethlin

The nextest runner already does the first, and I think with some adjustment it could do the per-test timeouts (which the default test runner I think is supposed to be able to do).

Nextest has per-test timeouts via its configuration:

slow-timeout = { period = "60s", terminate-after = 2 }

I'd be hesitant to turn something like this on by default, though.

[saethlin]

That's awesome. I'm not interested in a default timeout for Miri overall, I just need a default for my own project that uses Miri :)

[sunshowers]

@saethlin once the next version of nextest comes out, you'll be able to define for your own project:

[profile.default-miri]
slow-timeout = { period = "60s", terminate-after = 2 }

and have tests terminate after 2 minutes. The next version will also have per-test overrides for slow-timeout so you can specify that some tests have longer or shorter timeouts.

[RalfJung]

Now that is the diff I was hoping for. :)
@bors r+

@saethlin if we see sufficient use and have someone around in case of issues, we can also consider making this more official by adding some tests, but I see that as a separate step. Let's get this off the ground first by allowing some experimentation.

[bors]

📌 Commit 88ad9ca has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit 88ad9ca with merge 084e02f...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 084e02f to master...

[saethlin]

FYI rust-lang/rust#99627 takes a big chunk out of cargo miri nextest startup time.

[eddyb]

Did the // Invalid command. comment get lost during a rebase?

[sunshowers]

I think so yeah, whoops!