auto merge of #19560 : sfackler/rust/should-fail-reason, r=alexcrichton

The test harness will make sure that the panic message contains the
specified string. This is useful to help make `#[should_fail]` tests a
bit less brittle by decreasing the chance that the test isn't
"accidentally" passing due to a panic occurring earlier than expected.
The behavior is in some ways similar to JUnit's `expected` feature:
`@Test(expected=NullPointerException.class)`.

Without the message assertion, this test would pass even though it's not
actually reaching the intended part of the code:
```rust
#[test]
#[should_fail(message = "out of bounds")]
fn test_oob_array_access() {
    let idx: uint = from_str("13o").unwrap(); // oops, this will panic
    [1i32, 2, 3][idx];
}
```

Author: bors

src/compiletest/compiletest.rs

@@ -346,7 +346,7 @@ pub fn make_test(config: &Config, testfile: &Path, f: || -> test::TestFn)
         desc: test::TestDesc {
             name: make_test_name(config, testfile),
             ignore: header::is_test_ignored(config, testfile),
-            should_fail: false
+            should_fail: test::ShouldFail::No,
         },
         testfn: f(),
     }

src/doc/guide-testing.md

@@ -89,6 +89,21 @@ fn test_out_of_bounds_failure() {
 }
 ~~~
 
+`#[should_fail]` tests can be fragile as it's hard to guarantee that the test
+didn't fail for an unexpected reason. To help with this, an optional `expected`
+parameter can be added to the `should_fail` attribute. The test harness will
+make sure that the failure message contains the provided text. A safer version
+of the example above would be:
+
+~~~test_harness
+#[test]
+#[should_fail(expected = "index out of bounds")]
+fn test_out_of_bounds_failure() {
+    let v: &[int] = &[];
+    v[0];
+}
+~~~
+
 A test runner built with the `--test` flag supports a limited set of
 arguments to control which tests are run:
 

src/librustdoc/test.rs

