Proof of concept: implement test ignore-by-panic

[CAD97]

Implements dynamic ignoring of tests via panic_any(IgnoreTest { reason }). Additionally, does the work to pass through the ignored status via exit code across the process boundary in order to support doing so both in process-isolated cargo test tests and rustdoc documentation tests.

The actual API might look like panic_any(test::IgnoreTest { reason }) or test::ignore(reason) -> !; I've just implemented the mechanism.

I know this has been asked about before, though I can't find references at the moment.

The sketchiest part is enabling this in rustdoc, since it needs to inject uses of unstable API into the doctest. I've gone for the hacky solution of only enabling it when an attribute feature(test) and extern crate test; are both present in the test but would appreciate advice here on a better way to gate this. Just always adding the extra shim on the nightly toolchain isn't the most desirable, as then doctests would be able to use feature(test) without explicitly saying #![feature(test)].

@rustbot modify labels +T-lang +T-rustdoc

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @notriddle

(rust-highfive has picked a reviewer for you, use r? to override)

[CAD97]

@rustbot modify labels +T-libs -T-lang

whoops typed the wrong thing

[CAD97]

Per highfive I guess I should also

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

[ogoffart]

I know this has been asked about before, though I can't find references at the moment.

rust-lang/rfcs#3221

[yanganto]

This also relates to #68007

[CAD97]

I think probably the best way to handle the doctest wrapping will probably be an #[allow_internal_unstable] macro. Using a macro will allow to define the injected code in a hygienic manner.

It still requires stabilizing the macro or magicking up some way of injecting it without impacting user code, but I do think it'd be useful to use. But that's not really the point of this PR.

[m-ou-se]

I've also heard folks requesting a feature to allow them to mark a test as failed without returning/exiting/panicking just yet. That makes me wonder if that should use the same feature as a dynamic ignore feature.

For example, something like:

// to ignore:
test::set_result(TestResult::Ignored, "message");
return;

// to fail, but continue checking other checks in the same test:
test::set_result(TestResult::Failed, "message");

