tests: Migrate alt_registry to snapbox #14031

epage · 2024-06-07T21:02:58Z

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.

rustbot · 2024-06-07T21:03:04Z

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

weihanglo

Thanks for you making this come true!!!

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.

Are you going to work this out by yourself? I believe these could be good issues for contributors.

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

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

weihanglo · 2024-06-08T15:08:34Z

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

@@ -642,6 +647,7 @@ impl Execs {
    /// See [`compare`] for supported patterns.
    ///
    /// See note on [`Self::with_stderr_does_not_contain`].
+    #[deprecated]


What is the replacement of this?

I realized it after reading changes in alt_regsitry. This is deprecated because we're going to assert the entire output instead of a subset of the output. Is it the plan in your mind? There are some tests asserting against rustc output. How should we deal with that?

(Not a blocker of this PR)

See #14031 (comment)

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

bors · 2024-06-09T17:46:03Z

☔ The latest upstream changes (presumably #13995) made this pull request unmergeable. Please resolve the merge conflicts.

While this is noisy and hides other deprecations, I figured deprecations would make it easier for people to discover what tasks remain and allow us to divide and conquer this work rather than doing a heroic PR. In theory, this will be short lived and we'll go back to seeing deprecations in our tests.

epage · 2024-06-10T15:48:27Z

Are you going to work this out by yourself? I believe these could be good issues for contributors.

Not at all, I plan to make this available for anyone to contribute. In fact, Scott has already started...

Potential porting instructions

1. Select a file to port

The files should have #![allow(deprecated)] at the top, e.g.

$ git pull --rebase
$ rg '#!.allow.deprecated' tests/testsuite

Please check the comments on this issue for anyone to have claimed the file and then post that you claim the file

2. Remove `#![allow(deprecated)]` to identify what work is needed

If nothing, congrats, that was easy!

3. Resolve basic deprecations

Replace Execs::with_stdout("...") with

use cargo_test_support::prelude::*;
use cargo_test_support::str;

.with_stdout_data(str![])

Replace Execs::with_stderr("...") with

use cargo_test_support::prelude::*;
use cargo_test_support::str;

.with_stderr_data(str![])

(prelude is only needed for some steps)

Commit these changes

Run SNAPSHOTS=overwrite cargo test && cargo check --test testsuite. In rare cases, SNAPSHOTS=overwrite may make bad edits. Feel free to create a reproduction case and create an issue. They are usually obvious how to fix, so don't worry.

Diff the snapshots. Is there anything machine or run specific in them that previously was elided out with [..]?

Can you auto-redact it like 5ea1c8f? If so, revert the snapshot updates, add the auto-redaction (as a separate commit before any test changes), and re-run the snapshots
Otherwise, replace it with [..]

Once its working, amend your commit with the snapshots

4. Resolve non-literal deprecations

Like Step 3, but not just with_stdout("...") but with variables or format!.

Evaluate whether a literal could be used

Is the variable important for showing that output remains unchanged between instances, then keep it
Are we specifically testing for what we are using format! for, then keep it
- If not, see if it can be expressed with an auto-redaction

So this can end up with either

with_std*_data(expression)
with_std*_data(str![]) like in Step 3

5. Repeat with "straight forward" deprecations

with_stdout_unordered(expected) -> with_stdout_data(expected.unordered())
with_stderr_unordered(expected) -> with_stderr_data(expected.unordered())
with_json(expected) -> with_stdout_data(expected.json_lines()) or with_stdout_data(expected.json())
- May also require .unordered()

6. Contains / Does not Contains deprecations

A judgement needs to be made for how to handle these.

A contains can be modeled by surrounding a entry with ... (text) or "...", (json lines). The question is whether we should still do this or use a different method.

Multiple contains can be merged into a single eq with one ... and .unordered(). For example, see 3054936

A does_not_contain cannot be modeled at this time. The challenge with these is that they are brittle and you can't tell when they are no longer testing for what you think because the output changed. We should evaluate what to do with these on a case-by-case basis.

When in doubt, feel free to put #[allow(deprecated)] on a statement and move on. We can come back to these later.

weihanglo

Looks great! Thanks a lot.

weihanglo · 2024-06-10T17:10:42Z

@bors r+

bors · 2024-06-10T17:10:45Z

📌 Commit e589ed7 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-06-10T17:11:53Z

⌛ Testing commit e589ed7 with merge a9ee3e8...

bors · 2024-06-10T17:48:18Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing a9ee3e8 to master...

Update cargo 14 commits in b1feb75d062444e2cee8b3d2aaa95309d65e9ccd..4dcbca118ab7f9ffac4728004c983754bc6a04ff 2024-06-07 20:16:17 +0000 to 2024-06-11 16:27:02 +0000 - Add local registry overlays (rust-lang/cargo#13926) - docs(change): Don't mention non-existent workspace.badges (rust-lang/cargo#14042) - test: migrate binary_name to snapbox (rust-lang/cargo#14041) - Bump to 0.82.0; update changelog (rust-lang/cargo#14040) - tests: Migrate alt_registry to snapbox (rust-lang/cargo#14031) - fix: proc-macro example from dep no longer affects feature resolution (rust-lang/cargo#13892) - chore: Bump cargo-util-schemas to 0.5 (rust-lang/cargo#14038) - chore(deps): update rust crate pulldown-cmark to 0.11.0 (rust-lang/cargo#14037) - fix: remove `__CARGO_GITOXIDE_DISABLE_LIST_FILES` env var (rust-lang/cargo#14036) - chore(deps): update rust crate itertools to 0.13.0 (rust-lang/cargo#13998) - fix(toml): remove `lib.plugin` key support and make it warning (rust-lang/cargo#13902) - chore(deps): update compatible (rust-lang/cargo#13995) - fix: using `--release/debug` and `--profile` together becomes an error (rust-lang/cargo#13971) - fix(toml): Convert warnings that `licence` and `readme` files do not exist into errors (rust-lang/cargo#13921) r? ghost

rustbot assigned ehuss Jun 7, 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 Jun 7, 2024

weihanglo reviewed Jun 9, 2024

View reviewed changes

epage force-pushed the snap branch from 25d236a to d24a4a3 Compare June 10, 2024 14:48

epage added 10 commits June 10, 2024 10:20

chore(test): Bump cargo-test-support to 0.2.2

d028cfa

feat(test): Make latest snapbox features available

ef26e8e

feat(test): Enable structured data assertions

2b7e335

fix(test): Ensure panics show test code, not lib

15e3412

feat(test): Auto-redact crate sizes from cargo-package

8b7de1e

feat(test): Auto-redact registry source hashes

0761937

feat(test): Allow snapshotting of Execs

ff2ddeb

test: Resolve advanced_env deprecations

7f90d9a

test: Resolve alt_registry deprecations

e589ed7

epage force-pushed the snap branch from d24a4a3 to 2137cfe Compare June 10, 2024 15:21

epage force-pushed the snap branch 2 times, most recently from cda6b87 to e589ed7 Compare June 10, 2024 16:07

weihanglo approved these changes Jun 10, 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 10, 2024

bors merged commit a9ee3e8 into rust-lang:master Jun 10, 2024
24 checks passed

epage deleted the snap branch June 10, 2024 17:49

epage mentioned this pull request Jun 10, 2024

Port from bespoke assertions of snapbox #14039

Open

weihanglo mentioned this pull request Jun 12, 2024

Update cargo rust-lang/rust#126290

Merged

rustbot added this to the 1.81.0 milestone Jun 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Migrate alt_registry to snapbox #14031

tests: Migrate alt_registry to snapbox #14031

epage commented Jun 7, 2024

rustbot commented Jun 7, 2024

weihanglo left a comment

weihanglo Jun 8, 2024

weihanglo Jun 9, 2024

epage Jun 10, 2024

bors commented Jun 9, 2024

epage commented Jun 10, 2024 •

edited by weihanglo

Loading

weihanglo left a comment

weihanglo commented Jun 10, 2024

bors commented Jun 10, 2024

bors commented Jun 10, 2024

bors commented Jun 10, 2024

tests: Migrate alt_registry to snapbox #14031

tests: Migrate alt_registry to snapbox #14031

Conversation

epage commented Jun 7, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Jun 7, 2024

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Jun 8, 2024

Choose a reason for hiding this comment

weihanglo Jun 9, 2024

Choose a reason for hiding this comment

epage Jun 10, 2024

Choose a reason for hiding this comment

bors commented Jun 9, 2024

epage commented Jun 10, 2024 • edited by weihanglo Loading

Potential porting instructions

1. Select a file to port

2. Remove #![allow(deprecated)] to identify what work is needed

3. Resolve basic deprecations

4. Resolve non-literal deprecations

5. Repeat with "straight forward" deprecations

6. Contains / Does not Contains deprecations

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Jun 10, 2024

bors commented Jun 10, 2024

bors commented Jun 10, 2024

bors commented Jun 10, 2024

epage commented Jun 10, 2024 •

edited by weihanglo

Loading

2. Remove `#![allow(deprecated)]` to identify what work is needed