@@ -280,7 +280,7 @@ impl Collector {
             desc: testing::TestDesc {
                 name: testing::DynTestName(name),
                 ignore: should_ignore,
-                should_fail: false, // compiler failures are test failures
+                should_fail: testing::ShouldFail::No, // compiler failures are test failures
             },
             testfn: testing::DynTestFn(proc() {
                 runtest(test.as_slice(),

src/libsyntax/test.rs

@@ -37,12 +37,17 @@ use {ast, ast_util};
 use ptr::P;
 use util::small_vector::SmallVector;
 
+enum ShouldFail {
+    No,
+    Yes(Option<InternedString>),
+}
+
 struct Test {
     span: Span,
     path: Vec<ast::Ident> ,
     bench: bool,
     ignore: bool,
-    should_fail: bool
+    should_fail: ShouldFail
 }
 
 struct TestCtxt<'a> {
@@ -360,8 +365,16 @@ fn is_ignored(i: &ast::Item) -> bool {
     i.attrs.iter().any(|attr| attr.check_name("ignore"))
 }
 
-fn should_fail(i: &ast::Item) -> bool {
-    attr::contains_name(i.attrs.as_slice(), "should_fail")
+fn should_fail(i: &ast::Item) -> ShouldFail {
+    match i.attrs.iter().find(|attr| attr.check_name("should_fail")) {
+        Some(attr) => {
+            let msg = attr.meta_item_list()
+                .and_then(|list| list.iter().find(|mi| mi.check_name("expected")))
+                .and_then(|mi| mi.value_str());
+            ShouldFail::Yes(msg)
+        }
+        None => ShouldFail::No,
+    }
 }
 
 /*
@@ -550,7 +563,20 @@ fn mk_test_desc_and_fn_rec(cx: &TestCtxt, test: &Test) -> P<ast::Expr> {
                                   vec![name_expr]);
 
     let ignore_expr = ecx.expr_bool(span, test.ignore);
-    let fail_expr = ecx.expr_bool(span, test.should_fail);
+    let should_fail_path = |name| {
+        ecx.path(span, vec![self_id, test_id, ecx.ident_of("ShouldFail"), ecx.ident_of(name)])
+    };
+    let fail_expr = match test.should_fail {
+        ShouldFail::No => ecx.expr_path(should_fail_path("No")),
+        ShouldFail::Yes(ref msg) => {
+            let path = should_fail_path("Yes");
+            let arg = match *msg {
+                Some(ref msg) => ecx.expr_some(span, ecx.expr_str(span, msg.clone())),
+                None => ecx.expr_none(span),
+            };
+            ecx.expr_call(span, ecx.expr_path(path), vec![arg])
+        }
+    };
 
     // self::test::TestDesc { ... }
     let desc_expr = ecx.expr_struct(

src/libtest/lib.rs

@@ -47,6 +47,7 @@ use self::TestEvent::*;
 use self::NamePadding::*;
 use self::OutputLocation::*;
 
+use std::any::{Any, AnyRefExt};
 use std::collections::TreeMap;
 use stats::Stats;
 use getopts::{OptGroup, optflag, optopt};
@@ -78,7 +79,7 @@ pub mod test {
              MetricChange, Improvement, Regression, LikelyNoise,
              StaticTestFn, StaticTestName, DynTestName, DynTestFn,
              run_test, test_main, test_main_static, filter_tests,
-             parse_opts, StaticBenchFn};
+             parse_opts, StaticBenchFn, ShouldFail};
 }
 
 pub mod stats;
@@ -184,13 +185,19 @@ pub struct Bencher {
     pub bytes: u64,
 }
 
+#[deriving(Clone, Show, PartialEq, Eq, Hash)]
+pub enum ShouldFail {
+    No,
+    Yes(Option<&'static str>)
+}
+
 // The definition of a single test. A test runner will run a list of
 // these.
 #[deriving(Clone, Show, PartialEq, Eq, Hash)]
 pub struct TestDesc {
     pub name: TestName,
     pub ignore: bool,
-    pub should_fail: bool,
+    pub should_fail: ShouldFail,
 }
 
 #[deriving(Show)]
@@ -346,7 +353,7 @@ fn optgroups() -> Vec<getopts::OptGroup> {
 
 fn usage(binary: &str) {
     let message = format!("Usage: {} [OPTIONS] [FILTER]", binary);
-    println!(r"{usage}
+    println!(r#"{usage}
 
 The FILTER regex is tested against the name of all tests to run, and
 only those tests that match are run.
@@ -366,10 +373,12 @@ Test Attributes:
                      function takes one argument (test::Bencher).
     #[should_fail] - This function (also labeled with #[test]) will only pass if
                      the code causes a failure (an assertion failure or panic!)
+                     A message may be provided, which the failure string must
+                     contain: #[should_fail(expected = "foo")].
     #[ignore]      - When applied to a function which is already attributed as a
                      test, then the test runner will ignore these tests during
                      normal test runs. Running with --ignored will run these
-                     tests.",
+                     tests."#,
              usage = getopts::usage(message.as_slice(),
                                     optgroups().as_slice()));
 }
@@ -902,13 +911,13 @@ fn should_sort_failures_before_printing_them() {
     let test_a = TestDesc {
         name: StaticTestName("a"),
         ignore: false,
-        should_fail: false
+        should_fail: ShouldFail::No
     };
 
     let test_b = TestDesc {
         name: StaticTestName("b"),
         ignore: false,
-        should_fail: false
+        should_fail: ShouldFail::No
     };
 
     let mut st = ConsoleTestState {
@@ -1114,7 +1123,7 @@ pub fn run_test(opts: &TestOpts,
 
             let stdout = reader.read_to_end().unwrap().into_iter().collect();
             let task_result = result_future.into_inner();
-            let test_result = calc_result(&desc, task_result.is_ok());
+            let test_result = calc_result(&desc, task_result);
             monitor_ch.send((desc.clone(), test_result, stdout));
         })
     }
@@ -1148,13 +1157,17 @@ pub fn run_test(opts: &TestOpts,
     }
 }
 
-fn calc_result(desc: &TestDesc, task_succeeded: bool) -> TestResult {
-    if task_succeeded {
-        if desc.should_fail { TrFailed }
-        else { TrOk }
-    } else {
-        if desc.should_fail { TrOk }
-        else { TrFailed }
+fn calc_result(desc: &TestDesc, task_result: Result<(), Box<Any+Send>>) -> TestResult {
+    match (&desc.should_fail, task_result) {
+        (&ShouldFail::No, Ok(())) |
+        (&ShouldFail::Yes(None), Err(_)) => TrOk,
+        (&ShouldFail::Yes(Some(msg)), Err(ref err))
+            if err.downcast_ref::<String>()
+                .map(|e| &**e)
+                .or_else(|| err.downcast_ref::<&'static str>().map(|e| *e))
+                .map(|e| e.contains(msg))
+                .unwrap_or(false) => TrOk,
+        _ => TrFailed,
     }
 }
 
@@ -1437,7 +1450,7 @@ mod tests {
                TestDesc, TestDescAndFn, TestOpts, run_test,
                Metric, MetricMap, MetricAdded, MetricRemoved,
                Improvement, Regression, LikelyNoise,
-               StaticTestName, DynTestName, DynTestFn};
+               StaticTestName, DynTestName, DynTestFn, ShouldFail};
     use std::io::TempDir;
 
     #[test]
@@ -1447,7 +1460,7 @@ mod tests {
             desc: TestDesc {
                 name: StaticTestName("whatever"),
                 ignore: true,
-                should_fail: false
+                should_fail: ShouldFail::No,
             },
             testfn: DynTestFn(proc() f()),
         };
@@ -1464,7 +1477,7 @@ mod tests {
             desc: TestDesc {
                 name: StaticTestName("whatever"),
                 ignore: true,
-                should_fail: false
+                should_fail: ShouldFail::No,
             },
             testfn: DynTestFn(proc() f()),
         };
@@ -1481,7 +1494,24 @@ mod tests {
             desc: TestDesc {
                 name: StaticTestName("whatever"),
                 ignore: false,
-                should_fail: true
+                should_fail: ShouldFail::Yes(None)
+            },
+            testfn: DynTestFn(proc() f()),
+        };
+        let (tx, rx) = channel();
+        run_test(&TestOpts::new(), false, desc, tx);
+        let (_, res, _) = rx.recv();
+        assert!(res == TrOk);
+    }
+
+    #[test]
+    fn test_should_fail_good_message() {
+        fn f() { panic!("an error message"); }
+        let desc = TestDescAndFn {
+            desc: TestDesc {
+                name: StaticTestName("whatever"),
+                ignore: false,
+                should_fail: ShouldFail::Yes(Some("error message"))
             },
             testfn: DynTestFn(proc() f()),
         };
@@ -1491,14 +1521,31 @@ mod tests {
         assert!(res == TrOk);
     }
 
+    #[test]
+    fn test_should_fail_bad_message() {
+        fn f() { panic!("an error message"); }
+        let desc = TestDescAndFn {
+            desc: TestDesc {
+                name: StaticTestName("whatever"),
+                ignore: false,
+                should_fail: ShouldFail::Yes(Some("foobar"))
+            },
+            testfn: DynTestFn(proc() f()),
+        };
+        let (tx, rx) = channel();
+        run_test(&TestOpts::new(), false, desc, tx);
+        let (_, res, _) = rx.recv();
+        assert!(res == TrFailed);
+    }
+
     #[test]
     fn test_should_fail_but_succeeds() {
         fn f() { }
         let desc = TestDescAndFn {
             desc: TestDesc {
                 name: StaticTestName("whatever"),
                 ignore: false,
-                should_fail: true
+                should_fail: ShouldFail::Yes(None)
             },
             testfn: DynTestFn(proc() f()),
         };
@@ -1544,15 +1591,15 @@ mod tests {
                 desc: TestDesc {
                     name: StaticTestName("1"),
                     ignore: true,
-                    should_fail: false,
+                    should_fail: ShouldFail::No,
                 },
                 testfn: DynTestFn(proc() {}),
             },
             TestDescAndFn {
                 desc: TestDesc {
                     name: StaticTestName("2"),
                     ignore: false,
-                    should_fail: false
+                    should_fail: ShouldFail::No,
                 },
                 testfn: DynTestFn(proc() {}),
             });
@@ -1588,7 +1635,7 @@ mod tests {
                     desc: TestDesc {
                         name: DynTestName((*name).clone()),
                         ignore: false,
-                        should_fail: false
+                        should_fail: ShouldFail::No,
                     },
                     testfn: DynTestFn(testfn),
                 };
@@ -1629,7 +1676,7 @@ mod tests {
                 desc: TestDesc {
                     name: DynTestName(name.to_string()),
                     ignore: false,
-                    should_fail: false
+                    should_fail: ShouldFail::No,
                 },
                 testfn: DynTestFn(test_fn)
             }

src/test/run-fail/test-should-fail-bad-message.rs

@@ -0,0 +1,22 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// check-stdout
+// error-pattern:task 'test_foo' panicked at
+// compile-flags: --test
+// ignore-pretty: does not work well with `--test`
+
+#[test]
+#[should_fail(expected = "foobar")]
+fn test_foo() {
+    panic!("blah")
+}
+
+

src/test/run-pass/test-should-fail-good-message.rs

@@ -0,0 +1,26 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// compile-flags: --test
+// ignore-pretty: does not work well with `--test`
+
+#[test]
+#[should_fail(expected = "foo")]
+fn test_foo() {
+    panic!("foo bar")
+}
+
+#[test]
+#[should_fail(expected = "foo")]
+fn test_foo_dynamic() {
+    panic!("{} bar", "foo")
+}
+
+