(cc @de-vri-es for assert2's check!() macro)

It's not clear to me this would be a better solution, as it might be tricky to define what happens when different results are set this way, or when the test panics after setting a result. But I do think we should consider other use cases of dynamically picking a test result before deciding to go for the panic_any route for ignoring tests.

[de-vri-es]

Yes, I would love a way to mark a test as failed without panicking. For assert2::check!(...) specifically, we wouldn't need a failure message as it already does it's own printing. But in general it does sound like a nice feature.

An easy choice for dealing with conflicting results would simply be to panic, which would end up failing the test. However, for assert2::check!() we would at-least want to be able to call test::set_result(TestResult::Failed, "check failed") multiple times without panicking.

[mina86]

As per discussion in rust-lang/rfcs#3221 the two issues here are requirement for #![feature(test)] and incompatibility with --ignored flag (i.e. the test would be shown as ignored but it would be impossible to force run it via --ignored flag which is in contrast to tests ignored via #[ignore]). The latter can be solved by calling such tests ‘skipped’. But the former I feel is still an issue.

[CAD97]

it would be impossible to force run it via --ignored flag

--ignored is already kinda broken due to the use of ```ignore in doctests, but fundamentally I agree that making the situation worse is not great (I'm working to make it better).

This PR is a mechanism for signalling an ignore during runtime, but I suspect that the actual API might look more like fn test::ignore(reason: Option<String>), which would allow the test runtime to check a global to see if --ignored was used, and ignore the ignoring. It might also be useful to have a test::skip(reason: String) -> ! that is guaranteed to abort the test, but users could also "just" write test::ignore(None); panic("I meant it"); if letting the test fail normally is a bad idea.

requirement for #![feature(test)]

I mean, initially, it's going to be feature gated anyway, so while this is something that needs to be resolved for stabilization, it doesn't need to block implementation.

I don't see a way to properly expose this feature without stabilizing (minimally the existence of) the test support crate, though. You could provide the APIs as part of std... but that seriously smells like putting items in the wrong place just to get around the test crate being unstable currently.

[mina86]

I don't see a way to properly expose this feature without stabilizing

The linked draft RFC proposes extending the ignore macro, e.g.:

fn missing_avx_support() -> bool {
    !std::is_x86_feature_detected!("avx")
}

#[test]
#[ignore(if = missing_avx_support, reason = "missing AVX support")]
fn test_memcpy_avx() {
    // ...
}

My worry with requiring #![feature(test)] is that it could
unnecessarily delay this feature if there is no consensus regarding
the test support crate.

[CAD97]

#[ignore(if)] would be implemented to either add more fields to TestDesc or it could use this functionality internally.

Ultimately, I consider a more declarative interface like #[ignore(if)] separate from the imperative test::ignore(), even if they have similar use cases.

#[ignore(if)] suffers from TOCTOU style and effectively has to check twice:

#[test]
fn with_panic() {
    let chrome = match headless_chrome::start() {
        Ok(chrome) => chrome,
        Err(err) => test::abort(format!("headless chrome is not available: {err}")),
    }
    // do test
}

#[test]
#[ignore(if = headless_chrome::exists, reason = "headless chrome is not available")]
fn with_if() {
    let chrome = match headless_chrome::start() {
        Ok(chrome) => chrome,
        Err(err) => panic!("headless chrome is not available: {err}"),
    }
    // do test
}

This is especially important for is_x86_feature_detected! to check even if you aren't #[ignore]d, since if --include-ignored is used on a box without the feature you'll get just plain UB.

#[ignore(if)] can be useful, but I think is complementary to an ignore-by-panic mechanism.

[yanganto]

Hi @m-ou-se, @de-vri-es ,
The thing people ask maybe just to fail a test suite and to keep testing all the other test cases in the test suite.

// to fail, but continue checking other checks in the same test:
test::set_result(TestResult::Failed, "message");

The following solution may help them, and I believe that ignoring runtime is a different issue.

Changing #[test] on mod as a test suite could help with this, this can be enhanced by some test frameworks.

#[cfg(test)]
#[test]
mod infra {
   #[test]
   fn test_infra_works () {...}
   
   #[test]
   fn test_infra_fail () {...}
}

cargo test

running 2 tests
test infra::* ... FAILED
test infra::test_infra_works ... ok
test infra::test_infra_fail ... FAILED

test suite result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out;
test case result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; 
finished in 0.00s

Because it is a different scenario, could we consider ignore-by-panic without setting test fail on runtime?

[Boegie19]

@yanganto
I personally don't think that should be the standard behaver
I think it should be done in the same way as cargo run --no-fail-fast so it should be an option as a argument of cargo run.

[yanganto]

Hi @Boegie19,

Thanks for the advice.

This PR is a mechanism for signalling an ignore during runtime

and also address #68007, which wants to ignore the test in cargo test.

cargo run --no-fail-fast is away from the topic. I might make an RFC for discussing the #[test] on mod or also use Zulip first. If you are interested in this and want to move forward, it is welcome to tag me somewhere.

As you can see, we are just trying to provide some proposals here, and we do not have the power to make sure everything will happen. Such that #68007 was opened more than two years but there is no solution fully accepted to solve it now. Nevertheless, it is always a good try to make this better, because we love to use Rust to solve problems in a smart way and keep safe at the same time.

[Boegie19]

Hi @yanganto

My previous message

The thing people ask maybe just to fail a test suite and to keep testing all the other test cases in the test suite.

was as a reaction to

The thing people ask maybe just to fail a test suite and to keep testing all the other test cases in the test suite.

what you suggested in this post should be under a cargo argument and this should also probably be an other pr ;)

[CAD97]

(triaging my open PRs)

I'm going to close this PR for now. The implementation baseline can likely be reused in the future (is there a place to collect closed-but-potentially-interesting PRs?), but I think a different API surface should be pursued. Namely:

• Five(?) new user functions in the test crate:
  • fn test::mark_ignored — mark the current test as ignored; continue.
  • fn test::mark_failed — mark the current test as failed; continue.
  • fn test::ignore — mark the current test as ignored; continue if --include-ignored, otherwise abort (unwind).
  • fn test::skip — mark the current test as ignored; if --include-ignored, fail; abort (unwind).
    • Could be omitted, as it's just an alias for {test::ignore(); test::fail()}.
  • fn test::fail — mark the current test as failed; abort (unwind).
• Failure overrides ignoring.
• ignore, skip, and fail panic_any (or maybe resume_unwind?) with an internal test::Aborted type. catch_unwind will still prevent an unwind if the test is running under panic = "unwind".
• test::run_test translates a panic with test::Aborted into the marked result. (If no result has been marked, this is reported as an internal error.) Any other panic is reported as a failure due to that panic (overriding any marks).
• The rustdoc runner develops some way to wrap the doctests in test::run_test or maybe a test::inline_test! without impacting the stability of the tested code.
  • Perhaps this is just not doing so until the API is stable? Would be an unfortunate limitation of doctests, but perhaps an understandable one. The test crate could be unavailable in doctests, preventing any use of the API, or could just be an unwind.
• A possible way to stabilize access to this without stabilizing the test crate as existing would be to provide preluded macros under cfg(test). Personally, though, I think "there exists a standard test crate" is the easy part of the stabilization, and it's the testing APIs which are the hard part; there's just not been a sufficiently standalone API subset to justify stabilizing the test crate. (Just black_box is the best contender, but that got properly exposed in std::hint. And is a mini controversy of its own.
• W.r.t. custom runners, these functions should all be defined to panic if the current test runner does not support the operation.