cargo icon indicating copy to clipboard operation
cargo copied to clipboard

Port from bespoke assertions of snapbox

Open epage opened this issue 1 year ago • 20 comments

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

  • #14243
  • #14266
  • #14269
  • #14268
  • #14270
  • #14272

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 3054936cab3d4627d5d1d878426c4fd88fab41f8
    • 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 avatar Jun 10 '24 17:06 epage

Porting progress

  • [x] tests/testsuite/advanced_env.rs #14031
  • [x] tests/testsuite/alt_registry.rs #14031
  • [x] tests/testsuite/artifact_dep.rs #14048
  • [x] tests/testsuite/artifact_dir.rs #14048
  • [x] tests/testsuite/bad_config.rs #14048
  • [x] tests/testsuite/bad_manifest_path.rs #14048
  • [x] tests/testsuite/bench.rs #14048
  • [x] tests/testsuite/binary_name.rs #14041
  • [x] tests/testsuite/build.rs #14068
  • [x] tests/testsuite/build_plan.rs #14193
  • [x] tests/testsuite/build_script.rs #14193
  • [x] tests/testsuite/build_script_env.rs #14044
  • [x] tests/testsuite/build_script_extra_link_arg.rs #14069
  • [x] tests/testsuite/cache_lock.rs #14069
  • [x] tests/testsuite/cache_messages.rs #14069
  • [x] tests/testsuite/cargo_alias_config.rs #14093
  • [x] tests/testsuite/cargo_command.rs #14113
  • [x] tests/testsuite/cargo_config/mod.rs #14093
  • [x] tests/testsuite/cargo_env_config.rs #14113
  • [x] tests/testsuite/cargo_features.rs #14113
  • [x] tests/testsuite/cargo_targets.rs #14113
  • [x] tests/testsuite/cfg.rs #14185
  • [x] tests/testsuite/check.rs #14185
  • [x] tests/testsuite/check_cfg.rs #14235
  • [x] tests/testsuite/clean.rs #14096
  • [x] tests/testsuite/collisions.rs #14079
  • [x] tests/testsuite/concurrent.rs #14079
  • [x] tests/testsuite/config.rs #14079
  • [x] tests/testsuite/config_cli.rs #14079
  • [x] tests/testsuite/config_include.rs #14079
  • [x] tests/testsuite/corrupt_git.rs #14079
  • [x] tests/testsuite/credential_process.rs ##14132
  • [x] tests/testsuite/cross_compile.rs #14132
  • [x] tests/testsuite/cross_publish.rs #14132
  • [x] tests/testsuite/custom_target.rs #14132
  • [x] tests/testsuite/death.rs #14091
  • [x] tests/testsuite/dep_info.rs #14143
  • [x] tests/testsuite/diagnostics.rs #14143
  • [x] tests/testsuite/direct_minimal_versions.rs #14143
  • [x] tests/testsuite/directory.rs #14171
  • [x] tests/testsuite/doc.rs #14171
  • [x] tests/testsuite/docscrape.rs #14171
  • [x] tests/testsuite/edition.rs #14175
  • [x] tests/testsuite/error.rs #14175
  • [x] tests/testsuite/features.rs #14100
  • [x] tests/testsuite/features2.rs #14100
  • [x] tests/testsuite/features_namespaced.rs #14100
  • [x] tests/testsuite/fetch.rs #14214
  • [x] tests/testsuite/fix.rs #14173
  • [x] tests/testsuite/fix_n_times.rs #14173
  • [x] tests/testsuite/freshness.rs #14161
  • [x] tests/testsuite/future_incompat_report.rs #14173
  • [x] tests/testsuite/generate_lockfile.rs #14200
  • [x] tests/testsuite/git.rs #14159
  • [x] tests/testsuite/git_auth.rs #14172
  • [x] tests/testsuite/git_gc.rs #14087
  • [x] tests/testsuite/git_shallow.rs #14087
  • [x] tests/testsuite/glob_targets.rs #14200
  • [x] tests/testsuite/global_cache_tracker.rs #14244
  • [x] tests/testsuite/help.rs #14060
  • [x] tests/testsuite/https.rs #14187
  • [x] tests/testsuite/inheritable_workspace_fields.rs #14170
  • [x] tests/testsuite/install.rs #14170
  • [x] tests/testsuite/install_upgrade.rs #14170
  • [x] tests/testsuite/jobserver.rs #14191
  • [x] tests/testsuite/lints/implicit_features.rs #14245
  • [x] tests/testsuite/lints/mod.rs #14104
  • [x] tests/testsuite/lints/unknown_lints.rs #14104
  • [x] tests/testsuite/lints/unused_optional_dependencies.rs #14026
  • [x] tests/testsuite/lints_table.rs #14104
  • [x] tests/testsuite/list_availables.rs #14214
  • [x] tests/testsuite/local_registry.rs #14158
  • [x] tests/testsuite/locate_project.rs #14158
  • [x] tests/testsuite/lockfile_compat.rs #14158
  • [x] tests/testsuite/login.rs #14158
  • [x] tests/testsuite/logout.rs #14158
  • [x] tests/testsuite/lto.rs #14209
  • [x] tests/testsuite/member_discovery.rs #14082
  • [x] tests/testsuite/member_errors.rs #14210
  • [x] tests/testsuite/message_format.rs #14091
  • [x] tests/testsuite/messages.rs #14242
  • [x] tests/testsuite/metabuild.rs #14162
  • [x] tests/testsuite/metadata.rs #14162
  • [x] tests/testsuite/minimal_versions.rs #14080
  • [x] tests/testsuite/multitarget.rs #14210
  • [x] tests/testsuite/net_config.rs #14162
  • [x] tests/testsuite/new.rs #14210
  • [x] tests/testsuite/offline.rs #14138
  • [x] tests/testsuite/old_cargos.rs #14410
  • [x] tests/testsuite/open_namespaces.rs #14138
  • [x] tests/testsuite/owner.rs #14138
  • [x] tests/testsuite/package.rs #14130
  • [x] tests/testsuite/package_features.rs #14130
  • [x] tests/testsuite/patch.rs #14181
  • [x] tests/testsuite/path.rs #14109
  • [x] tests/testsuite/paths.rs #14109
  • [x] tests/testsuite/pkgid.rs #14181
  • [x] tests/testsuite/precise_pre_release.rs #14091
  • [x] tests/testsuite/proc_macro.rs #14181
  • [x] tests/testsuite/profile_config.rs #14128
  • [x] tests/testsuite/profile_custom.rs #14128
  • [x] tests/testsuite/profile_overrides.rs #14128
  • [x] tests/testsuite/profile_targets.rs #14128
  • [x] tests/testsuite/profile_trim_paths.rs #14128
  • [x] tests/testsuite/profiles.rs #14128
  • [x] tests/testsuite/progress.rs #14181
  • [x] tests/testsuite/pub_priv.rs #14103
  • [x] tests/testsuite/publish.rs #14130
  • [x] tests/testsuite/publish_lockfile.rs #14130
  • [x] tests/testsuite/read_manifest.rs #14180
  • [x] tests/testsuite/registry.rs #14149
  • [x] tests/testsuite/registry_auth.rs #14149
  • [x] tests/testsuite/rename_deps.rs #14127
  • [x] tests/testsuite/replace.rs #14127
  • [x] tests/testsuite/required_features.rs #14127
  • [x] tests/testsuite/run.rs #14127
  • [x] tests/testsuite/rust_version.rs #14177
  • [x] tests/testsuite/rustc.rs #14177
  • [x] tests/testsuite/rustc_info_cache.rs #14177
  • [x] tests/testsuite/rustdoc.rs #14098
  • [x] tests/testsuite/rustdoc_extern_html.rs #14180
  • [x] tests/testsuite/rustdocflags.rs #14098
  • [x] tests/testsuite/rustflags.rs #14126
  • [x] tests/testsuite/rustup.rs #14126
  • [x] tests/testsuite/script.rs #14126
  • [x] tests/testsuite/search.rs #14151
  • [x] tests/testsuite/shell_quoting.rs #14051
  • [x] tests/testsuite/source_replacement.rs #14151
  • [x] tests/testsuite/ssh.rs #14187
  • [x] tests/testsuite/standard_lib.rs #14151
  • [x] tests/testsuite/test.rs #14226
  • [x] tests/testsuite/timings.rs #14082
  • [x] tests/testsuite/tool_paths.rs #14180
  • [x] tests/testsuite/tree.rs #14094
  • [x] tests/testsuite/tree_graph_features.rs #14094
  • [x] tests/testsuite/unit_graph.rs #14119
  • [x] tests/testsuite/update.rs #14119
  • [x] tests/testsuite/vendor.rs #14119
  • [x] tests/testsuite/verify_project.rs #14091
  • [x] tests/testsuite/version.rs #14091
  • [x] tests/testsuite/warn_on_failure.rs #14180
  • [x] tests/testsuite/weak_dep_features.rs #14111
  • [x] tests/testsuite/workspaces.rs #14111
  • [x] tests/testsuite/yank.rs #14111
  • [x] tests/build-std/main.rs #14241

