Rename #[should_fail] to #[should_panic] #21824

sfackler · 2015-01-31T23:39:23Z

No description provided.

rust-highfive · 2015-01-31T23:39:33Z

(rust_highfive has picked a reviewer for you, use r? to override)

steveklabnik · 2015-01-31T23:40:12Z

This is intentional. Tests pass or fail. That the test fails because of a panic is an implementation detail.

sfackler · 2015-02-01T00:00:13Z

Inflating "not panicing" with the test failing seems wrong, for exactly this case where you're expecting a panic for the test to succeed.

steveklabnik · 2015-02-01T00:03:13Z

Inflating "not panicing" with the test failing seems wrong,

Sorry, it's late. My point is just this: the panic aspect is implementation. Semanticly, tests pass or fail, they don't pass or panic. Hence should_fail.

sfackler · 2015-02-01T00:22:49Z

But #[should_fail]/#[should_panic] happens before the determination of a test succeeding or failing. It in fact defines the conditions under which a test succeeds or fails. If I write this:

#[test]
#[should_panic(expected = "out of bounds")]
fn test_oob() {
    vec![][10]
}

I'm saying "I expect a panic to occur when running this test, and the string associated with the panic to contain "out of bounds". The test should succeed if and only if that happens." The attribute is explicitly tied to the panic system. The equivalent in JUnit is @Test(expected=OutOfBoundsException.class) which also avoids the term "failure".

To me, #[should_fail] is saying "this test only succeeds if it fails", which which seems pretty paradoxical.

sanxiyn · 2015-02-01T01:05:53Z

I don't think should_fail is a good name because it may be confused with xfail mechanism in other testing frameworks. It is completely different from xfail in that the test is expected to pass, not fail.

steveklabnik · 2015-02-01T01:07:48Z

To me, #[should_fail] is saying "this test only succeeds if it fails", which which seems pretty paradoxical.

Only as paradoxial as !. It's just inverting the condition. You expect this test to fail, instead of pass.

sfackler · 2015-02-01T01:36:45Z

So I was under the impression that this was just a missed rename from the fail -> panic switch. If it's intentional, then this should probably move to an RFC.

steveklabnik · 2015-02-01T01:45:03Z

The fail -> panic RFC mentioned nothing about the attribute at all, and we had a momentary discussion of "should this attribute change too?" and didn't do it, basically for the reason I cited. So yeah, maybe this should sit on an RFC instead.

alexcrichton · 2015-02-01T04:06:13Z

I would personally agree with @sfackler here because we have precisely one method of determining whether a test has passed or failed, which is a panic. I agree with @steveklabnik that if we had multiple methods to fail a test then #[should_fail] should probably be the name of the attribute.

@steveklabnik, knowing that there's only one method of "failing a test", aka panicking, does that sway your thoughts one way or the other?

steveklabnik · 2015-02-01T04:09:08Z

Basically my thoughts are this:

should_panic describes an implementation detail, should_fail describes a semantic. We should name things after thier semantics, not their internals.
I don't really care that strongly to seriously argue about it if everyone else feels differently than I do.

sfackler · 2015-02-01T04:15:56Z

In my opinion, the semantic vs implementation detail descriptions are actually the other way around. If I have a function that documents that it panics if some condition happens, I want to tell the test framework to expect specifically a panic, maybe with some string attached. Keep in mind that assert! and assert_eq! are general purpose macros whose semantic purpose is to panic, as opposed to macros designed specifically for test cases like the static methods in JUnit's Assert class.

On the other hand, it may not necessarily be true in the future that panicking is the only way to cause a test to fail. Go's test infrastructure provides functions to indicate that the test will fail, without stopping execution (this is a bit like the difference between span_err and span_fatal in the compiler). It can be useful to have this variant if you're doing something like testing multiple inputs in one test functions, since you'll be able to see error messages for all of the inputs that broke instead of only the first.

bombless · 2015-02-01T04:17:45Z

More like

#[test]
#[should_pass]
fn test_oob() {
    assert_panic!("out of bounds", {        
        vec![][10]
    }); // if the block executed successfully, we panic
}

sfackler · 2015-02-01T04:19:02Z

@bombless panics can't be caught, so that test would end up panicking and therefore failing in all cases.

bombless · 2015-02-01T04:21:37Z

@sfackler Need some compiler magic for the assert_panic macro, I guess.

bombless · 2015-02-01T08:01:23Z

Ah, I see, the real problem is the lack of should_panic, we'll need it anyway.

pnkfelix · 2015-02-12T10:17:42Z

