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

test: migrate some files to snapbox #14113

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

heisen-li
Copy link
Contributor

What does this PR try to resolve?

Changes to the following documents were completed:

tests/testsuite/cargo_command.rs
tests/testsuite/cargo_env_config.rs
tests/testsuite/cargo_features.rs
tests/testsuite/cargo_targets.rs

part of #14039

@rustbot
Copy link
Collaborator

rustbot commented Jun 20, 2024

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

@rustbot 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 20, 2024
}

#[cargo_test]
fn list_builtin_aliases_with_descriptions() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the output on the right is complete, the test case here is deleted.

@@ -132,6 +178,7 @@ fn list_command_looks_at_path_case_mismatch() {
);
}

#[allow(deprecated)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since with_stdout_does_not_contain,add #[allow(deprecated)].

@bors
Copy link
Collaborator

bors commented Jun 23, 2024

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

crates/cargo-test-support/src/compare.rs Outdated Show resolved Hide resolved
crates/cargo-test-support/src/compare.rs Outdated Show resolved Hide resolved
tests/testsuite/cargo_command.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Jun 26, 2024

Could you clean up the commit history?

Comment on lines 392 to 395
.with_stdout_data(str![[r#"
[CARGO_PATH]

"#]])
Copy link
Contributor

Choose a reason for hiding this comment

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

So this was already a "self updating" test output and I believe the design of it means that there is little to evolve that we'd need to allow snapshotting more from this.

Should we keep the expected-str generation that was in here before, just switching the assert method used?

Copy link
Contributor Author

@heisen-li heisen-li Jun 26, 2024

Choose a reason for hiding this comment

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

It is not possible to just convert with_stdout to with_stdout_data, such as the error reported below:

--- Expected
++++ actual:   stdout
   1      - /home/heisen/temp/temp/temp/cargo/target/debug/cargo∅
        1 + /home/heisen/temp/temp/temp/cargo/target/debug/cargo

Need to add line breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding in a newline is trivial and doesn't invalidate the idea I mentioned that this is unlikely to have surrounding text that gets regularly changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been modified and you can see if it meets the requirements.

tests/testsuite/version.rs Outdated Show resolved Hide resolved
@heisen-li heisen-li force-pushed the env_features_target branch 6 times, most recently from 204d05d to c335fb0 Compare June 29, 2024 10:40
tests/testsuite/cargo_command.rs Outdated Show resolved Hide resolved
tests/testsuite/cargo_command.rs Outdated Show resolved Hide resolved
[cargo-new]
vcs = "none"
"#,
[cargo-new]
Copy link
Member

Choose a reason for hiding this comment

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

Why whitespaces changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto. It's normal in my local area.

let mut path = path();
path.push(target_dir.clone());
let path = env::join_paths(path.iter()).unwrap();

cargo_process("envtest")
.env("PATH", &path)
.with_stdout(cargo.to_str().unwrap())
.with_stdout_data(str![[r#"
Copy link
Member

Choose a reason for hiding this comment

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

This asserts an exact value. I don't think we need a snapshot here.

Suggested change
.with_stdout_data(str![[r#"
.with_stdout_data(cargo.to_str())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this modification doesn't work, I tried several different modification options and got different results. I would like to describe it in detail:
1.Try modifying it to:.with_stdout_data(cargo.to_str().unwrap());
The results obtained are:

--- Expected
++++ actual:   stdout
   1      - /home/heisen/temp/temp/temp/cargo/target/debug/cargo∅
        1 + /home/heisen/temp/temp/temp/cargo/target/debug/cargo

Line breaks need to be added. If line breaks are added after: .with_stdout_data(format!("{}\n",cargo.to_str().unwrap()), then the suffix [EXE] needs to be handled. If [EXE] is added, then an error occurs:

--- Expected
++++ actual:   stdout
   1      - \\?\D:\Rust\temp\temp\temp\cargo\target\debug\cargo.exe[EXE]
        1 + //?/D:/Rust/temp/temp/temp/cargo/target/debug/cargo[EXE]

2.Try changing it to .with_stdout_data(cargo.to_str()); it gives an error:

error[E0277]: the trait bound `std::option::Option<&str>: snapbox::IntoData` is not satisfied
  1. Trying to change to .with_stdout_data(envtest_bin) in the following will result in an error:
--- Expected
++++ actual:   stdout
   1      - \\?\D:\Rust\temp\temp\temp\cargo\target\tmp\cit\t0\cargo-envtest\target\debug\cargo-envtest.exe∅
        1 + //?/[ROOT]/cargo-envtest/target/debug/cargo-envtest[EXE]

All I can think of at the moment is to use the current modification or add #[allow(deprecated)].

@@ -385,7 +405,10 @@ fn cargo_subcommand_env() {
cargo_process("envtest")
.env("PATH", &path)
.env(cargo::CARGO_ENV, &envtest_bin)
.with_stdout(envtest_bin)
.with_stdout_data(str![[r#"
Copy link
Member

Choose a reason for hiding this comment

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

ditto. Just check the value against envtest_bin.

tests/testsuite/cargo_command.rs Outdated Show resolved Hide resolved
@ehuss ehuss assigned weihanglo and unassigned ehuss Jul 2, 2024
@@ -341,6 +357,7 @@ fn override_cargo_home() {
assert!(paths::root().join("foo2/.git").is_dir());
}

#[allow(deprecated)]
Copy link
Member

Choose a reason for hiding this comment

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

@weihanglo
Copy link
Member

Looks good. Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 3, 2024

📌 Commit a32be92 has been approved by weihanglo

It is now in the queue for this repository.

@bors 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 Jul 3, 2024
@bors
Copy link
Collaborator

bors commented Jul 3, 2024

⌛ Testing commit a32be92 with merge d1b5f07...

@bors
Copy link
Collaborator

bors commented Jul 3, 2024

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

@bors bors merged commit d1b5f07 into rust-lang:master Jul 3, 2024
22 checks passed
@heisen-li heisen-li deleted the env_features_target branch July 4, 2024 01:42
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 5, 2024
Update cargo

13 commits in a515d463427b3912ec0365d106791f88c1c14e1b..f86ce56396168edf5d0e1c412ddda0b2edba7d46
2024-07-02 20:53:36 +0000 to 2024-07-05 17:52:05 +0000
- test: Migrate jobserver to snapbox (rust-lang/cargo#14191)
- chore(deps): update msrv (3 versions) to v1.77 (rust-lang/cargo#14186)
- test: migrate build_plan and build_script to snapbox (rust-lang/cargo#14193)
- test: migrate cfg and check to snapbox (rust-lang/cargo#14185)
- test: migrate install* and inheritable_workspace_fields to snapbox (rust-lang/cargo#14170)
- Pass rustflags to artifacts built with implicit targets when using target-applies-to-host (rust-lang/cargo#13900)
- test: Migrate network tests to snapbox (rust-lang/cargo#14187)
- test: migrate some files to snapbox (rust-lang/cargo#14113)
- test: Auto-redact `... after last build at ...`; Migrate `freshness` to Snapbox (rust-lang/cargo#14161)
- chore: fix some typos (rust-lang/cargo#14182)
- fix: improve message for inactive weak optional feature with edition2024 through unused dep collection (rust-lang/cargo#14026)
- test:migrate `doc/directory/docscrape` to snapbox (rust-lang/cargo#14171)
- test: Migrate git_auth to snapbox (rust-lang/cargo#14172)
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 S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants