Add compiletest and bootstrap "--skip" option forwarded to libtest #96493
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
It looks like curently filtering tests in the positive direction (rather than excluding some tests) is done by libtest, not within compiletest, and I believe the support there is more primitive, being based on just substrings. I think we should (a) not diverge from the filtering in terms of supported syntax and (b) should push this functionality down into libtest, like we do for regular filtering. I don't see any reason it should be special to compiletest, and I think the functionality makes sense more generally -- excluding a subset or allowing just a subset are both roughly equivalent to me. If you'd like to make those changes here, that would be fine, or feel free to open a new PR -- we will want to start with the exclude option being unstable in libtest, but that shouldn't impede use from compiletest. |
Mark-Simulacrum
added
S-waiting-on-author
|
@Mark-Simulacrum Thanks for the tip, makes sense. Just to check before I open a PR: is that an OK change without an RFC? At least for adding the unstable option. |
|
An unstable addition can happen with just a T-libs-api member signing off on a PR, no RFC necessary. |
chbaker0
force-pushed
the
issue-96342-fix
branch
2 times, most recently
from
3c6aad4
to
6a54090
Compare
rustbot
added
the
T-compiler
|
@rustbot label -S-waiting-on-author +S-waiting-on-review @Mark-Simulacrum I didn't do enough digging; "--skip" in libtest does what I want. The PR now just adds the arguments to bootstrap and compiletest and forwards them down to libtest. |
rustbot
added
S-waiting-on-review
chbaker0
changed the title
This comment has been minimized.
This comment has been minimized.
libtest already supports a "--skip SUBSTRING" arg which excludes any test names matching SUBSTRING. This adds a "--skip" argument to compiletest and bootstrap which is forwarded to libtest.
|
I'm not sure we should be exposing a separate helper flag for this in bootstrap, but it also seems OK. Certainly likely to make it more discoverable, which is probably good. @bors r+ |
|
📌 Commit b2316c1 has been approved by |
bors
added
S-waiting-on-bors
…mulacrum Add compiletest and bootstrap "--skip" option forwarded to libtest With this PR, "x.py test --skip SKIP ..." will run the specified test suite, but forward "--skip SKIP" to the test tool. libtest already supports this option. The PR also adds it to compiletest which itself just forwards it to libtest. Adds the functionality requested in rust-lang#96342. This is useful to work around tests broken upstream. rust-lang#96362 (comment) is the specific test issue my project is trying to work around.
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (925e774): comparison url. Summary: This benchmark run did not return any relevant results. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Rolls a new Rust toolchain with the following changes in addition to updating the revision: With rust-lang/rust#96493, disables test src/test/ui/numeric/numeric-cast.rs (which fails on our CI due to bug rust-lang/rust#94322) and re-enables the rest of the src/test/ui suite. Includes Rust std sources and vendored dependencies in the package at third_party/rust-toolchain/lib/rustlib/src/rust/ Fixed: 1320459 Change-Id: I6b25be163214c4408123f3abb7e0135e94ec642e Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-rel,linux-rust-x64-dbg Change-Id: I6b25be163214c4408123f3abb7e0135e94ec642e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3635438 Commit-Queue: Collin Baker <collinbaker@chromium.org> Reviewed-by: Adrian Taylor <adetaylor@chromium.org> Cr-Commit-Position: refs/heads/main@{#1003924}
|
This seems like a misfeature. There are two existing ways to do this:
I think if we're going to support this specially, it should be with (@Mark-Simulacrum can you please tag me on PRs that propose to fix an issue I opened in the future? I didn't even know this existed until I looked through a list of bootstrap PRs for unrelated reasons, the issue wasn't actually closed by this PR because didn't have "fixes" in the description.) |
|
Sure, if I notice. I was ambivalent at best about this at the time of addition, and FWIW I don't feel strongly we need to keep it. Exclude and skip are pretty different, I think (skip works on test name, not path, which overlaps only for compiletest), but --test-args already working for this is definitely true. |
|
yeah, I think it makes sense to have |
…imulacrum Move `x test --skip` to be part of `--exclude` `--skip` is inconsistent with the rest of the interface and redundant with `--exclude`. Fix --exclude to work properly for files and directories rather than having a separate flag. Fixes rust-lang#96342. cc rust-lang#96493 (comment) r? `@Mark-Simulacrum`
Move `x test --skip` to be part of `--exclude` `--skip` is inconsistent with the rest of the interface and redundant with `--exclude`. Fix --exclude to work properly for files and directories rather than having a separate flag. Fixes rust-lang/rust#96342. cc rust-lang/rust#96493 (comment) r? `@Mark-Simulacrum`
Move `x test --skip` to be part of `--exclude` `--skip` is inconsistent with the rest of the interface and redundant with `--exclude`. Fix --exclude to work properly for files and directories rather than having a separate flag. Fixes rust-lang/rust#96342. cc rust-lang/rust#96493 (comment) r? `@Mark-Simulacrum`
With this PR, "x.py test --skip SKIP ..." will run the specified test suite, but forward "--skip SKIP" to the test tool. libtest already supports this option. The PR also adds it to compiletest which itself just forwards it to libtest.
Adds the functionality requested in #96342. This is useful to work around tests broken upstream.
#96362 (comment) is the specific test issue my project is trying to work around.