-
Notifications
You must be signed in to change notification settings - Fork 13.3k
compiletest: Add an experimental new executor to replace libtest #139660
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
Conversation
Some changes occurred in src/tools/compiletest cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
62d2626
to
3f2fa86
Compare
pro gamer move I'll look at this tmrw/Sunday. |
Is there some history behind this change? |
This is in the context of the stage 0 std redesign (#119899). Currently, If we no longer depend on libtest, then we won't need this |
Depending on libtest internals is a big maintenance headache for compiletest. We already have a bunch of annoying hacks in compiletest to work around limitations that libtest imposes. And changing libtest to better meet compiletest's needs is very risky, because libtest is the backbone of testing across most of the wider Rust ecosystem. What has made this change even more desirable is the upcoming stage 0 redesign (#119899). Currently compiletest relies on an in-tree libtest built with the stage 0 compiler. But after the redesign, stage 0 builds of the standard library will no longer be supported, so compiletest will have to rely on either a stage 1 build of libtest (resulting in unacceptable workflow regressions for compiler contributors), or the pre-built stage 0 libtest (which can go out of sync with the in-tree version, causing various bootstrapping problems). This replacement still needs unstable library features (for now), but |
Tweaked some comments (diff); no functional change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! The new executor implementation here is very nice to follow, and having more control over how compiletest actually executes the tests is very much appreciated!
I left two nits, but otherwise the changes look good to me.
|
||
/// Applies command-line arguments for filtering/skipping tests by name. | ||
/// | ||
/// Adapted from `filter_tests` in libtest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: can you leave me a note here to "reconsider test filtering behavior in new executor"? One gripe I had with libtest's test filtering was that after compiletest passes to libtest the canonicalized test name (based on a relative path), the default filter logic is a naive substring match which can often produce some surprising running more tests than expected behavior. A benefit of rolling our own executor here is to allow us to redesign the filter logic (as a possibility).
/// | ||
/// Copied from `get_concurrency` in libtest. | ||
fn get_concurrency() -> usize { | ||
if let Ok(value) = env::var("RUST_TEST_THREADS") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion: Since the only intended consumer of compiletest's cli is bootstrap nowadays( Although I guess even if it was an cli flag, we need also need to process rustdoc-gui-test
uses compiletest programmatically), should this instead be a cli flag instead of an env var?RUST_TEST_THREADS
anyway.
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
☔ The latest upstream changes (presumably #139781) made this pull request unmergeable. Please resolve the merge conflicts. |
The new executor can be enabled by passing `--new-executor` or `-n` to compiletest. For example: `./x test ui -- -n`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
@bors r+ rollup |
Rollup of 17 pull requests Successful merges: - rust-lang#138374 (Enable contracts for const functions) - rust-lang#138380 (ci: add runners for vanilla LLVM 20) - rust-lang#138393 (Allow const patterns of matches to contain pattern types) - rust-lang#139517 (std: sys: process: uefi: Use NULL stdin by default) - rust-lang#139554 (std: add Output::exit_ok) - rust-lang#139660 (compiletest: Add an experimental new executor to replace libtest) - rust-lang#139669 (Overhaul `AssocItem`) - rust-lang#139671 (Proc macro span API redesign: Replace proc_macro::SourceFile by Span::{file, local_file}) - rust-lang#139750 (std/thread: Use default stack size from menuconfig for NuttX) - rust-lang#139772 (Remove `hir::Map`) - rust-lang#139785 (Let CStrings be either 1 or 2 byte aligned.) - rust-lang#139789 (do not unnecessarily leak auto traits in item bounds) - rust-lang#139791 (drop global where-bounds before merging candidates) - rust-lang#139798 (normalize: prefer `ParamEnv` over `AliasBound` candidates) - rust-lang#139822 (Fix: Map EOPNOTSUPP to ErrorKind::Unsupported on Unix) - rust-lang#139833 (Fix some HIR pretty-printing problems) - rust-lang#139836 (Basic tests of MPMC receiver cloning) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#139660 - Zalathar:new-executor, r=jieyouxu compiletest: Add an experimental new executor to replace libtest This PR adds a new "executor" to compiletest for running the list of collected tests, to eventually replace the current dependency on unstable libtest internals. The new executor is currently inactive by default. It must be activated explicitly by passing `-n` or `--new-executor` to compiletest, e.g. `./x test ui -- -n`. (After some amount of wider manual testing, the new executor will hopefully be made the default, and the libtest dependency can be removed. Contributors should not notice any change.) The new executor is a stripped-down rewrite of the subset of libtest needed by compiletest. # Supported functionality - Specifying the number of concurrent tests with `RUST_TEST_THREADS` - Filtering and skipping tests by name (substring or exact-match) - Forcibly running ignored tests with `--ignored` - Optional fail-fast with `--fail-fast` - JSON output, compatible with bootstrap's parser for libtest output - Running each test in its own thread - Short backtraces that ignore the executor itself - Slow test detection, with a hard-coded timeout of 60 seconds - Capturing stdout/stderr, via `#![feature(internal_output_capture)]` - Suppressing output capture with `--no-capture` # Unsupported functionality - Non-JSON output, as this is handled by bootstrap instead - Separate code path for concurrency=1, as the concurrent path should handle this case naturally - Fallback to running tests synchronously if new threads can't be spawned - Special handling of hosts that don't support basic functionality like threads or timers - Our ability to test *targets* should be unaffected - Graceful handling of some edge cases that could occur in arbitrary user-written unit tests, but would represent bugs in compiletest - Due to the current need for output capture, the new executor is still not entirely written in stable Rust --- r? jieyouxu
fn remove_tests_past_deadline(&mut self) -> Vec<DeadlineEntry<'a>> { | ||
let now = Instant::now(); | ||
let mut timed_out = vec![]; | ||
while let Some(deadline_entry) = pop_front_if(&mut self.queue, |entry| now < entry.deadline) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be now >= entry.deadline
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, you're right. I'll make a fix.
Rollup of 17 pull requests Successful merges: - rust-lang#138374 (Enable contracts for const functions) - rust-lang#138380 (ci: add runners for vanilla LLVM 20) - rust-lang#138393 (Allow const patterns of matches to contain pattern types) - rust-lang#139517 (std: sys: process: uefi: Use NULL stdin by default) - rust-lang#139554 (std: add Output::exit_ok) - rust-lang#139660 (compiletest: Add an experimental new executor to replace libtest) - rust-lang#139669 (Overhaul `AssocItem`) - rust-lang#139671 (Proc macro span API redesign: Replace proc_macro::SourceFile by Span::{file, local_file}) - rust-lang#139750 (std/thread: Use default stack size from menuconfig for NuttX) - rust-lang#139772 (Remove `hir::Map`) - rust-lang#139785 (Let CStrings be either 1 or 2 byte aligned.) - rust-lang#139789 (do not unnecessarily leak auto traits in item bounds) - rust-lang#139791 (drop global where-bounds before merging candidates) - rust-lang#139798 (normalize: prefer `ParamEnv` over `AliasBound` candidates) - rust-lang#139822 (Fix: Map EOPNOTSUPP to ErrorKind::Unsupported on Unix) - rust-lang#139833 (Fix some HIR pretty-printing problems) - rust-lang#139836 (Basic tests of MPMC receiver cloning) r? `@ghost` `@rustbot` modify labels: rollup
compiletest: Fix deadline bugs in new executor The experimental new executor for compiletest (rust-lang#139660) was found to have two major bugs in deadline handling for detecting slow tests: - The comparison between `now` and test deadlines was reversed, causing no timeouts to ever be recognised. - After fixing that bug, it was found that the existing code would issue timeouts for any test that had started more than 60 seconds ago, even if the test had finished long before its deadline was reached. This PR fixes those bugs. (The new executor is not yet enabled by default, so this PR has no immediate effect on contributors.) --- I noted in rust-lang#139998 (comment) that I hoped to have some unit tests to accompany these fixes. Unfortunately that turned out to be infeasible, because `DeadlineQueue` is tightly coupled to concrete `mpsc::Receiver` APIs (in addition to `Instant::now`), and trying to mock all of those would make the code much more complicated. I did, however, add a few assertions that would have caught the failure to remove tests from the queue after their deadline. r? jieyouxu
Rollup merge of rust-lang#140031 - Zalathar:deadline, r=jieyouxu compiletest: Fix deadline bugs in new executor The experimental new executor for compiletest (rust-lang#139660) was found to have two major bugs in deadline handling for detecting slow tests: - The comparison between `now` and test deadlines was reversed, causing no timeouts to ever be recognised. - After fixing that bug, it was found that the existing code would issue timeouts for any test that had started more than 60 seconds ago, even if the test had finished long before its deadline was reached. This PR fixes those bugs. (The new executor is not yet enabled by default, so this PR has no immediate effect on contributors.) --- I noted in rust-lang#139998 (comment) that I hoped to have some unit tests to accompany these fixes. Unfortunately that turned out to be infeasible, because `DeadlineQueue` is tightly coupled to concrete `mpsc::Receiver` APIs (in addition to `Instant::now`), and trying to mock all of those would make the code much more complicated. I did, however, add a few assertions that would have caught the failure to remove tests from the queue after their deadline. r? jieyouxu
compiletest: Use the new non-libtest executor by default The new executor was implemented in rust-lang#139660, but required a manual opt-in. This PR activates the new executor by default, but leaves the old libtest-based executor in place (temporarily) to make reverting easier if something unexpectedly goes horribly wrong. Currently the new executor can be explicitly disabled by passing the `-N` flag to compiletest (e.g. `./x test ui -- -N`), but eventually that flag will be removed, alongside the removal of the libtest dependency. The flag is mostly there to make manual comparative testing easier if something does go wrong. As before, there *should* be no user-visible difference between the old executor and the new executor. --- I didn't get much of a response to my [call for testing thread on Zulip](https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/Call.20for.20testing.3A.20New.20test.20executor.20for.20compiletest/with/512452105), and the reports I did get (along with my own usage) indicate that there aren't any problems. So I think it's reasonable to move forward with making this the default, in the hopes of being able to remove the libtest dependency relatively soon. When the libtest dependency is removed, it should be reasonable to build compiletest against pre-built stage0 std by default, even after the stage0 redesign. (Though we should probably have at least one CI job using in-tree stage1 std instead, to guard against the possibility of the `#![feature(internal_output_capture)]` API actually changing.)
compiletest: Use the new non-libtest executor by default The new executor was implemented in rust-lang#139660, but required a manual opt-in. This PR activates the new executor by default, but leaves the old libtest-based executor in place (temporarily) to make reverting easier if something unexpectedly goes horribly wrong. Currently the new executor can be explicitly disabled by passing the `-N` flag to compiletest (e.g. `./x test ui -- -N`), but eventually that flag will be removed, alongside the removal of the libtest dependency. The flag is mostly there to make manual comparative testing easier if something does go wrong. As before, there *should* be no user-visible difference between the old executor and the new executor. --- I didn't get much of a response to my [call for testing thread on Zulip](https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/Call.20for.20testing.3A.20New.20test.20executor.20for.20compiletest/with/512452105), and the reports I did get (along with my own usage) indicate that there aren't any problems. So I think it's reasonable to move forward with making this the default, in the hopes of being able to remove the libtest dependency relatively soon. When the libtest dependency is removed, it should be reasonable to build compiletest against pre-built stage0 std by default, even after the stage0 redesign. (Though we should probably have at least one CI job using in-tree stage1 std instead, to guard against the possibility of the `#![feature(internal_output_capture)]` API actually changing.)
This PR adds a new "executor" to compiletest for running the list of collected tests, to eventually replace the current dependency on unstable libtest internals.
The new executor is currently inactive by default. It must be activated explicitly by passing
-n
or--new-executor
to compiletest, e.g../x test ui -- -n
.(After some amount of wider manual testing, the new executor will hopefully be made the default, and the libtest dependency can be removed. Contributors should not notice any change.)
The new executor is a stripped-down rewrite of the subset of libtest needed by compiletest.
Supported functionality
RUST_TEST_THREADS
--ignored
--fail-fast
#![feature(internal_output_capture)]
--no-capture
Unsupported functionality
r? jieyouxu