Stabilize unit tests with non-() return type

[nikomatsakis]

CURRENT STATUS:

Awaiting someone to write the stabilization PR! Mentoring instructions here.

---

This is a sub-issue of the larger tracking issue devoted to the ? in main RFC. This issue corresponds to stabilizing the use of unit tests whose return type is something other than () -- it basically an extension of #48453, which was discussing the same thing but for the main function.

What would be stabilized

As before, unit tests that return ():

• Succeed unless they panic
• Can be annotated with #[should_panic]

However, unit tests can now have other return types:

• The return type must implement the Termination trait
  • Test and expected error
• The test passes if the return value is considered a "success" (e.g., an Ok from Result)
  • Test
• If the type is not (), then #[should_panic] is disallowed
  • Test and expected error

Unknowns

• Question: what's up with #[should_panic] and #[bench] anyway? (Example)
• Can we use this with rustdoc yet?

Older proposal

Possible changes needed before stabilizing

• Adjust libtest runner to distinguish #[should_panic] from #[should_error], as suggested by @scottmcm?
• Successful examples of using with rustdoc

What would be stabilized

Unit tests (and #[bench] functions) would join the main function in being allowed to return any value that implements the Termination trait.

This commits us to the following (each link is to a test):

• tests that return Result and other Termination types. In the case of Result:
  • Ok means test passes
  • Err causes test failure
  • more generally, success or failure is determined by invoking report(), although that detail is not being stabilized
• benchmarks work the same way (Ok, Err)
  • note that #[bench] is still effectively unstable though in general

What remains unstable

• The library details: where the trait is and its methods
  • Which methods the test runner invokes on the trait -- this is unstable as the trait itself is unstable

[nikomatsakis]

@rfcbot fcp merge

I propose that we further stabilize unit tests etc that return non-() return types.

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

• @aturon
• @cramertj
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

Concerns:

• experience (Stabilize unit tests with non-() return type #48854 (comment))

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[scottmcm]

Thanks for including the test links! Super helpful.

I think letting Ok(()) (and ExitCode::SUCCESS 🙂) be test pass is great.

This one surprised me, though:

#[test]
#[should_panic]
fn not_a_num() -> Result<(), ParseIntError> {
    let _: u32 = "abc".parse()?;
    Ok(())
}

I get cognitive dissonance from that, because I my brain says "what do you mean parse should panic? I thought the point was that it doesn't panic!", and I think it would be unfortunate if the test were to continue to pass should it actually start panicking.

I think I was expecting

• (unannotated): test pass is .report()==SUCCESS, everything else is test fail
• [should_panic]: catch_panic caught a panic is test pass, everything else is test fail
• [should_error] (hypothetical): test pass is .report()!=SUCCESS, everything else is test fail

(Unimportant thought: a -> ! test seems fundamentally useless. But probably not worth prohibiting?)

[cramertj]

Unimportant thought: a -> ! test seems fundamentally useless. But probably not worth prohibiting?

#[test]
#[should_panic]
fn panic_panics() -> ! { panic!() }

😛

[sgrif]

I think this definitely warrants eventually adding an alias for should_panic (perhaps should_error or should_fail), but that seems like it's strictly orthogonal to stabilizing this

[aturon]

cc @jonhoo re: test frameworks

[scottmcm]

@sgrif Note that I'm explicitly not suggesting that it should be an alias: I would like should_panic to only result in a test pass if there was an actual panic, which is something that I don't think we'd want to change after stabilization if there were a bunch of tests using should_panic to check for Err.

[sgrif]

@scottmcm Fair point. I agree.

[jonhoo]

In the context of custom test frameworks, this seems fine. The path we're taking going forward is that what ships with Rust by default is the same thing that libtest does at the time of the change, so committing to this now just means that the libtest we build using ctf will also have to support it (which seems fine).

Note that with ctfs, new test frameworks can implement entirely new testing annotations, and are not bound to the annotations, signatures, and semantics as the current built-in test support.

Also cc @Manishearth .

[nrc]

@rfcbot concern experience

Has this had much use? It's the first I knew about it even being a thing (I've not been following the main return types stuff) so I wouldn't be surprised if this has not had much usage. In which case I'm wary of stabilising without getting some experience of it in practice.

[jonhoo]

Yeah, I didn't even know this had landed on nightly yet? Super excited about the feature though!

[nikomatsakis]

I don't think it's had much experience. I agree with @scottmcm that #[should_panic] surprised me a bit -- so perhaps I was premature in moving to stabilize. (I'd welcome a PR to adjust that, though.)

I'm ok with waiting a bit, although it makes stabilizing the "main" case a bit harder, since we have to split the two. But I would encourage those who have doubts about the feature to experiment -- there really isn't much more to it, I don't think.

In other words: under this feature, there are really three (and a half) distinct possible outcomes:

• Test panics
• Test returns a "successful" result (libc::EXIT_SUCCESS)
• Test returns a "failed" result
  • This could be subdivided to depend on the specific return code

I'm personally not inclined to subdivide the final case: I feel like if you want to test for a specific return code, you ought to just add an assert into your test.

(One other reason for holding back on this feature: a major motivation -- perhaps the motivation -- was using it from within rustdoc, so that those examples can show idiomatic code -- I'm not sure if that works yet or what it takes to make it work.)

[nikomatsakis]

@rfcbot fcp cancel

I'm going to cancel the FCP. I added @scottmcm's concern to the header, as well as the desire to see a rustdoc example in practice. =)

It might however be hard to make #[should_panic] not cover the case of a failed return. Or, well, maybe not hard, but that will require deeper changes to libtest.

Thinking more about it, I'm not sure how desirable it is. I agree #[should_panic] is confusing but I wonder if it makes sense to introduce more than one way sense of unit test "failure". Is that making things overly complex? (We could rename #[should_panic])

[rfcbot]

@nikomatsakis proposal cancelled.

[shepmaster]

I agree #[should_panic] is confusing

I'd use stronger language than "confusing". If the only test framework we can use (and will forever be the default test framework) says that "returning a Result is the same as a panic", that's going to work against any kind of teaching that we have ever done that they are different methods of error handling with different use cases.

Tests are something that should check specific conditions. #[should_panic] is already fairly loose in that you can't use it to identify a specific line of code that should generate a panic:

#[test]
#[should_panic(expected = "index out of bounds")]
fn e() {
    let a = [1, 2, 100];
    let b = a[3]; // oops
    let c = t(&a, b);
}

fn t(a: &[usize], idx: usize) {
    a[idx];
}

It's my belief that should_panic was basically a workaround for the fact that we didn't have a lightweight, stable way of catching panics at Rust 1.0. In a magical world of test frameworks, I'd want such an assertion to be part of the test, not just metadata:

#[test]
fn e() {
    let a = [1, 2, 100];
    let b = a[3]; // oops
    assert_that(|| {
        let c = t(&a, b);
    }).panics()
        .with("index out of bounds")
}

IMO, returning a Result::Err from a test should always cause the test to fail. If it's expected that the operation fails, that should be part of the test:

assert!(f.is_err());

[nikomatsakis]

IMO, returning a Result::Err from a test should always cause the test to fail. If it's expected that the operation fails, that should be part of the test:

Hmm. Plausible. Maybe that's how we should refactor the test runner's internal workings, actually. That is, we could refactor #[should_panic] tests so that they catch the panic (and make the test runner consider any uncaught panic as a failure).

[nikomatsakis]

I've been thinking about this a bit. Here is one simple possible design:

• #[should_panic] can only be applied to tests with () return type and continues to have the same meaning.
• For non-should-panic tests, the return type T defines the error result using the Termination trait.

Under this version, if we ignore #[should_panic] then:

• A panic is always a failure.
• The "non-Ok" variant of the return is also a failure.
  • If you want that to be considered success, you should write assert!(foo.is_err());
• This seems nicely biased towards "all the code you see in the test executed without panic'ing or returning an error".

With #[should_panic], the test either returns () (fails) or panics (succeeds).

Thoughts?

[shepmaster]

all the code you see in the test executed without panic'ing or returning an error

I think this is the right mindset. I've always envisioned ? in tests to be a shorthand for what is currently an unwrap/expect: "theoretically this can fail, but it shouldn't in this test".

[scottmcm]

That sounds great, @nikomatsakis; it resolves all my concerns about #[should_panic].

And it allows the great "replace .unwrap() with ?" crusade to continue 🙂 I added unit tests as another place a "whatever, I just want to use ?" type would be nice (rust-lang/rfcs#2367).

[pnkfelix]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[nikomatsakis]

The final comment period is up, let's do this! There are general instructions on how to stabilize a feature given here:

https://forge.rust-lang.org/stabilization-guide.html

In this case, the feature name in question is termination_trait_test.

[Dylan-DPC-zz]

hi @nikomatsakis I can work on this

[Dylan-DPC-zz]

Since #51298 is merged, we can close this issue?

[frewsxcv]

Pretty sure this can be closed now that this is stabilized, thanks @Dylan-DPC!

[shepmaster]

I'm missing something. I went to use this, but what's all this junk about 1 and 0 that has nothing to do with my test? This is what we stabilized?

#[test]
fn example() -> Result<(), Box<dyn std::error::Error>> {
    Err(String::from("boom"))?;
    Ok(())
}

(Playground)

Output:

running 1 test
test example ... FAILED

failures:

---- example stdout ----
Error: StringError("boom")
thread 'example' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`', libtest/lib.rs:326:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    example

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

[ehuss]

@shepmaster I think there is a PR at #52453 to fix that.

[shepmaster]

Thanks. It's a shame this will go out in the suboptimal form in 1.28, and presumably 1.29, but hopefully we will see it in 1.30!

[nalply]

Very late to the party! Sorry!

@nikomatsakis wrote at the top:

If the type is not (), then #[should_panic] is disallowed

Why?

I wrote a test runner for a complicated test setup and tear-down use case using catch_unwind() and resume_unwind(). I would like to test where a test returns the Err case and check whether the test runner panics.

In other words, I wrote a test where I return Result together with #[should_panic]:

  #[test]
  #[should_panic]
  fn return_err() -> Result<()> {
    let _ = Test::init().run_silent(|_| display("TEST ERROR")?);
    Ok(())
  }

Meanwhile, I commented out this test because it does not compile. I have to admit that I am confused and probably what I am trying is based on faulty assumptions, however the question remains:

Why is #[should_panic] disallowed on non-unit returning tests?

[dralley]

@nalply I think the answer is #48854 (comment) but I'd love clarification on this also.