weihanglo avatar Jun 10 '24 17:06 weihanglo

I would like to try the following file:

 tests/testsuite/build_script_env.rs
 tests/testsuite/build_script_extra_link_arg.rs
 tests/testsuite/cache_lock.rs
 tests/testsuite/cache_messages.rs
 tests/testsuite/cargo_alias_config.rs
 tests/testsuite/cargo_command.rs
 tests/testsuite/cargo_config/mod.rs
 tests/testsuite/cargo_env_config.rs
 tests/testsuite/cargo_features.rs
 tests/testsuite/cargo_targets.rs

heisen-li avatar Jun 11 '24 03:06 heisen-li

#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.

epage avatar Jun 11 '24 13:06 epage

@weihanglo I'd like to claim tests/testsuite/shell_quoting.rs.

henry40408 avatar Jun 12 '24 14:06 henry40408

@weihanglo I'd like to claim tests/testsuite/help.rs.

choznerol avatar Jun 13 '24 13:06 choznerol

@weihanglo I would like to claim/lock tests/testsuite/check_cfg.rs and tests/testsuite/freshness.rs for now to prevent conflict with the fix for #14076.

choznerol avatar Jun 15 '24 14:06 choznerol

@weihanglo going to pick up these:

tests/testsuite/collisions.rs
tests/testsuite/concurrent.rs
tests/testsuite/config.rs
tests/testsuite/config_cli.rs
tests/testsuite/config_include.rs
tests/testsuite/corrupt_git.rs