I'm inclined to agree with @alexcrichton that given that panic! (and wrappers around it like assert! and assert_eq!) are the way to signal failure, #[should_panic] makes sense.

(And it makes the tests easier to talk about: "This test failed because it is marked should_panic and did not panic", as opposed to "This test failed because it is marked should_fail and did not panic.")

(The only other way I can imagine a test failing is via a segmentation fault, but I assume that #[should_fail] test that segfaults is not supposed to be accepted by the test driver.)

Gankra · 2015-02-21T04:49:30Z

What's the status on this? Are we going for an RFC?

alexcrichton · 2015-02-24T22:00:08Z

We talked about this in today's meeting (minutes to be posted soon) and would like to move forward with this. r=me with a rebase, and thanks again @sfackler! (sorry about the delay)

sfackler · 2015-02-24T22:05:47Z

Cool, will update tonight.

@alexcrichton

This needs to make it into the snapshot before #21824 can land. r? @alexcrichton

huonw · 2015-03-02T10:30:08Z

@bors r=alexcrichton a84a

bors · 2015-03-02T11:08:47Z

⌛ Testing commit a84a5ea with merge f649c3b...

bors · 2015-03-02T11:13:26Z

💔 Test failed - auto-linux-64-nopt-t

Manishearth · 2015-03-02T12:46:32Z

/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libarena/lib.rs:324:1: 324:16 error: The attribute `should_panic` is currently unknown to the the compiler and may have meaning added to it in the future
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libarena/lib.rs:324 #[should_panic]
                                                                                        ^~~~~~~~~~~~~~~
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libarena/lib.rs:324:1: 324:16 help: add #![feature(custom_attribute)] to the crate attributes to enable
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libarena/lib.rs:324 #[should_panic]
                                                                                        ^~~~~~~~~~~~~~~
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libarena/lib.rs:339:1: 339:16 error: The attribute `should_panic` is currently unknown to the the compiler and may have meaning added to it in the future
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libarena/lib.rs:339 #[should_panic]
                                                                                        ^~~~~~~~~~~~~~~
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libarena/lib.rs:339:1: 339:16 help: add #![feature(custom_attribute)] to the crate attributes to enable
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libarena/lib.rs:339 #[should_panic]
                                                                                        ^~~~~~~~~~~~~~~

Manishearth · 2015-03-02T12:47:20Z

IIRC this did not make it into the snap. @flaper87 could you make another snapshot?

Needed so that rust-lang#21824 can land

Needed so that #21824 can land

Manishearth · 2015-03-09T08:34:05Z

@bors: retry

bors · 2015-03-09T09:29:52Z

⌛ Testing commit a84a5ea with merge 85d602f...

bors · 2015-03-09T10:36:14Z

💔 Test failed - auto-mac-32-opt

Manishearth · 2015-03-09T10:53:43Z

/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:333:30: 335:6 error: `arena` does not live long enough
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:333     let result = arena.alloc(|| Outer {
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:334         inner: arena.alloc(|| Inner { value: 10 })
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:335     });
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:329:42: 338:2 note: reference must be valid for the block suffix following statement 1 at 329:41...
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:329     struct Outer<'a> { inner: &'a Inner }
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:330 
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:331     let arena = Arena::new();
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:332 
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:333     let result = arena.alloc(|| Outer {
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:334         inner: arena.alloc(|| Inner { value: 10 })
                                                                                       ...
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:331:29: 338:2 note: ...but borrowed value is only valid for the block suffix following statement 2 at 331:28
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:331     let arena = Arena::new();
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:332 
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:333     let result = arena.alloc(|| Outer {
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:334         inner: arena.alloc(|| Inner { value: 10 })
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:335     });
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/libarena/lib.rs:336

Was that test perhaps removed?

pnkfelix · 2015-03-09T13:43:33Z

Was that test perhaps removed?

Yes, in commit 2c9d81b

sfackler · 2015-03-09T18:22:31Z

@bors r=alexcrichton e2605b4

bors · 2015-03-09T18:32:17Z

⌛ Testing commit e2605b4 with merge 638832e...

bors · 2015-03-09T21:02:49Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

@steveklabnik

Update documentation to reflect rust-lang#21824. r? @steveklabnik

rust-highfive assigned Gankra Jan 31, 2015

sfackler force-pushed the should_panic branch from d175e32 to a84a5ea Compare February 26, 2015 06:03

sfackler mentioned this pull request Feb 26, 2015

Whitelist #[should_panic] #22827

Merged

bors added a commit that referenced this pull request Feb 26, 2015

Auto merge of #22827 - sfackler:should_panic_pt1, r=alexcrichton

3a96d6a

This needs to make it into the snapshot before #21824 can land. r? @alexcrichton

huonw mentioned this pull request Mar 2, 2015

TRPL: Documentation #22549

Merged

This was referenced Mar 2, 2015

Do not error for unknown attributes at stage0 #22964

Closed

Register new snapshots (270a677) #23153

Merged

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 8, 2015

Rollup merge of rust-lang#23153 - Manishearth:snap, r=alexcrichton

f7c1fce

Needed so that rust-lang#21824 can land

bors added a commit that referenced this pull request Mar 9, 2015

Auto merge of #23153 - Manishearth:snap, r=alexcrichton

91bdf23

Needed so that #21824 can land

Rename #[should_fail] to #[should_panic]

e2605b4

sfackler force-pushed the should_panic branch from a84a5ea to e2605b4 Compare March 9, 2015 18:21

bors added a commit that referenced this pull request Mar 9, 2015

Auto merge of #21824 - sfackler:should_panic, r=alexcrichton

638832e

bors merged commit e2605b4 into rust-lang:master Mar 9, 2015

jooert mentioned this pull request Mar 18, 2015

Rename should_fail to should_panic in docs #23490

Closed

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 19, 2015

Rollup merge of rust-lang#23490 - jooert:master, r=steveklabnik

30718dd

Update documentation to reflect rust-lang#21824. r? @steveklabnik

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename #[should_fail] to #[should_panic] #21824

Rename #[should_fail] to #[should_panic] #21824

sfackler commented Jan 31, 2015

rust-highfive commented Jan 31, 2015

steveklabnik commented Jan 31, 2015

sfackler commented Feb 1, 2015

steveklabnik commented Feb 1, 2015

sfackler commented Feb 1, 2015

sanxiyn commented Feb 1, 2015

steveklabnik commented Feb 1, 2015

sfackler commented Feb 1, 2015

steveklabnik commented Feb 1, 2015

alexcrichton commented Feb 1, 2015

steveklabnik commented Feb 1, 2015

sfackler commented Feb 1, 2015

bombless commented Feb 1, 2015

sfackler commented Feb 1, 2015

bombless commented Feb 1, 2015

bombless commented Feb 1, 2015

pnkfelix commented Feb 12, 2015

Gankra commented Feb 21, 2015

alexcrichton commented Feb 24, 2015

sfackler commented Feb 24, 2015

huonw commented Mar 2, 2015

bors commented Mar 2, 2015

bors commented Mar 2, 2015

Manishearth commented Mar 2, 2015

Manishearth commented Mar 2, 2015

Manishearth commented Mar 9, 2015

bors commented Mar 9, 2015

bors commented Mar 9, 2015

Manishearth commented Mar 9, 2015

pnkfelix commented Mar 9, 2015

sfackler commented Mar 9, 2015

bors commented Mar 9, 2015

bors commented Mar 9, 2015

Rename #[should_fail] to #[should_panic] #21824

Rename #[should_fail] to #[should_panic] #21824

Conversation

sfackler commented Jan 31, 2015

rust-highfive commented Jan 31, 2015

steveklabnik commented Jan 31, 2015

sfackler commented Feb 1, 2015

steveklabnik commented Feb 1, 2015

sfackler commented Feb 1, 2015

sanxiyn commented Feb 1, 2015

steveklabnik commented Feb 1, 2015

sfackler commented Feb 1, 2015

steveklabnik commented Feb 1, 2015

alexcrichton commented Feb 1, 2015

steveklabnik commented Feb 1, 2015

sfackler commented Feb 1, 2015

bombless commented Feb 1, 2015

sfackler commented Feb 1, 2015

bombless commented Feb 1, 2015

bombless commented Feb 1, 2015

pnkfelix commented Feb 12, 2015

Gankra commented Feb 21, 2015

alexcrichton commented Feb 24, 2015

sfackler commented Feb 24, 2015

huonw commented Mar 2, 2015

bors commented Mar 2, 2015

bors commented Mar 2, 2015

Manishearth commented Mar 2, 2015

Manishearth commented Mar 2, 2015

Manishearth commented Mar 9, 2015

bors commented Mar 9, 2015

bors commented Mar 9, 2015

Manishearth commented Mar 9, 2015

pnkfelix commented Mar 9, 2015

sfackler commented Mar 9, 2015

bors commented Mar 9, 2015

bors commented Mar 9, 2015