Skip to content

Commit

Permalink
Auto merge of #14031 - epage:snap, r=weihanglo
Browse files Browse the repository at this point in the history
tests: Migrate alt_registry to snapbox

### What does this PR try to resolve?

The overall goal is to enable the use of snapshot testing on as many cargo tests as possible to reduce the burden when having to touch a lot of tests.  Towards that aim, this PR
- Adds snapshot testing to `cargo_test_support::Execs`
- Migrates `alt_registry` tests over as an example (and to vet it)

I've taken the approach of deprecating all other output assertions on `Execs` with `#[allow(deprecated)]` in every test file.  This let's us easily identity what files haven't been migrated, what in a file needs migration, and helps prevent a file from regressing.  This should make it easier to do a gradual migration that (as many people as they want) can chip in.  It comes at the cost of losing visibility into deprecated items we use.  Hopefully we won't be in this intermediate state for too long.

To reduce manual touch ups of snapshots, I've added some regex redactions.  My main concern with the `FILE_SIZE` redaction was when we test for specific sizes.  That shouldn't be a problem because we don't use `Execs::with_stderr` to test those but we capture the output and have a custom asserter for it.

### How should we test and review this PR?

Yes, this puts us in an intermediate state which isn't ideal but much better than one person trying to do all of this in a single branch / PR.

The main risk is that we'll hit a snag with snapbox being able to support our needs.  We got away with a lot because everything was custom, down to the diffing algorithm.  This is why I at least started with `alt_registry` to get a feel for what problems we might have.  There will likely be some we uncover.  I'm fairly confident that we can resolve them in some way,

### Additional information

This is a continuation of the work done in #13980.
  • Loading branch information
bors committed Jun 10, 2024
2 parents cc34b84 + e589ed7 commit a9ee3e8
Show file tree
Hide file tree
Showing 149 changed files with 1,235 additions and 565 deletions.
3 changes: 2 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ sha1 = "0.10.6"
sha2 = "0.10.8"
shell-escape = "0.1.5"
supports-hyperlinks = "3.0.0"
snapbox = { version = "0.6.5", features = ["diff", "dir", "term-svg", "regex"] }
snapbox = { version = "0.6.9", features = ["diff", "dir", "term-svg", "regex", "json"] }
tar = { version = "0.4.40", default-features = false }
tempfile = "3.10.1"
thiserror = "1.0.59"
Expand Down
2 changes: 1 addition & 1 deletion crates/cargo-test-support/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "cargo-test-support"
version = "0.2.1"
version = "0.2.2"
edition.workspace = true
rust-version = "1.78" # MSRV:1
license.workspace = true
Expand Down
20 changes: 20 additions & 0 deletions crates/cargo-test-support/src/compare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,16 @@ pub fn assert_ui() -> snapbox::Assert {
regex::Regex::new("Finished.*in (?<redacted>[0-9]+(\\.[0-9]+))s").unwrap(),
)
.unwrap();
subs.insert(
"[FILE_SIZE]",
regex::Regex::new("(?<redacted>[0-9]+(\\.[0-9]+)([a-zA-Z]i)?)B").unwrap(),
)
.unwrap();
subs.insert(
"[HASH]",
regex::Regex::new("home/\\.cargo/registry/src/-(?<redacted>[a-z0-9]+)").unwrap(),
)
.unwrap();
snapbox::Assert::new()
.action_env(snapbox::assert::DEFAULT_ACTION_ENV)
.redact_with(subs)
Expand Down Expand Up @@ -146,6 +156,16 @@ pub fn assert_e2e() -> snapbox::Assert {
regex::Regex::new("[FINISHED].*in (?<redacted>[0-9]+(\\.[0-9]+))s").unwrap(),
)
.unwrap();
subs.insert(
"[FILE_SIZE]",
regex::Regex::new("(?<redacted>[0-9]+(\\.[0-9]+)([a-zA-Z]i)?)B").unwrap(),
)
.unwrap();
subs.insert(
"[HASH]",
regex::Regex::new("home/\\.cargo/registry/src/-(?<redacted>[a-z0-9]+)").unwrap(),
)
.unwrap();
snapbox::Assert::new()
.action_env(snapbox::assert::DEFAULT_ACTION_ENV)
.redact_with(subs)
Expand Down
58 changes: 58 additions & 0 deletions crates/cargo-test-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use std::time::{self, Duration};

use anyhow::{bail, Result};
use cargo_util::{is_ci, ProcessBuilder, ProcessError};
use snapbox::IntoData as _;
use url::Url;

