Fix should_panic on merged doctests

Author: GuillaumeGomez

library/test/src/lib.rs

@@ -45,7 +45,9 @@ pub mod test {
     pub use crate::cli::{TestOpts, parse_opts};
     pub use crate::helpers::metrics::{Metric, MetricMap};
     pub use crate::options::{Options, RunIgnored, RunStrategy, ShouldPanic};
-    pub use crate::test_result::{TestResult, TrFailed, TrFailedMsg, TrIgnored, TrOk};
+    pub use crate::test_result::{
+        RustdocResult, TestResult, TrFailed, TrFailedMsg, TrIgnored, TrOk, get_rustdoc_result,
+    };
     pub use crate::time::{TestExecTime, TestTimeOptions};
     pub use crate::types::{
         DynTestFn, DynTestName, StaticBenchFn, StaticTestFn, StaticTestName, TestDesc,

library/test/src/test_result.rs

@@ -1,7 +1,8 @@
 use std::any::Any;
 #[cfg(unix)]
 use std::os::unix::process::ExitStatusExt;
-use std::process::ExitStatus;
+use std::process::{ExitStatus, Output};
+use std::{fmt, io};
 
 pub use self::TestResult::*;
 use super::bench::BenchSamples;
@@ -103,15 +104,14 @@ pub(crate) fn calc_result(
     result
 }
 
-/// Creates a `TestResult` depending on the exit code of test subprocess.
-pub(crate) fn get_result_from_exit_code(
-    desc: &TestDesc,
+/// Creates a `TestResult` depending on the exit code of test subprocess
+pub(crate) fn get_result_from_exit_code_inner(
     status: ExitStatus,
-    time_opts: Option<&time::TestTimeOptions>,
-    exec_time: Option<&time::TestExecTime>,
+    success_error_code: i32,
 ) -> TestResult {
-    let result = match status.code() {
-        Some(TR_OK) => TestResult::TrOk,
+    match status.code() {
+        Some(error_code) if error_code == success_error_code => TestResult::TrOk,
+        Some(crate::ERROR_EXIT_CODE) => TestResult::TrFailed,
         #[cfg(windows)]
         Some(STATUS_FAIL_FAST_EXCEPTION) => TestResult::TrFailed,
         #[cfg(unix)]
@@ -131,7 +131,17 @@ pub(crate) fn get_result_from_exit_code(
         Some(code) => TestResult::TrFailedMsg(format!("got unexpected return code {code}")),
         #[cfg(not(any(windows, unix)))]
         Some(_) => TestResult::TrFailed,
-    };
+    }
+}
+
+/// Creates a `TestResult` depending on the exit code of test subprocess and on its runtime.
+pub(crate) fn get_result_from_exit_code(
+    desc: &TestDesc,
+    status: ExitStatus,
+    time_opts: Option<&time::TestTimeOptions>,
+    exec_time: Option<&time::TestExecTime>,
+) -> TestResult {
+    let result = get_result_from_exit_code_inner(status, TR_OK);
 
     // If test is already failed (or allowed to fail), do not change the result.
     if result != TestResult::TrOk {
@@ -147,3 +157,92 @@ pub(crate) fn get_result_from_exit_code(
 
     result
 }
+
+pub enum RustdocResult {
+    /// The test failed to compile.
+    CompileError,
+    /// The test is marked `compile_fail` but compiled successfully.
+    UnexpectedCompilePass,
+    /// The test failed to compile (as expected) but the compiler output did not contain all
+    /// expected error codes.
+    MissingErrorCodes(Vec<String>),
+    /// The test binary was unable to be executed.
+    ExecutionError(io::Error),
+    /// The test binary exited with a non-zero exit code.
+    ///
+    /// This typically means an assertion in the test failed or another form of panic occurred.
+    ExecutionFailure(Output),
+    /// The test is marked `should_panic` but the test binary executed successfully.
+    NoPanic(Option<String>),
+}
+
+impl fmt::Display for RustdocResult {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::CompileError => {
+                write!(f, "Couldn't compile the test.")
+            }
+            Self::UnexpectedCompilePass => {
+                write!(f, "Test compiled successfully, but it's marked `compile_fail`.")
+            }
+            Self::NoPanic(msg) => {
+                write!(f, "Test didn't panic, but it's marked `should_panic`")?;
+                if let Some(msg) = msg {
+                    write!(f, " ({msg})")?;
+                }
+                f.write_str(".")
+            }
+            Self::MissingErrorCodes(codes) => {
+                write!(f, "Some expected error codes were not found: {codes:?}")
+            }
+            Self::ExecutionError(err) => {
+                write!(f, "Couldn't run the test: {err}")?;
+                if err.kind() == io::ErrorKind::PermissionDenied {
+                    f.write_str(" - maybe your tempdir is mounted with noexec?")?;
+                }
+                Ok(())
+            }
+            Self::ExecutionFailure(out) => {
+                writeln!(f, "Test executable failed ({reason}).", reason = out.status)?;
+
+                // FIXME(#12309): An unfortunate side-effect of capturing the test
+                // executable's output is that the relative ordering between the test's
+                // stdout and stderr is lost. However, this is better than the
+                // alternative: if the test executable inherited the parent's I/O
+                // handles the output wouldn't be captured at all, even on success.
+                //
+                // The ordering could be preserved if the test process' stderr was
+                // redirected to stdout, but that functionality does not exist in the
+                // standard library, so it may not be portable enough.
+                let stdout = str::from_utf8(&out.stdout).unwrap_or_default();
+                let stderr = str::from_utf8(&out.stderr).unwrap_or_default();
+
+                if !stdout.is_empty() || !stderr.is_empty() {
+                    writeln!(f)?;
+
+                    if !stdout.is_empty() {
+                        writeln!(f, "stdout:\n{stdout}")?;
+                    }
+
+                    if !stderr.is_empty() {
+                        writeln!(f, "stderr:\n{stderr}")?;
+                    }
+                }
+                Ok(())
+            }
+        }
+    }
+}
+
+pub fn get_rustdoc_result(output: Output, should_panic: bool) -> Result<(), RustdocResult> {
+    let result = get_result_from_exit_code_inner(output.status, 0);
+    match (result, should_panic) {
+        (TestResult::TrFailed, true) | (TestResult::TrOk, false) => Ok(()),
+        (TestResult::TrOk, true) => Err(RustdocResult::NoPanic(None)),
+        (TestResult::TrFailedMsg(msg), true) => Err(RustdocResult::NoPanic(Some(msg))),
+        (TestResult::TrFailedMsg(_) | TestResult::TrFailed, false) => {
+            Err(RustdocResult::ExecutionFailure(output))
+        }
+        _ => unreachable!("unexpected status for rustdoc test output"),
+    }
+}

src/librustdoc/doctest.rs

@@ -30,6 +30,7 @@ use rustc_span::symbol::sym;
 use rustc_span::{FileName, Span};
 use rustc_target::spec::{Target, TargetTuple};
 use tempfile::{Builder as TempFileBuilder, TempDir};
+use test::test::{RustdocResult, get_rustdoc_result};
 use tracing::debug;
 
 use self::rust::HirCollector;
@@ -445,25 +446,6 @@ fn scrape_test_config(
     opts
 }
 
-/// Documentation test failure modes.
-enum TestFailure {
-    /// The test failed to compile.
-    CompileError,
-    /// The test is marked `compile_fail` but compiled successfully.
-    UnexpectedCompilePass,
-    /// The test failed to compile (as expected) but the compiler output did not contain all
-    /// expected error codes.
-    MissingErrorCodes(Vec<String>),
-    /// The test binary was unable to be executed.
-    ExecutionError(io::Error),
-    /// The test binary exited with a non-zero exit code.
-    ///
-    /// This typically means an assertion in the test failed or another form of panic occurred.
-    ExecutionFailure(process::Output),
-    /// The test is marked `should_panic` but the test binary executed successfully.
-    UnexpectedRunPass,
-}
-
 enum DirState {
     Temp(TempDir),
     Perm(PathBuf),
@@ -553,7 +535,7 @@ fn run_test(
     rustdoc_options: &RustdocOptions,
     supports_color: bool,
     report_unused_externs: impl Fn(UnusedExterns),
-) -> (Duration, Result<(), TestFailure>) {
+) -> (Duration, Result<(), RustdocResult>) {
     let langstr = &doctest.langstr;
     // Make sure we emit well-formed executable names for our target.
     let rust_out = add_exe_suffix("rust_out".to_owned(), &rustdoc_options.target);
@@ -642,7 +624,7 @@ fn run_test(
         if std::fs::write(&input_file, &doctest.full_test_code).is_err() {
             // If we cannot write this file for any reason, we leave. All combined tests will be
             // tested as standalone tests.
-            return (Duration::default(), Err(TestFailure::CompileError));
+            return (Duration::default(), Err(RustdocResult::CompileError));
         }
         if !rustdoc_options.no_capture {
             // If `no_capture` is disabled, then we don't display rustc's output when compiling
@@ -719,7 +701,7 @@ fn run_test(
         if std::fs::write(&runner_input_file, merged_test_code).is_err() {
             // If we cannot write this file for any reason, we leave. All combined tests will be
             // tested as standalone tests.
-            return (instant.elapsed(), Err(TestFailure::CompileError));
+            return (instant.elapsed(), Err(RustdocResult::CompileError));
         }
         if !rustdoc_options.no_capture {
             // If `no_capture` is disabled, then we don't display rustc's output when compiling
@@ -772,7 +754,7 @@ fn run_test(
     let _bomb = Bomb(&out);
     match (output.status.success(), langstr.compile_fail) {
         (true, true) => {
-            return (instant.elapsed(), Err(TestFailure::UnexpectedCompilePass));
+            return (instant.elapsed(), Err(RustdocResult::UnexpectedCompilePass));
         }
         (true, false) => {}
         (false, true) => {
@@ -788,12 +770,15 @@ fn run_test(
                     .collect();
 
                 if !missing_codes.is_empty() {
-                    return (instant.elapsed(), Err(TestFailure::MissingErrorCodes(missing_codes)));
+                    return (
+                        instant.elapsed(),
+                        Err(RustdocResult::MissingErrorCodes(missing_codes)),
+                    );
                 }
             }
         }
         (false, false) => {
-            return (instant.elapsed(), Err(TestFailure::CompileError));
+            return (instant.elapsed(), Err(RustdocResult::CompileError));
         }
     }
 
@@ -831,17 +816,9 @@ fn run_test(
         cmd.output()
     };
     match result {
-        Err(e) => return (duration, Err(TestFailure::ExecutionError(e))),
-        Ok(out) => {
-            if langstr.should_panic && out.status.success() {
-                return (duration, Err(TestFailure::UnexpectedRunPass));
-            } else if !langstr.should_panic && !out.status.success() {
-                return (duration, Err(TestFailure::ExecutionFailure(out)));
-            }
-        }
+        Err(e) => (duration, Err(RustdocResult::ExecutionError(e))),
+        Ok(output) => (duration, get_rustdoc_result(output, langstr.should_panic)),
     }
-
-    (duration, Ok(()))
 }
 
 /// Converts a path intended to use as a command to absolute if it is
@@ -1132,54 +1109,7 @@ fn doctest_run_fn(
         run_test(runnable_test, &rustdoc_options, doctest.supports_color, report_unused_externs);
 
     if let Err(err) = res {
-        match err {
-            TestFailure::CompileError => {
-                eprint!("Couldn't compile the test.");
-            }
-            TestFailure::UnexpectedCompilePass => {
-                eprint!("Test compiled successfully, but it's marked `compile_fail`.");
-            }
-            TestFailure::UnexpectedRunPass => {
-                eprint!("Test executable succeeded, but it's marked `should_panic`.");
-            }
-            TestFailure::MissingErrorCodes(codes) => {
-                eprint!("Some expected error codes were not found: {codes:?}");
-            }
-            TestFailure::ExecutionError(err) => {
-                eprint!("Couldn't run the test: {err}");
-                if err.kind() == io::ErrorKind::PermissionDenied {
-                    eprint!(" - maybe your tempdir is mounted with noexec?");
-                }
-            }
-            TestFailure::ExecutionFailure(out) => {
-                eprintln!("Test executable failed ({reason}).", reason = out.status);
-
-                // FIXME(#12309): An unfortunate side-effect of capturing the test
-                // executable's output is that the relative ordering between the test's
-                // stdout and stderr is lost. However, this is better than the
-                // alternative: if the test executable inherited the parent's I/O
-                // handles the output wouldn't be captured at all, even on success.
-                //
-                // The ordering could be preserved if the test process' stderr was
-                // redirected to stdout, but that functionality does not exist in the
-                // standard library, so it may not be portable enough.
-                let stdout = str::from_utf8(&out.stdout).unwrap_or_default();
-                let stderr = str::from_utf8(&out.stderr).unwrap_or_default();
-
-                if !stdout.is_empty() || !stderr.is_empty() {
-                    eprintln!();
-
-                    if !stdout.is_empty() {
-                        eprintln!("stdout:\n{stdout}");
-                    }
-
-                    if !stderr.is_empty() {
-                        eprintln!("stderr:\n{stderr}");
-                    }
-                }
-            }
-        }
-
+        eprint!("{err}");
         panic::resume_unwind(Box::new(()));
     }
     Ok(())