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

epage · 2024-05-29T16:46:12Z

What does this PR try to resolve?

Cargo has a bespoke testing framework for functional tests

Extra stuff for us to maintain
Don't leverage benefits from contributions related to other projects
Less incentive to be thoroughly documented

UI tests are written using snapbox. The latest release of snapbox (#13963) was geared at supporting cargo's needs in the hope that we can consolidate on testing frameworks.

Besides having a single set of semantics, benefits we'd gain include

Updating of test snapshots
Fancier redacting of test output (e.g. feat(test): Auto-redact elapsed time #13973)

This is the first incremental step in this direction. This replaces direct assertions with snapbox assertions. This still leaves all of the CLI output assertions. These will be done incrementally.

How should we test and review this PR?

Additional information

rustbot · 2024-05-29T16:46:17Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

This comes with improved redacting on mismatches which will provide more focused assertions and there will be less need for intervention on snapshot updates. This also gives more flexibility with placeholders

This leaves off `validate_crate_contents` as that would be an effort on its own

epage · 2024-05-29T21:49:27Z

@rustbot ready

Rustin170506

Thanks for adding it.

Rustin170506 · 2024-05-30T11:25:01Z

tests/testsuite/artifact_dep.rs

+            str![[r#"
+[ROOT]/foo/target/debug/deps/artifact/bar-baz-[..]/bin
+[ROOT]/foo/target/debug/deps/artifact/bar-baz-[..]/bin/baz_suffix-[..]
+


Can I ask when we need to add a blank line? I tried this locally, it seemed to pass the test after I removed it.

The snapshotting always adds a newline over whats already there so the closing "#]] is on its own line. Rather than error if the newline isn't there, its just ignored.

(iirc cargo-test-support was also sloppy with newlines but it wasn't as obvious because it doesn't have snapshot updating)

Rustin170506 · 2024-05-30T11:27:34Z

tests/testsuite/alt_registry.rs

@@ -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/"


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

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)

Got it. Make sense. Thanks!

Rustin170506

Looks good to me. Thanks! 👍

Rustin170506 · 2024-06-02T13:57:13Z

I guess you need to rerun the tests.

weihanglo · 2024-06-02T14:13:30Z

@bors try

bors · 2024-06-02T14:14:49Z

⌛ Trying commit 995746b with merge 2883e4f...

refactor: Transition direct assertions from cargo-test-support to snapbox ### What does this PR try to resolve? Cargo has a bespoke testing framework for functional tests - Extra stuff for us to maintain - Don't leverage benefits from contributions related to other projects - Less incentive to be thoroughly documented UI tests are written using snapbox. The latest release of snapbox (#13963) was geared at supporting cargo's needs in the hope that we can consolidate on testing frameworks. Besides having a single set of semantics, benefits we'd gain include - Updating of test snapshots - Fancier redacting of test output (e.g. #13973) This is the first incremental step in this direction. This replaces direct assertions with snapbox assertions. This still leaves all of the CLI output assertions. These will be done incrementally. ### How should we test and review this PR? ### Additional information

bors · 2024-06-02T14:41:57Z

☀️ Try build successful - checks-actions
Build commit: 2883e4f (2883e4f248f493cf212ecf13cb907c999c89e50d)

weihanglo

Thank you all for working on and reviewing this!

weihanglo · 2024-06-02T14:46:34Z

crates/cargo-test-support/src/compare.rs

+///
+/// # Patterns
+///
+/// - `[..]` is a character wildcard, stopping at line breaks


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

weihanglo · 2024-06-02T14:46:54Z

@bors r=hi-rustin

bors · 2024-06-02T14:46:56Z

📌 Commit 995746b has been approved by hi-rustin

It is now in the queue for this repository.

bors · 2024-06-02T14:48:04Z

⌛ Testing commit 995746b with merge 4b681c7...

bors · 2024-06-02T15:16:50Z

☀️ Test successful - checks-actions
Approved by: hi-rustin
Pushing 4b681c7 to master...

Update cargo 9 commits in 7a6fad0984d28c8330974636972aa296b67c4513..34a6a87d8a2330d8c9d578f927489689328a652d 2024-05-31 22:26:03 +0000 to 2024-06-04 15:31:01 +0000 - Silence the warning about forgetting the vendoring (rust-lang/cargo#13886) - fix(vendor): Ensure sort happens for vendor (rust-lang/cargo#14004) - fix(add): Avoid escaping double-quotes by using string literals (rust-lang/cargo#14006) - refactor(source): Split `RecursivePathSource` out of `PathSource` (rust-lang/cargo#13993) - doc: Add README for resolver-tests (rust-lang/cargo#13977) - Allows the default git/gitoxide configuration to be obtained from the ENV and config (rust-lang/cargo#13687) - refactor: Transition direct assertions from cargo-test-support to snapbox (rust-lang/cargo#13980) - Fix: Skip deserialization of unrelated fields with overlapping name (rust-lang/cargo#14000) - chore(deps): update alpine docker tag to v3.20 (rust-lang/cargo#13996) r? ghost

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.

Update cargo 9 commits in 7a6fad0984d28c8330974636972aa296b67c4513..34a6a87d8a2330d8c9d578f927489689328a652d 2024-05-31 22:26:03 +0000 to 2024-06-04 15:31:01 +0000 - Silence the warning about forgetting the vendoring (rust-lang/cargo#13886) - fix(vendor): Ensure sort happens for vendor (rust-lang/cargo#14004) - fix(add): Avoid escaping double-quotes by using string literals (rust-lang/cargo#14006) - refactor(source): Split `RecursivePathSource` out of `PathSource` (rust-lang/cargo#13993) - doc: Add README for resolver-tests (rust-lang/cargo#13977) - Allows the default git/gitoxide configuration to be obtained from the ENV and config (rust-lang/cargo#13687) - refactor: Transition direct assertions from cargo-test-support to snapbox (rust-lang/cargo#13980) - Fix: Skip deserialization of unrelated fields with overlapping name (rust-lang/cargo#14000) - chore(deps): update alpine docker tag to v3.20 (rust-lang/cargo#13996) r? ghost

rustbot assigned ehuss May 29, 2024

rustbot added A-testing-cargo-itself Area: cargo's tests S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2024

epage marked this pull request as draft May 29, 2024 16:59

epage force-pushed the compare branch from 2fa20f6 to 37c1e4b Compare May 29, 2024 18:58

epage added 8 commits May 29, 2024 14:07

chore: Upgrade to snapbox 0.6.7

995c2b7

This comes with improved redacting on mismatches which will provide more focused assertions and there will be less need for intervention on snapshot updates. This also gives more flexibility with placeholders

fix(test)!: Hide API that isnt used by Cargo

f22c43b

docs(test): Clarify role of assert_ui

94119af

refactor(test): Share redactions between ui and e2e

ea36ed0

refactor(test): Pull out end-to-end literal redactions

fe5c2d3

refactor: Port from assert_matches_exact to assert_e2e

3054936

This leaves off `validate_crate_contents` as that would be an effort on its own

refactor: Port from matches_unordered to assert_e2e

eee1053

refactor: Port from matches_contains to assert_e2e

995746b

epage force-pushed the compare branch 4 times, most recently from e450cbd to 010c112 Compare May 29, 2024 21:03

epage marked this pull request as ready for review May 29, 2024 21:42

Rustin170506 reviewed May 30, 2024

View reviewed changes

epage force-pushed the compare branch 3 times, most recently from 982eadf to 995746b Compare June 1, 2024 01:29

Rustin170506 approved these changes Jun 2, 2024

View reviewed changes

weihanglo approved these changes Jun 2, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2024

bors merged commit 4b681c7 into rust-lang:master Jun 2, 2024
15 of 23 checks passed

bors mentioned this pull request Jun 2, 2024

Add cargo update --breaking #13979

Merged

6 tasks

epage deleted the compare branch June 3, 2024 14:37

weihanglo mentioned this pull request Jun 5, 2024

Update cargo rust-lang/rust#126005

Merged

rustbot added this to the 1.80.0 milestone Jun 5, 2024

epage mentioned this pull request Jun 7, 2024

tests: Migrate alt_registry to snapbox #14031

Merged

epage mentioned this pull request Jun 10, 2024

Port from bespoke assertions of snapbox #14039

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

epage commented May 29, 2024

rustbot commented May 29, 2024

epage commented May 29, 2024

Rustin170506 left a comment

Rustin170506 May 30, 2024

epage May 30, 2024

Rustin170506 Jun 2, 2024

Rustin170506 May 30, 2024

epage May 30, 2024

Rustin170506 Jun 2, 2024

Rustin170506 left a comment

Rustin170506 commented Jun 2, 2024

weihanglo commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024

weihanglo left a comment

weihanglo Jun 2, 2024

weihanglo commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024

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

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

Conversation

epage commented May 29, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented May 29, 2024

epage commented May 29, 2024

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 May 30, 2024

Choose a reason for hiding this comment

epage May 30, 2024

Choose a reason for hiding this comment

Rustin170506 Jun 2, 2024

Choose a reason for hiding this comment

Rustin170506 May 30, 2024

Choose a reason for hiding this comment

epage May 30, 2024

Choose a reason for hiding this comment

Rustin170506 Jun 2, 2024

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 commented Jun 2, 2024

weihanglo commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Jun 2, 2024

Choose a reason for hiding this comment

weihanglo commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024