Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Transition direct assertions from cargo-test-support to snapbox #13980

Merged
merged 8 commits into from
Jun 2, 2024
Merged

refactor: Transition direct assertions from cargo-test-support to snapbox #13980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
995c2b7
chore: Upgrade to snapbox 0.6.7
epage May 29, 2024
f22c43b
fix(test)!: Hide API that isnt used by Cargo
epage May 24, 2024
94119af
docs(test): Clarify role of assert_ui
epage May 24, 2024
ea36ed0
refactor(test): Share redactions between ui and e2e
epage May 24, 2024
fe5c2d3
refactor(test): Pull out end-to-end literal redactions
epage May 24, 2024
3054936
refactor: Port from assert_matches_exact to assert_e2e
epage May 24, 2024
eee1053
refactor: Port from matches_unordered to assert_e2e
epage May 25, 2024
995746b
refactor: Port from matches_contains to assert_e2e
epage May 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.lock
View file
Open in desktop

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
View file
Open in desktop
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.7", features = ["diff", "dir", "term-svg", "regex"] }
tar = { version = "0.4.40", default-features = false }
tempfile = "3.10.1"
thiserror = "1.0.59"
Expand Down
213 changes: 142 additions & 71 deletions crates/cargo-test-support/src/compare.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,14 @@ use crate::diff;
use crate::paths;
use anyhow::{bail, Context, Result};
use serde_json::Value;
use std::env;
use std::fmt;
use std::path::Path;
use std::str;
use url::Url;

