From c1137fee181afc6aa6af34a5f2ea28229676477f Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Thu, 3 Oct 2024 17:08:23 -0700 Subject: [PATCH] Redo integration test output (#633) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Much more coherent — shows output of inner tests consistently regardless of how they fail, with a nice prefix. Removes the clunky `assert_success` function. --- Cargo.lock | 11 +++ cargo-insta/Cargo.toml | 1 + cargo-insta/tests/main.rs | 190 +++++++++++++++++++++++--------------- 3 files changed, 129 insertions(+), 73 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 754c2389..2b7844aa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -75,6 +75,7 @@ dependencies = [ "insta", "itertools", "lazy_static", + "os_pipe", "proc-macro2", "semver", "serde", @@ -469,6 +470,16 @@ version = "1.14.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2f7254b99e31cad77da24b08ebf628882739a608578bb1bcdfc1f9c21260d7c0" +[[package]] +name = "os_pipe" +version = "0.9.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fb233f06c2307e1f5ce2ecad9f8121cffbbee2c95428f44ea85222e460d0d213" +dependencies = [ + "libc", + "winapi", +] + [[package]] name = "pest" version = "2.7.11" diff --git a/cargo-insta/Cargo.toml b/cargo-insta/Cargo.toml index 2258d64f..11dd43b4 100644 --- a/cargo-insta/Cargo.toml +++ b/cargo-insta/Cargo.toml @@ -37,3 +37,4 @@ walkdir = "2.3.1" similar= "2.2.1" itertools = "0.10.0" termcolor = "1.1.2" +os_pipe = "0.9.0" \ No newline at end of file diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index acce0dc4..0eb74a23 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1,38 +1,51 @@ /// Integration tests which allow creating a full repo, running `cargo-insta` /// and then checking the output. /// +/// By default, the output of the inner test is forwarded to the outer test with +/// a colored prefix. If we want to assert the inner test contains some output, +/// we need to disable that forwarding with `Stdio::piped()` like: +/// +/// ```rust +/// let output = test_project +/// .insta_cmd() +/// .args(["test"]) +/// .stderr(Stdio::piped()) +/// +/// assert!(String::from_utf8_lossy(&output.stderr).contains("info: 2 snapshots to review") +/// ``` +/// /// Often we want to see output from the test commands we run here; for example /// a `dbg!` statement we add while debugging. Cargo by default hides the output /// of passing tests. -/// - Like any test, to forward the output of an outer test (i.e. one of the -/// `#[test]`s in this file) to the terminal, pass `--nocapture` to the test -/// runner, like `cargo insta test -- --nocapture`. -/// - To forward the output of an inner test (i.e. the test commands we create -/// and run within an outer test) to the output of an outer test, pass +/// - Like any test, to forward the output of a passing outer test (i.e. one of +/// the `#[test]`s in this file) to the terminal, pass `--nocapture` to the +/// test runner, like `cargo insta test -- --nocapture`. +/// - To forward the output of a passing inner test (i.e. the test commands we +/// create and run within an outer test) to the output of an outer test, pass /// `--nocapture` in the command we create; for example `.args(["test", -/// "--accept", "--", "--nocapture"])`. We then also need to pass -/// `--nocapture` to the outer test to forward that to the terminal. -/// - If a test fails on `assert_success` or `assert_failure`, we print the -/// output of the inner test (bypassing the capturing). We color it to -/// discriminate it from the output of the outer test. +/// "--accept", "--", "--nocapture"])`. +/// - We also need to pass `--nocapture` to the outer test to forward that to +/// the terminal, per the previous bullet. /// -/// We can write more docs if that would be helpful. For the moment one thing to -/// be aware of: it seems the packages must have different names, or we'll see -/// interference between the tests. +/// Note that the packages must have different names, or we'll see interference +/// between the tests. /// /// > That seems to be because they all share the same `target` directory, which /// > cargo will confuse for each other if they share the same name. I haven't -/// > worked out why — this is the case even if the files are the same between two -/// > tests but with different commands — and those files exist in different -/// > temporary workspace dirs. (We could try to enforce different names, or give -/// > up using a consistent target directory for a cache, but it would slow down -/// > repeatedly running the tests locally. To demonstrate the effect, name crates -/// > the same... This also causes issues when running the same tests +/// > worked out why — this is the case even if the files are the same between +/// > two tests but with different commands — and those files exist in different +/// > temporary workspace dirs. (We could try to enforce different names, or +/// > give up using a consistent target directory for a cache, but it would slow +/// > down repeatedly running the tests locally. To demonstrate the effect, name +/// > crates the same... This also causes issues when running the same tests /// > concurrently. use std::collections::HashMap; use std::fs; +use std::io::{BufRead, BufReader}; use std::path::{Path, PathBuf}; use std::process::Command; +use std::process::Stdio; +use std::thread; use std::{env, fs::remove_dir_all}; use console::style; @@ -42,6 +55,33 @@ use itertools::Itertools; use similar::udiff::unified_diff; use tempfile::TempDir; +/// Wraps a formatting function to be used as a `Stdio` +struct OutputFormatter(F) +where + F: Fn(&str) -> String + Send + 'static; + +impl From> for Stdio +where + F: Fn(&str) -> String + Send + 'static, +{ + // Creates a pipe, spawns a thread to read from the pipe, applies the + // formatting function to each line, and prints the result. + fn from(output: OutputFormatter) -> Stdio { + let (read_end, write_end) = os_pipe::pipe().unwrap(); + + thread::spawn(move || { + let mut reader = BufReader::new(read_end); + let mut line = String::new(); + while reader.read_line(&mut line).unwrap() > 0 { + print!("{}", (output.0)(&line)); + line.clear(); + } + }); + + Stdio::from(write_end) + } +} + struct TestFiles { files: HashMap, } @@ -82,29 +122,6 @@ fn target_dir() -> PathBuf { target_dir } -fn assert_success(output: &std::process::Output) { - assert!( - output.status.success(), - "Tests failed: {}\n{}", - // Color the inner output so we can tell what's coming from there vs our own - // output - // Should we also do something like indent them? Or add a prefix? - format_args!("{}", style(String::from_utf8_lossy(&output.stdout)).green()), - format_args!("{}", style(String::from_utf8_lossy(&output.stderr)).red()) - ); -} - -fn assert_failure(output: &std::process::Output) { - assert!( - !output.status.success(), - "Tests unexpectedly succeeded: {}\n{}", - // Color the inner output so we can tell what's coming from there vs our own - // output - // Should we also do something like indent them? Or add a prefix? - format_args!("{}", style(String::from_utf8_lossy(&output.stdout)).green()), - format_args!("{}", style(String::from_utf8_lossy(&output.stderr)).red()) - ); -} struct TestProject { /// Temporary directory where the project is created workspace_dir: PathBuf, @@ -158,11 +175,29 @@ impl TestProject { command.current_dir(self.workspace_dir.as_path()); // Use the same target directory as other tests, consistent across test - // run. This makes the compilation much faster (though do some tests + // runs. This makes the compilation much faster (though do some tests // tread on the toes of others? We could have a different cache for each // project if so...) command.env("CARGO_TARGET_DIR", target_dir()); + let workspace_name = self + .workspace_dir + .file_name() + .unwrap() + .to_string_lossy() + .into_owned(); + + let stdout_name = workspace_name.clone(); + let stderr_name = workspace_name; + + command + .stdout(OutputFormatter(move |line| { + format!("{} {}", style(&stdout_name).green(), line) + })) + .stderr(OutputFormatter(move |line| { + format!("{} {}", style(&stderr_name).red(), line) + })); + command } @@ -262,7 +297,7 @@ fn test_json_snapshot() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.diff("src/main.rs"), @r##" --- Original: src/main.rs @@ -331,7 +366,7 @@ fn test_yaml_snapshot() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.diff("src/main.rs"), @r###" --- Original: src/main.rs @@ -406,7 +441,7 @@ fn test_trailing_comma_in_inline_snapshot() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.diff("src/main.rs"), @r##" --- Original: src/main.rs @@ -529,7 +564,7 @@ fn test_root_crate_workspace_accept() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.file_tree_diff(), @r###" --- Original file tree @@ -562,6 +597,7 @@ fn test_root_crate_workspace() { .insta_cmd() // Need to disable colors to assert the output below .args(["test", "--workspace", "--color=never"]) + .stderr(Stdio::piped()) .output() .unwrap(); @@ -585,7 +621,7 @@ fn test_root_crate_no_all() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.file_tree_diff(), @r###" --- Original file tree @@ -687,7 +723,7 @@ fn test_virtual_manifest_all() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.file_tree_diff(), @r###" --- Original file tree @@ -724,7 +760,7 @@ fn test_virtual_manifest_default() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.file_tree_diff(), @r###" --- Original file tree @@ -761,7 +797,7 @@ fn test_virtual_manifest_single_crate() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.file_tree_diff(), @r###" --- Original file tree @@ -824,7 +860,7 @@ fn test_old_yaml_format() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.diff("src/lib.rs"), @""); } @@ -889,7 +925,7 @@ Hello, world! .output() .unwrap(); - assert_success(&output_current); + assert!(&output_current.status.success()); // Test with insta 1.40.0 let output_1_40_0 = test_insta_1_40_0 @@ -898,7 +934,7 @@ Hello, world! .output() .unwrap(); - assert_success(&output_1_40_0); + assert!(&output_1_40_0.status.success()); // Check that both versions updated the snapshot correctly assert_snapshot!(test_current_insta.diff("src/snapshots/test_force_update_current__force_update.snap"), @r#" @@ -976,7 +1012,7 @@ fn test_linebreaks() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.diff("src/lib.rs"), @r#####" --- Original: src/lib.rs @@ -1035,7 +1071,7 @@ fn test_excessive_hashes() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); // TODO: we would like to update the number of hashes, but that's not easy // given the reasons at https://github.com/mitsuhiko/insta/pull/573. So this @@ -1083,7 +1119,7 @@ fn test_wrong_indent_force() { .args(["test", "--check", "--", "--nocapture"]) .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); // Then run the test with --force-update-snapshots and --accept to confirm // the new snapshot is written @@ -1098,7 +1134,7 @@ fn test_wrong_indent_force() { ]) .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); // https://github.com/mitsuhiko/insta/pull/563 will fix the starting & // ending newlines @@ -1156,7 +1192,7 @@ fn test_hashtag_escape() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); assert_snapshot!(test_project.diff("src/main.rs"), @r####" --- Original: src/main.rs @@ -1252,23 +1288,25 @@ fn test_insta_workspace_root() { .current_dir(&test_project.workspace_dir) .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); let test_binary_path = find_test_binary(&test_project.workspace_dir); // Run the test without snapshot (should fail) - assert_failure(&run_test_binary( - &test_binary_path, - &test_project.workspace_dir, - None, - )); + assert!( + !&run_test_binary(&test_binary_path, &test_project.workspace_dir, None,) + .status + .success() + ); // Create the snapshot - assert_success(&run_test_binary( + assert!(&run_test_binary( &test_binary_path, &test_project.workspace_dir, Some(("INSTA_UPDATE", "always")), - )); + ) + .status + .success()); // Verify snapshot creation assert!(test_project.workspace_dir.join("src/snapshots").exists()); @@ -1288,14 +1326,20 @@ fn test_insta_workspace_root() { let moved_binary_path = find_test_binary(&moved_workspace); // Run test in moved workspace without INSTA_WORKSPACE_ROOT (should fail) - assert_failure(&run_test_binary(&moved_binary_path, &moved_workspace, None)); + assert!( + !&run_test_binary(&moved_binary_path, &moved_workspace, None) + .status + .success() + ); // Run test in moved workspace with INSTA_WORKSPACE_ROOT (should pass) - assert_success(&run_test_binary( + assert!(&run_test_binary( &moved_binary_path, &moved_workspace, Some(("INSTA_WORKSPACE_ROOT", moved_workspace.to_str().unwrap())), - )); + ) + .status + .success()); } #[test] @@ -1352,7 +1396,7 @@ fn test_hello() { .output() .unwrap(); - assert_failure(&output); + assert!(!&output.status.success()); // Verify that the snapshot was created in the correct location assert_snapshot!(TestProject::current_file_tree(&test_project.workspace_dir), @r" @@ -1375,7 +1419,7 @@ fn test_hello() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); // Verify that the snapshot was created in the correct location assert_snapshot!(TestProject::current_file_tree(&test_project.workspace_dir), @r" @@ -1397,7 +1441,7 @@ fn test_hello() { .output() .unwrap(); - assert_success(&output); + assert!(&output.status.success()); let snapshot_path = test_project .workspace_dir