From cfd19f612b1480ffa8cf383e04403d11767d5e24 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 16 Jan 2024 11:07:06 -0800 Subject: [PATCH] Make errors from `make_test_list` legible (#1228) In debugging I found that invocations like `~/.rustup/toolchains/beta-x86_64-unknown-linux-gnu/bin/cargo nextest run` cause some test failures in the nextest tree, for reasons I'm still working to understand. In `main`, those errors are printed like this, with a pile of ASCII byte values: ``` thread 'basic::test_termination' panicked at nextest-runner/tests/integration/fixtures.rs:383:10: test list successfully created: CommandFail { binary_id: RustBinaryId("cdylib-example"), command: ["/home/mbp/src/nextest/fixtures/nextest-tests/target/debug/deps/cdylib_example-9222c584dde9dbd0", "--list", "--format", "terse"], exit_status: ExitStatus(unix_wait_status(32512)), stdout: [], stderr: [47, 104, 111, 109, 101, 47, 109, 98, 112, 47, 115, 114, 99, 47, 110, 101, 120, 116, 101, 115, 116, 47, 102, 105, 120, 116, 117, 114, 101, 115, 47, 110, 101, 120, 116, 101, 115, 116, 45, 116, 101, 115, 116, 115, 47, 116, 97, 114, 103, 101, 116, 47, 100, 101, 98, 117, 103, 47, 100, 101, 112, 115, 47, 99, 100, 121, 108, 105, 98, 95, 101, 120, 97, 109, 112, 108, 101, 45, 57, 50, 50, 50, 99, 53, 56, 52, 100, 100, 101, 57, 100, 98, 100, 48, 58, 32, 101, 114, 114, 111, 114, 32, 119, 104, 105, 108, 101, 32, 108, 111, 97, 100, 105, 110, 103, 32, 115, 104, 97, 114, 101, 100, 32, 108, 105, 98, 114, 97, 114, 105, 101, 115, 58, 32, 108, 105, 98, 116, 101, 115, 116, 45, 99, 98, 97, 98, 55, 49, 54, 57, 55, 100, 102, 57, 55, 98, 48, 48, 46, 115, 111, 58, 32, 99, 97, 110, 110, 111, 116, 32, 111, 112, 101, 110, 32, 115, 104, 97, 114, 101, 100, 32, 111, 98, 106, 101, 99, 116, 32, 102, 105, 108, 101, 58, 32, 78, 111, 32, 115, 117, 99, 104, 32, 102, 105, 108, 101, 32, 111, 114, 32, 100, 105, 114, 101, 99, 116, 111, 114, 121, 10] } note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` This is because `unwrap` prints the `Debug` form and the error holds a `Vec`. There could be a few ways to change this but it seemed like the intention of the existing code was to keep the exact output in the error enum as bytes, and only do lossy conversion to UTF-8 from Display. So, I looked for a way to get the Display form printed, and it seemed like a good way was to just return the error, since the tests that call this already themselves return Results. With this applied the failures are much more helpful: ``` error: creating test list failed Caused by: for `cdylib-example`, command `/tmp/nextest-fixtureQWK72c/src/target/debug/deps/cdylib_example-9222c584dde9dbd0 --list --format terse` exited with code 127 --- stdout: --- stderr: /tmp/nextest-fixtureQWK72c/src/target/debug/deps/cdylib_example-9222c584dde9dbd0: error while loading shared libraries: libtest-cbab71697df97b00.so: cannot open shared object file: No such file or directory --- ``` --- nextest-runner/src/errors.rs | 4 ++-- nextest-runner/tests/integration/basic.rs | 16 ++++++++-------- nextest-runner/tests/integration/fixtures.rs | 5 +++-- .../tests/integration/target_runner.rs | 6 +++--- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/nextest-runner/src/errors.rs b/nextest-runner/src/errors.rs index c7e30427616..0c555ffb5b0 100644 --- a/nextest-runner/src/errors.rs +++ b/nextest-runner/src/errors.rs @@ -454,11 +454,11 @@ impl fmt::Display for PartitionerBuilderParseError { } } -/// An error that occures while operating on a +/// An error that occurs while operating on a /// [`TestFilterBuilder`](crate::test_filter::TestFilterBuilder). #[derive(Clone, Debug, Error)] pub enum TestFilterBuilderError { - /// An error that occured while constructing test filters. + /// An error that occurred while constructing test filters. #[error("error constructing test filters")] Construct { /// The underlying error. diff --git a/nextest-runner/tests/integration/basic.rs b/nextest-runner/tests/integration/basic.rs index d5ccac0ad6c..45d74be4946 100644 --- a/nextest-runner/tests/integration/basic.rs +++ b/nextest-runner/tests/integration/basic.rs @@ -51,7 +51,7 @@ fn test_list_tests() -> Result<()> { set_env_vars(); let test_filter = TestFilterBuilder::any(RunIgnored::Default); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty()); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty())?; let mut summary = test_list.to_summary(); for (name, expected) in &*EXPECTED_TESTS { @@ -90,7 +90,7 @@ fn test_run() -> Result<()> { set_env_vars(); let test_filter = TestFilterBuilder::any(RunIgnored::Default); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty()); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty())?; let config = load_config(); let profile = config .profile(NextestConfig::DEFAULT_PROFILE) @@ -198,7 +198,7 @@ fn test_run_ignored() -> Result<()> { vec![expr], ) .unwrap(); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty()); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty())?; let config = load_config(); let profile = config .profile(NextestConfig::DEFAULT_PROFILE) @@ -275,7 +275,7 @@ fn test_filter_expr_with_string_filters() -> Result<()> { vec![expr], ) .unwrap(); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty()); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty())?; for test in test_list.iter_tests() { if test.name == "tests::call_dylib_add_two" { assert!( @@ -334,7 +334,7 @@ fn test_filter_expr_without_string_filters() -> Result<()> { let test_filter = TestFilterBuilder::new(RunIgnored::Default, None, Vec::::new(), vec![expr]) .unwrap(); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty()); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty())?; for test in test_list.iter_tests() { if test.name.contains("test_multiply_two") || test.name == "tests::call_dylib_add_two" { assert!( @@ -363,7 +363,7 @@ fn test_string_filters_without_filter_expr() -> Result<()> { vec![], ) .unwrap(); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty()); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty())?; for test in test_list.iter_tests() { if test.name.contains("test_multiply_two") || test.name.contains("tests::call_dylib_add_two") @@ -395,7 +395,7 @@ fn test_retries(retries: Option) -> Result<()> { set_env_vars(); let test_filter = TestFilterBuilder::any(RunIgnored::Default); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty()); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty())?; let config = load_config(); let profile = config .profile("with-retries") @@ -544,7 +544,7 @@ fn test_termination() -> Result<()> { ) .unwrap(); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty()); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty())?; let config = load_config(); let profile = config .profile("with-termination") diff --git a/nextest-runner/tests/integration/fixtures.rs b/nextest-runner/tests/integration/fixtures.rs index 84e6c2e40c2..76ec4032b77 100644 --- a/nextest-runner/tests/integration/fixtures.rs +++ b/nextest-runner/tests/integration/fixtures.rs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 use camino::{Utf8Path, Utf8PathBuf}; +use color_eyre::eyre::{Context, Result}; use duct::cmd; use guppy::{graph::PackageGraph, MetadataCommand}; use maplit::{btreemap, btreeset}; @@ -363,7 +364,7 @@ impl FixtureTargets { &self, test_filter: &TestFilterBuilder, target_runner: &TargetRunner, - ) -> TestList<'_> { + ) -> Result> { let test_bins: Vec<_> = self.test_artifacts.values().cloned().collect(); let double_spawn = DoubleSpawnInfo::disabled(); let ctx = TestExecuteContext { @@ -380,7 +381,7 @@ impl FixtureTargets { self.env.to_owned(), get_num_cpus(), ) - .expect("test list successfully created") + .context("Failed to make test list") } } diff --git a/nextest-runner/tests/integration/target_runner.rs b/nextest-runner/tests/integration/target_runner.rs index 3eba81200c8..9cd0c01eff2 100644 --- a/nextest-runner/tests/integration/target_runner.rs +++ b/nextest-runner/tests/integration/target_runner.rs @@ -162,7 +162,7 @@ fn test_listing_with_target_runner() -> Result<()> { set_env_vars(); let test_filter = TestFilterBuilder::any(RunIgnored::Default); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty()); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty())?; let bin_count = test_list.binary_count(); let test_count = test_list.test_count(); @@ -174,7 +174,7 @@ fn test_listing_with_target_runner() -> Result<()> { ); let (_, target_runner) = runner_for_target(None).unwrap(); - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &target_runner); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &target_runner)?; assert_eq!(bin_count, test_list.binary_count()); assert_eq!(test_count, test_list.test_count()); @@ -207,7 +207,7 @@ fn test_run_with_target_runner() -> Result<()> { assert_eq!(passthrough_path(), runner.binary()); } - let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &target_runner); + let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &target_runner)?; let config = load_config(); let profile = config