/// Default `snapbox` Assertions
/// Assertion policy for UI tests
///
/// This emphasizes showing as much content as possible at the cost of more brittleness
///
/// # Snapshots
///
Expand Down Expand Up @@ -82,7 +83,7 @@ pub fn assert_ui() -> snapbox::Assert {
let root_url = url::Url::from_file_path(&root).unwrap().to_string();

let mut subs = snapbox::Redactions::new();
subs.extend([("[EXE]", std::env::consts::EXE_SUFFIX)])
subs.extend(MIN_LITERAL_REDACTIONS.into_iter().cloned())
.unwrap();
subs.insert("[ROOT]", root).unwrap();
subs.insert("[ROOTURL]", root_url).unwrap();
Expand All @@ -96,6 +97,121 @@ pub fn assert_ui() -> snapbox::Assert {
.redact_with(subs)
}

/// Assertion policy for functional end-to-end tests
///
/// This emphasizes showing as much content as possible at the cost of more brittleness
///
/// # Snapshots
///
/// Updating of snapshots is controlled with the `SNAPSHOTS` environment variable:
///
/// - `skip`: do not run the tests
/// - `ignore`: run the tests but ignore their failure
/// - `verify`: run the tests
/// - `overwrite`: update the snapshots based on the output of the tests
///
/// # Patterns
///
/// - `[..]` is a character wildcard, stopping at line breaks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess we might want to add [CWD] in the future. Nevertheless it is not a blocker.

/// - `\n...\n` is a multi-line wildcard
/// - `[EXE]` matches the exe suffix for the current platform
/// - `[ROOT]` matches [`paths::root()`][crate::paths::root]
/// - `[ROOTURL]` matches [`paths::root()`][crate::paths::root] as a URL
///
/// # Normalization
///
/// In addition to the patterns described above, text is normalized
/// in such a way to avoid unwanted differences. The normalizations are:
///
/// - Backslashes are converted to forward slashes to deal with Windows paths.
/// This helps so that all tests can be written assuming forward slashes.
/// Other heuristics are applied to try to ensure Windows-style paths aren't
/// a problem.
/// - Carriage returns are removed, which can help when running on Windows.
pub fn assert_e2e() -> snapbox::Assert {
let root = paths::root();
// Use `from_file_path` instead of `from_dir_path` so the trailing slash is
// put in the users output, rather than hidden in the variable
let root_url = url::Url::from_file_path(&root).unwrap().to_string();

let mut subs = snapbox::Redactions::new();
subs.extend(MIN_LITERAL_REDACTIONS.into_iter().cloned())
.unwrap();
subs.extend(E2E_LITERAL_REDACTIONS.into_iter().cloned())
.unwrap();
subs.insert("[ROOT]", root).unwrap();
subs.insert("[ROOTURL]", root_url).unwrap();
subs.insert(
"[ELAPSED]",
regex::Regex::new("[FINISHED].*in (?<redacted>[0-9]+(\\.[0-9]+))s").unwrap(),
)
.unwrap();
snapbox::Assert::new()
.action_env(snapbox::assert::DEFAULT_ACTION_ENV)
.redact_with(subs)
}

static MIN_LITERAL_REDACTIONS: &[(&str, &str)] = &[
("[EXE]", std::env::consts::EXE_SUFFIX),
("[BROKEN_PIPE]", "Broken pipe (os error 32)"),
("[BROKEN_PIPE]", "The pipe is being closed. (os error 232)"),
];
static E2E_LITERAL_REDACTIONS: &[(&str, &str)] = &[
("[RUNNING]", " Running"),
("[COMPILING]", " Compiling"),
("[CHECKING]", " Checking"),
("[COMPLETED]", " Completed"),
("[CREATED]", " Created"),
("[CREATING]", " Creating"),
("[CREDENTIAL]", " Credential"),
("[DOWNGRADING]", " Downgrading"),
("[FINISHED]", " Finished"),
("[ERROR]", "error:"),
("[WARNING]", "warning:"),
("[NOTE]", "note:"),
("[HELP]", "help:"),
("[DOCUMENTING]", " Documenting"),
("[SCRAPING]", " Scraping"),
("[FRESH]", " Fresh"),
("[DIRTY]", " Dirty"),
("[LOCKING]", " Locking"),
("[UPDATING]", " Updating"),
("[ADDING]", " Adding"),
("[REMOVING]", " Removing"),
("[REMOVED]", " Removed"),
("[UNCHANGED]", " Unchanged"),
("[DOCTEST]", " Doc-tests"),
("[PACKAGING]", " Packaging"),
("[PACKAGED]", " Packaged"),
("[DOWNLOADING]", " Downloading"),
("[DOWNLOADED]", " Downloaded"),
("[UPLOADING]", " Uploading"),
("[UPLOADED]", " Uploaded"),
("[VERIFYING]", " Verifying"),
("[ARCHIVING]", " Archiving"),
("[INSTALLING]", " Installing"),
("[REPLACING]", " Replacing"),
("[UNPACKING]", " Unpacking"),
("[SUMMARY]", " Summary"),
("[FIXED]", " Fixed"),
("[FIXING]", " Fixing"),
("[IGNORED]", " Ignored"),
("[INSTALLED]", " Installed"),
("[REPLACED]", " Replaced"),
("[BUILDING]", " Building"),
("[LOGIN]", " Login"),
("[LOGOUT]", " Logout"),
("[YANK]", " Yank"),
("[OWNER]", " Owner"),
("[MIGRATING]", " Migrating"),
("[EXECUTABLE]", " Executable"),
("[SKIPPING]", " Skipping"),
("[WAITING]", " Waiting"),
("[PUBLISHED]", " Published"),
("[BLOCKING]", " Blocking"),
("[GENERATED]", " Generated"),
];

/// Normalizes the output so that it can be compared against the expected value.
fn normalize_actual(actual: &str, cwd: Option<&Path>) -> String {
// It's easier to read tabs in outputs if they don't show up as literal
Expand Down Expand Up @@ -185,64 +301,11 @@ fn normalize_windows(text: &str, cwd: Option<&Path>) -> String {
}

fn substitute_macros(input: &str) -> String {
let macros = [
("[RUNNING]", " Running"),
("[COMPILING]", " Compiling"),
("[CHECKING]", " Checking"),
("[COMPLETED]", " Completed"),
("[CREATED]", " Created"),
("[CREATING]", " Creating"),
("[CREDENTIAL]", " Credential"),
("[DOWNGRADING]", " Downgrading"),
("[FINISHED]", " Finished"),
("[ERROR]", "error:"),
("[WARNING]", "warning:"),
("[NOTE]", "note:"),
("[HELP]", "help:"),
("[DOCUMENTING]", " Documenting"),
("[SCRAPING]", " Scraping"),
("[FRESH]", " Fresh"),
("[DIRTY]", " Dirty"),
("[LOCKING]", " Locking"),
("[UPDATING]", " Updating"),
("[ADDING]", " Adding"),
("[REMOVING]", " Removing"),
("[REMOVED]", " Removed"),
("[UNCHANGED]", " Unchanged"),
("[DOCTEST]", " Doc-tests"),
("[PACKAGING]", " Packaging"),
("[PACKAGED]", " Packaged"),
("[DOWNLOADING]", " Downloading"),
("[DOWNLOADED]", " Downloaded"),
("[UPLOADING]", " Uploading"),
("[UPLOADED]", " Uploaded"),
("[VERIFYING]", " Verifying"),
("[ARCHIVING]", " Archiving"),
("[INSTALLING]", " Installing"),
("[REPLACING]", " Replacing"),
("[UNPACKING]", " Unpacking"),
("[SUMMARY]", " Summary"),
("[FIXED]", " Fixed"),
("[FIXING]", " Fixing"),
("[EXE]", env::consts::EXE_SUFFIX),
("[IGNORED]", " Ignored"),
("[INSTALLED]", " Installed"),
("[REPLACED]", " Replaced"),
("[BUILDING]", " Building"),
("[LOGIN]", " Login"),
("[LOGOUT]", " Logout"),
("[YANK]", " Yank"),
("[OWNER]", " Owner"),
("[MIGRATING]", " Migrating"),
("[EXECUTABLE]", " Executable"),
("[SKIPPING]", " Skipping"),
("[WAITING]", " Waiting"),
("[PUBLISHED]", " Published"),
("[BLOCKING]", " Blocking"),
("[GENERATED]", " Generated"),
];
let mut result = input.to_owned();
for &(pat, subst) in &macros {
for &(pat, subst) in MIN_LITERAL_REDACTIONS {
result = result.replace(pat, subst)
}
for &(pat, subst) in E2E_LITERAL_REDACTIONS {
result = result.replace(pat, subst)
}
result
Expand All @@ -254,7 +317,7 @@ fn substitute_macros(input: &str) -> String {
///
/// - `description` explains where the output is from (usually "stdout" or "stderr").
/// - `other_output` is other output to display in the error (usually stdout or stderr).
pub fn match_exact(
pub(crate) fn match_exact(
expected: &str,
actual: &str,
description: &str,
Expand Down Expand Up @@ -282,7 +345,7 @@ pub fn match_exact(

/// Convenience wrapper around [`match_exact`] which will panic on error.
#[track_caller]
pub fn assert_match_exact(expected: &str, actual: &str) {
pub(crate) fn assert_match_exact(expected: &str, actual: &str) {
if let Err(e) = match_exact(expected, actual, "", "", None) {
crate::panic_error("", e);
}
Expand All @@ -292,7 +355,7 @@ pub fn assert_match_exact(expected: &str, actual: &str) {
/// of the lines.
///
/// See [Patterns](index.html#patterns) for more information on pattern matching.
pub fn match_unordered(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()> {
pub(crate) fn match_unordered(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()> {
let expected = normalize_expected(expected, cwd);
let actual = normalize_actual(actual, cwd);
let e: Vec<_> = expected.lines().map(|line| WildStr::new(line)).collect();
Expand Down Expand Up @@ -342,7 +405,7 @@ pub fn match_unordered(expected: &str, actual: &str, cwd: Option<&Path>) -> Resu
/// somewhere.
///
/// See [Patterns](index.html#patterns) for more information on pattern matching.
pub fn match_contains(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()> {
pub(crate) fn match_contains(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()> {
let expected = normalize_expected(expected, cwd);
let actual = normalize_actual(actual, cwd);
let e: Vec<_> = expected.lines().map(|line| WildStr::new(line)).collect();
Expand All @@ -369,7 +432,11 @@ pub fn match_contains(expected: &str, actual: &str, cwd: Option<&Path>) -> Resul
/// anywhere.
///
/// See [Patterns](index.html#patterns) for more information on pattern matching.
pub fn match_does_not_contain(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()> {
pub(crate) fn match_does_not_contain(
expected: &str,
actual: &str,
cwd: Option<&Path>,
) -> Result<()> {
if match_contains(expected, actual, cwd).is_ok() {
bail!(
"expected not to find:\n\
Expand All @@ -388,7 +455,7 @@ pub fn match_does_not_contain(expected: &str, actual: &str, cwd: Option<&Path>)
/// somewhere, and should be repeated `number` times.
///
/// See [Patterns](index.html#patterns) for more information on pattern matching.
pub fn match_contains_n(
pub(crate) fn match_contains_n(
expected: &str,
number: usize,
actual: &str,
Expand Down Expand Up @@ -425,7 +492,7 @@ pub fn match_contains_n(
///
/// See [`crate::Execs::with_stderr_line_without`] for an example and cautions
/// against using.
pub fn match_with_without(
pub(crate) fn match_with_without(
actual: &str,
with: &[String],
without: &[String],
Expand Down Expand Up @@ -473,7 +540,7 @@ pub fn match_with_without(
/// expected JSON objects.
///
/// See [`crate::Execs::with_json`] for more details.
pub fn match_json(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()> {
pub(crate) fn match_json(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()> {
let (exp_objs, act_objs) = collect_json_objects(expected, actual)?;
if exp_objs.len() != act_objs.len() {
bail!(
Expand All @@ -494,7 +561,7 @@ pub fn match_json(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()
///
/// See [`crate::Execs::with_json_contains_unordered`] for more details and
/// cautions when using.
pub fn match_json_contains_unordered(
pub(crate) fn match_json_contains_unordered(
expected: &str,
actual: &str,
cwd: Option<&Path>,
Expand Down Expand Up @@ -552,7 +619,11 @@ fn collect_json_objects(
/// as paths). You can use a `"{...}"` string literal as a wildcard for
/// arbitrary nested JSON (useful for parts of object emitted by other programs
/// (e.g., rustc) rather than Cargo itself).
pub fn find_json_mismatch(expected: &Value, actual: &Value, cwd: Option<&Path>) -> Result<()> {
pub(crate) fn find_json_mismatch(
expected: &Value,
actual: &Value,
cwd: Option<&Path>,
) -> Result<()> {
match find_json_mismatch_r(expected, actual, cwd) {
Some((expected_part, actual_part)) => bail!(
"JSON mismatch\nExpected:\n{}\nWas:\n{}\nExpected part:\n{}\nActual part:\n{}\n",
Expand Down Expand Up @@ -619,7 +690,7 @@ fn find_json_mismatch_r<'a>(
}

/// A single line string that supports `[..]` wildcard matching.
pub struct WildStr<'a> {
pub(crate) struct WildStr<'a> {
has_meta: bool,
line: &'a str,
}
Expand Down
1 change: 1 addition & 0 deletions crates/cargo-test-support/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ pub mod prelude {
pub use crate::CargoCommand;
pub use crate::ChannelChanger;
pub use crate::TestEnv;
pub use snapbox::IntoData;
}

/*
Expand Down
14 changes: 9 additions & 5 deletions tests/testsuite/alt_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
//! Tests for alternative registries.

use cargo_test_support::compare::assert_match_exact;
use cargo_test_support::compare::assert_e2e;
use cargo_test_support::publish::validate_alt_upload;
use cargo_test_support::registry::{self, Package, RegistryBuilder};
use cargo_test_support::str;
use cargo_test_support::{basic_manifest, paths, project};
use std::fs;

Expand Down Expand Up @@ -1476,9 +1477,10 @@ fn sparse_lockfile() {
.build();

p.cargo("generate-lockfile").run();
assert_match_exact(
assert_e2e().eq(
&p.read_lockfile(),
r#"# This file is automatically @generated by Cargo.
str![[r##"
# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
version = 3

Expand All @@ -1492,8 +1494,10 @@ dependencies = [
[[package]]
name = "foo"
version = "0.1.0"
source = "sparse+http://[..]/"
checksum = "458c1addb23fde7dfbca0410afdbcc0086f96197281ec304d9e0e10def3cb899""#,
source = "sparse+http://127.0.0.1:[..]/index/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that source = "source = "sparse+http://[..]/index/" also passes the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I had snapshot updating process all of these and re-added the globbing. I tried to be more limited in where I added globs. I also tried to find where we could replace globs with something else (e.g. the [BROKEN_PIPE]) so snapshot updating can be more automatic.

I could possibly detect localhost with a port and redact the port (127.0.0.1:[PORT]). The main risk with redactions is we'd redact something that another test explicitly checks (e.g. most of the time we don't care about the .crate size reported by cargo publish but some tests specifically do)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Make sense. Thanks!

checksum = "458c1addb23fde7dfbca0410afdbcc0086f96197281ec304d9e0e10def3cb899"

"##]],
);
}

Expand Down
Loading
Loading