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.

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

Port from bespoke assertions of snapbox #14039

Open
epage opened this issue Jun 10, 2024 · 20 comments
Open

Port from bespoke assertions of snapbox #14039

epage opened this issue Jun 10, 2024 · 20 comments
Labels
A-testing-cargo-itself Area: cargo's tests C-enhancement Category: enhancement S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@epage
Copy link
Contributor

epage commented Jun 10, 2024

Traditionally, cargo-test-support has had bespoke assertions, requiring hand implemented diff algorithms. As there is less of a community around this, the knowledge is more specialized, its less likely to be documented, and we are on our own for feature development.

In #13980 and #14031, we introduced the use of snapbox as replacement for our own assertions and need to work to migrate to them.

Aside: doc updates related to this transition

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

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)

Side note: If the test is specifically wanting to assert an empty string, use "" rather than str![] so that people are less likely to accidentally do a snapshot update to bless output that gets added.

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.

  • If replacing a contains with an equality check, use multi-line globs (...) for rustc errors that weren't previously matched to minimize tying Cargo's tests to the exact output of rustc
  • Multiple contains can be merged into a single eq with .. interspersed (or one ... and .unordered()). For example, see 3054936
    • When using unordered, likely string literals, rather than str![] should be used, as these can't correctly be updated

A does_not_contain/line_without 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.

@epage epage added C-enhancement Category: enhancement A-testing-cargo-itself Area: cargo's tests S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review labels Jun 10, 2024
@weihanglo

This comment was marked as resolved.

@heisen-li

This comment was marked as resolved.

@epage
Copy link
Contributor Author

epage commented Jun 11, 2024

#14041 highlights a blocker for tests/testsuite/message_format.rs (and other jsonlines tests): snapbox can only render the output as jsonlines and not our "pretty printed" variant which can make the messages harder to read.

@henry40408

This comment was marked as resolved.

bors added a commit that referenced this issue Jun 12, 2024
test: migrate build_script_env to snapbox

### What does this PR try to resolve?

part of #14039.
bors added a commit that referenced this issue Jun 13, 2024
test: migrate features_are_quoted to snapbox

### What does this PR try to resolve?

Part of #14039.

Migrate `tests/testsuite/shell_quoting.rs` to snapbox.

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

N/A

### Additional information

N/A
@choznerol

This comment was marked as resolved.

bors added a commit that referenced this issue Jun 13, 2024
Migrate a few test files to snapbox

This migrates the following files to `snapbox`
- `artifact_dep`
  - Has a few `does_not_contain`
- `artifact_dir`
- `bad_config`
- `bad_manifest_path`
  - Does not use `str!` for all tests
- `bench`

Note: This also adds auto-redactions for:
- `[HOST_TARGET]`
- `[ALT_TARGET]`
  - Only added if cross-compilation is allowed for the target
- `[AVG_ELAPSED]`
  - For `bench` output
- `[JITTER]`
  - For `bench` output

Part of #14039
@choznerol

This comment has been minimized.

@dieterplex

This comment has been minimized.

dieterplex added a commit to dieterplex/cargo that referenced this issue Jun 15, 2024
dieterplex added a commit to dieterplex/cargo that referenced this issue Jun 15, 2024
dieterplex added a commit to dieterplex/cargo that referenced this issue Jun 15, 2024
bors added a commit that referenced this issue Jun 15, 2024
test: Migrate tests/testsuite/co*.rs to snapbox

Migrating files:

- tests/testsuite/collisions.rs
  - `with_stderr_does_not_contain` in test `collision_doc_host_target_feature_split`
- tests/testsuite/concurrent.rs
- tests/testsuite/config.rs
- tests/testsuite/config_cli.rs
- tests/testsuite/config_include.rs
- tests/testsuite/corrupt_git.rs

Testing with command `SNAPSHOTS=overwrite cargo test collisions::` or so.

Part of #14039
@henry40408

This comment was marked as resolved.

bors added a commit that referenced this issue Jul 12, 2024
test: migrate build-std/main to snapbox

### What does this PR try to resolve?

Part of #14039.

Migrate following to snapbox:

- `tests/build-std/main.rs`
bors added a commit that referenced this issue Jul 12, 2024
test: migrate implicit_features to snapbox

### What does this PR try to resolve?

Part of #14039.

Migrate following to snapbox:

- `tests/testsuite/implicit_features.rs`
bors added a commit that referenced this issue Jul 13, 2024
feat(test): Add cargo_test to test-support prelude

### What does this PR try to resolve?

With where we are at with #14039, I was revisiting our test writing documentation.  I was wanting to put more emphasis on rustdoc and found it would be helpful to talk about the concepts if a prelude was used instead of `extern crate`.

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

Yes, this included some `use` clean ups in tangentially-related commit but its already painful enough
walking through every test file, I didn't want to do it twice.

### Additional information
bors added a commit that referenced this issue Jul 15, 2024
test: migrate fetch and list_availables to snapbox

### What does this PR try to resolve?

Part of #14039.

Migrate following to snapbox:

- `tests/testsuite/fetch.rs`
- `tests/testsuite/list_availables.rs`
bors added a commit that referenced this issue Jul 17, 2024
Migrate global_cache_tracker snapbox

Part of #14039.

Some of these tests may require to assert specific removed file count or size that would be redact by current design. Need feedback to come out solution.

Temporary commit to pass the tests and illustrate what need to fix.
@epage
Copy link
Contributor Author

epage commented Jul 19, 2024

I've created assert-rs/snapbox#348 to track what we need from snapbox for jsonlines. See that issue for my proposed solution.

bors added a commit that referenced this issue Jul 22, 2024
docs(test): Expand documentation of cargo-test-support

### What does this PR try to resolve?

In wanting to document #14039, I felt it would be good to put that documentation in `cargo-test-support`.  To do so, I wanted a baseline of existing documentation for it to build on top of.

I was tempted to move more of "Writing tests" contrib documentation here, as its more about using `cargo-test-support` but I decided to hold off for now as most of that was long-form documentation and this is mostly focused on reference documentation.

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

### Additional information
@epage
Copy link
Contributor Author

epage commented Jul 23, 2024

bors added a commit that referenced this issue Jul 24, 2024
test: Migrate some json tests to snapbox

### What does this PR try to resolve?

This builds on assert-rs/snapbox#348 and is part of #14039.

Note: this also updates existing `.is_jsonlines()` usage to `.is_json().against_jsonlines()`.

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

### Additional information
bors added a commit that referenced this issue Jul 25, 2024
Misc test clean up

### What does this PR try to resolve?

Part of this is a follow up to #14293.

Part of this is a follow up to some of #14039's work.

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

### Additional information
bors added a commit that referenced this issue Jul 26, 2024
test: migrate messages to snapbox

### What does this PR try to resolve?

Part of #14039.

Migrate following to snapbox:

- `tests/testsuite/messages.rs`
bors added a commit that referenced this issue Aug 15, 2024
test: Migrate some json tests to snapbox

### What does this PR try to resolve?

This is part of #14039

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

### Additional information

This was unblocked because of assert-rs/snapbox#350
epage added a commit to epage/cargo that referenced this issue Aug 15, 2024
bors added a commit that referenced this issue Aug 16, 2024
test: Migrate old_cargos to snapbox

This is part of #14039
@epage
Copy link
Contributor Author

epage commented Aug 16, 2024

Looks like we've finished every file-level migration. We have 124 remaining #[allow(deprecated)]s left.

antoniospg pushed a commit to antoniospg/cargo that referenced this issue Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing-cargo-itself Area: cargo's tests C-enhancement Category: enhancement S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

No branches or pull requests

7 participants