use self::paths::CargoPathExt;
Expand Down Expand Up @@ -534,6 +535,8 @@ pub struct Execs {
expect_stdin: Option<String>,
expect_stderr: Option<String>,
expect_exit_code: Option<i32>,
expect_stdout_data: Option<snapbox::Data>,
expect_stderr_data: Option<snapbox::Data>,
expect_stdout_contains: Vec<String>,
expect_stderr_contains: Vec<String>,
expect_stdout_contains_n: Vec<(String, usize)>,
Expand All @@ -545,6 +548,7 @@ pub struct Execs {
expect_json: Option<String>,
expect_json_contains_unordered: Option<String>,
stream_output: bool,
assert: snapbox::Assert,
}

impl Execs {
Expand All @@ -555,18 +559,36 @@ impl Execs {

/// Verifies that stdout is equal to the given lines.
/// See [`compare`] for supported patterns.
#[deprecated(note = "replaced with `Execs::with_stdout_data(expected)`")]
pub fn with_stdout<S: ToString>(&mut self, expected: S) -> &mut Self {
self.expect_stdout = Some(expected.to_string());
self
}

/// Verifies that stderr is equal to the given lines.
/// See [`compare`] for supported patterns.
#[deprecated(note = "replaced with `Execs::with_stderr_data(expected)`")]
pub fn with_stderr<S: ToString>(&mut self, expected: S) -> &mut Self {
self.expect_stderr = Some(expected.to_string());
self
}

/// Verifies that stdout is equal to the given lines.
///
/// See [`compare::assert_e2e`] for assertion details.
pub fn with_stdout_data(&mut self, expected: impl snapbox::IntoData) -> &mut Self {
self.expect_stdout_data = Some(expected.into_data());
self
}

/// Verifies that stderr is equal to the given lines.
///
/// See [`compare::assert_e2e`] for assertion details.
pub fn with_stderr_data(&mut self, expected: impl snapbox::IntoData) -> &mut Self {
self.expect_stderr_data = Some(expected.into_data());
self
}

/// Writes the given lines to stdin.
pub fn with_stdin<S: ToString>(&mut self, expected: S) -> &mut Self {
self.expect_stdin = Some(expected.to_string());
Expand All @@ -593,6 +615,7 @@ impl Execs {
/// its output.
///
/// See [`compare`] for supported patterns.
#[deprecated(note = "replaced with `Execs::with_stdout_data(expected)`")]
pub fn with_stdout_contains<S: ToString>(&mut self, expected: S) -> &mut Self {
self.expect_stdout_contains.push(expected.to_string());
self
Expand All @@ -602,6 +625,7 @@ impl Execs {
/// its output.
///
/// See [`compare`] for supported patterns.
#[deprecated(note = "replaced with `Execs::with_stderr_data(expected)`")]
pub fn with_stderr_contains<S: ToString>(&mut self, expected: S) -> &mut Self {
self.expect_stderr_contains.push(expected.to_string());
self
Expand All @@ -611,6 +635,7 @@ impl Execs {
/// its output, and should be repeated `number` times.
///
/// See [`compare`] for supported patterns.
#[deprecated(note = "replaced with `Execs::with_stdout_data(expected)`")]
pub fn with_stdout_contains_n<S: ToString>(&mut self, expected: S, number: usize) -> &mut Self {
self.expect_stdout_contains_n
.push((expected.to_string(), number));
Expand All @@ -622,6 +647,7 @@ impl Execs {
/// See [`compare`] for supported patterns.
///
/// See note on [`Self::with_stderr_does_not_contain`].
#[deprecated]
pub fn with_stdout_does_not_contain<S: ToString>(&mut self, expected: S) -> &mut Self {
self.expect_stdout_not_contains.push(expected.to_string());
self
Expand All @@ -636,6 +662,7 @@ impl Execs {
/// your test will pass without verifying the correct behavior. If
/// possible, write the test first so that it fails, and then implement
/// your fix/feature to make it pass.
#[deprecated]
pub fn with_stderr_does_not_contain<S: ToString>(&mut self, expected: S) -> &mut Self {
self.expect_stderr_not_contains.push(expected.to_string());
self
Expand All @@ -645,6 +672,7 @@ impl Execs {
/// ignoring the order of the lines.
///
/// See [`Execs::with_stderr_unordered`] for more details.
#[deprecated(note = "replaced with `Execs::with_stdout_data(expected.unordered())`")]
pub fn with_stdout_unordered<S: ToString>(&mut self, expected: S) -> &mut Self {
self.expect_stdout_unordered.push(expected.to_string());
self
Expand All @@ -671,6 +699,7 @@ impl Execs {
///
/// This will randomly fail if the other crate name is `bar`, and the
/// order changes.
#[deprecated(note = "replaced with `Execs::with_stderr_data(expected.unordered())`")]
pub fn with_stderr_unordered<S: ToString>(&mut self, expected: S) -> &mut Self {
self.expect_stderr_unordered.push(expected.to_string());
self
Expand Down Expand Up @@ -698,6 +727,7 @@ impl Execs {
///
/// Be careful writing the `without` fragments, see note in
/// `with_stderr_does_not_contain`.
#[deprecated]
pub fn with_stderr_line_without<S: ToString>(
&mut self,
with: &[S],
Expand Down Expand Up @@ -730,6 +760,7 @@ impl Execs {
/// - The order of arrays is ignored.
/// - Strings support patterns described in [`compare`].
/// - Use `"{...}"` to match any object.
#[deprecated(note = "replaced with `Execs::with_stdout_data(expected.json_lines())`")]
pub fn with_json(&mut self, expected: &str) -> &mut Self {
self.expect_json = Some(expected.to_string());
self
Expand All @@ -744,6 +775,7 @@ impl Execs {
/// what you are doing.
///
/// See `with_json` for more detail.
#[deprecated]
pub fn with_json_contains_unordered(&mut self, expected: &str) -> &mut Self {
match &mut self.expect_json_contains_unordered {
None => self.expect_json_contains_unordered = Some(expected.to_string()),
Expand Down Expand Up @@ -908,11 +940,14 @@ impl Execs {
}
}

#[track_caller]
fn verify_checks_output(&self, stdout: &[u8], stderr: &[u8]) {
if self.expect_exit_code.unwrap_or(0) != 0
&& self.expect_stdout.is_none()
&& self.expect_stdin.is_none()
&& self.expect_stderr.is_none()
&& self.expect_stdout_data.is_none()
&& self.expect_stderr_data.is_none()
&& self.expect_stdout_contains.is_empty()
&& self.expect_stderr_contains.is_empty()
&& self.expect_stdout_contains_n.is_empty()
Expand All @@ -934,6 +969,7 @@ impl Execs {
}
}

#[track_caller]
fn match_process(&self, process: &ProcessBuilder) -> Result<RawOutput> {
println!("running {}", process);
let res = if self.stream_output {
Expand Down Expand Up @@ -984,6 +1020,7 @@ impl Execs {
}
}

#[track_caller]
fn match_output(&self, code: Option<i32>, stdout: &[u8], stderr: &[u8]) -> Result<()> {
self.verify_checks_output(stdout, stderr);
let stdout = std::str::from_utf8(stdout).expect("stdout is not utf8");
Expand All @@ -1008,6 +1045,24 @@ impl Execs {
if let Some(expect_stderr) = &self.expect_stderr {
compare::match_exact(expect_stderr, stderr, "stderr", stdout, cwd)?;
}
if let Some(expect_stdout_data) = &self.expect_stdout_data {
if let Err(err) = self.assert.try_eq(
Some(&"stdout"),
stdout.into_data(),
expect_stdout_data.clone(),
) {
panic!("{err}")
}
}
if let Some(expect_stderr_data) = &self.expect_stderr_data {
if let Err(err) = self.assert.try_eq(
Some(&"stderr"),
stderr.into_data(),
expect_stderr_data.clone(),
) {
panic!("{err}")
}
}
for expect in self.expect_stdout_contains.iter() {
compare::match_contains(expect, stdout, cwd)?;
}
Expand Down Expand Up @@ -1060,6 +1115,8 @@ pub fn execs() -> Execs {
expect_stderr: None,
expect_stdin: None,
expect_exit_code: Some(0),
expect_stdout_data: None,
expect_stderr_data: None,
expect_stdout_contains: Vec::new(),
expect_stderr_contains: Vec::new(),
expect_stdout_contains_n: Vec::new(),
Expand All @@ -1071,6 +1128,7 @@ pub fn execs() -> Execs {
expect_json: None,
expect_json_contains_unordered: None,
stream_output: false,
assert: compare::assert_e2e(),
}
}

Expand Down
1 change: 1 addition & 0 deletions tests/build-std/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
//! Otherwise the tests are skipped.

#![allow(clippy::disallowed_methods)]
#![allow(deprecated)]

use cargo_test_support::*;
use std::env;
Expand Down

0 comments on commit a9ee3e8

Please sign in to comment.