dieterplex avatar Jun 15 '24 16:06 dieterplex

@weihanglo I'd like to claim tests/testsuite/minimal_versions.rs

henry40408 avatar Jun 16 '24 03:06 henry40408

@weihanglo I'd like to report that with_stdout or with_stderr doesn't appear in tests/testsuite/member_discovery.rs, so the file should be removed from the list.

By the way, I'd like to claim tests/testsuite/timings.rs.

henry40408 avatar Jun 16 '24 13:06 henry40408

@henry40408 would you mind removing #[allow(deprecated)] from memeber_discovery.rs?

weihanglo avatar Jun 16 '24 13:06 weihanglo

@weihanglo I'll remove it in https://github.com/rust-lang/cargo/pull/14082

henry40408 avatar Jun 16 '24 14:06 henry40408

woring on clean at #14096

dieterplex avatar Jun 18 '24 14:06 dieterplex

I'd like to claim tests/testsuite/pub_priv.rs since i have added the test before

linyihai avatar Jun 19 '24 02:06 linyihai

I would like to complete the following document:

 tests/testsuite/credential_process.rs
 tests/testsuite/cross_compile.rs
 tests/testsuite/cross_publish.rs
 tests/testsuite/custom_target.rs

 tests/testsuite/dep_info.rs
 tests/testsuite/diagnostics.rs
 tests/testsuite/direct_minimal_versions.rs
 tests/testsuite/directory.rs
 tests/testsuite/doc.rs
 tests/testsuite/docscrape.rs
 tests/testsuite/edition.rs
 tests/testsuite/error.rs

heisen-li avatar Jun 20 '24 12:06 heisen-li

Claim these

  • tests/testsuite/git.rs
  • tests/testsuite/git_auth.rs

dieterplex avatar Jun 26 '24 22:06 dieterplex

claim tests/testsuite/lints/unused_optional_dependencies.rs

Since I am now modifying this test case, I can migrate it together :)

linyihai avatar Jul 02 '24 02:07 linyihai

Cooking tests/testsuite/lto.rs

dieterplex avatar Jul 04 '24 15:07 dieterplex

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

epage avatar Jul 19 '24 19:07 epage

I fixed assert-rs/snapbox#348 but found assert-rs/snapbox#351 and assert-rs/snapbox#352

epage avatar Jul 23 '24 19:07 epage

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

epage avatar Aug 16 '24 21:08 epage