compiletest: Add an experimental new executor to replace libtest by Zalathar · Pull Request #139660 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation19 Commits3 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
Area: The testsuite used to check the correctness of rustc
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
Some changes occurred in src/tools/compiletest
cc @jieyouxu
This comment has been minimized.
compiletest: Add an experimental "new" executor to replace libtest compiletest: Add an experimental new executor to replace libtest
pro gamer move
I'll look at this tmrw/Sunday.
and the libtest dependency can be removed
Is there some history behind this change?
Why is it so important to remove the libtest dependency?
and the libtest dependency can be removed
Is there some history behind this change? Why is it so important to remove the libtest dependency?
This is in the context of the stage 0 std redesign (#119899). Currently, compiletest depends on in-tree libtest (unless compiletest-use-stage0-libtest = true is specified). For compiler workflows, this means that compiletest will be rebuilt when iterating on compiler changes which feels quite bad with the stage 0 std redesign otherwise (see #t-infra/bootstrap > Review thread: stage 0 redesign PR @ 💬 discussions).
If we no longer depend on libtest, then we won't need this compiletest-use-stage0-libtest = true workaround/hack.
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 internal_output_capture is a much smaller unstable surface area. And in the future it should be possible to work towards removing the need for output capture in compiletest, making it 100% compatible with stable Rust. That removes a lot of the maintenance concerns associated with wanting to build it with the stage 0 compiler.
Tweaked some comments (diff); no functional change.
Member
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. |
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") { |
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.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Reminder, once the PR becomes ready for a review, use @rustbot ready.
The new executor can be enabled by passing --new-executor or -n to
compiletest.
For example: ./x test ui -- -n
Rebased to fix trivial conflicts, then added some FIXMEs to filter_tests and get_concurrency to address feedback (diff).
@rustbot ready
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
📌 Commit e3d6813 has been approved by jieyouxu
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
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 |
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?
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.
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request
ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request
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
nowand 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
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
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
nowand 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
bors added a commit to rust-lang-ci/rust that referenced this pull request
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, 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.)
bors added a commit to rust-lang-ci/rust that referenced this pull request
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, 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.)
bors added a commit to rust-lang-ci/rust that referenced this pull request
compiletest: Re-land using the new non-libtest executor by default
This PR re-lands rust-lang#139998, which had the misfortune of triggering download-rustc in its CI jobs, so we didn't get proper test metrics for comparison with the old implementation. So that was PR was reverted in rust-lang#140233, with the intention of re-landing it alongside a dummy compiler change to inhibit download-rustc.
Original PR description for rust-lang#139998:
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
-Nflag 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.
r? jieyouxu
Labels
Area: The compiletest test runner
Area: The testsuite used to check the correctness of rustc
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)