Integrate snapbox in with cargo-test-support #10581
Conversation
|
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| pub fn cargo_exe() -> PathBuf { | ||
| cargo_dir().join(format!("cargo{}", env::consts::EXE_SUFFIX)) | ||
| snapbox::cmd::cargo_bin("cargo") |
There was a problem hiding this comment.
This doesn't seem to be implemented quite the same. Is there a particular reason that it doesn't use CARGO_BIN_PATH? Making assumptions around current_exe seems potentially risky, and something we'd like to discourage.
There was a problem hiding this comment.
I'm not seeing anything set CARGO_BIN_PATH unless that is an undocumented escape hatch for developers to override where it comes from. I just double checked that its not being used on my system and that instead we are falling back to the current_exe trick that already exists in cargo-test-support:
[crates/cargo-test-support/src/lib.rs:475] env::var_os("CARGO_BIN_PATH") = None
[crates/cargo-test-support/src/lib.rs:483] path = "/home/epage/src/personal/cargo/target/debug"
There was a problem hiding this comment.
Oh, I guess CARGO_BIN_PATH is an old thing from long ago. I guess it could be used to test with a different cargo, but I've never used that. I was thinking it was CARGO_BIN_EXE_cargo, which I thought we switched to long ago, but I guess we didn't (and I don't think it works anyways since this code lives in a dependency).
| /// # Patterns | ||
| /// | ||
| /// - `[..]` is a character wildcard, stopping at lin breaks | ||
| /// - `\n...\n` is a line wildcard |
There was a problem hiding this comment.
It's not clear to me, why would one use this instead of [..]?
There was a problem hiding this comment.
\n...\n` is a line wildcard` was clarified to \n...\n is a multi-line wildcard
A line with just [..] will only match that one line. \n...\n will match multiple lines. This is helpful if you don't know how many lines you need elided (e.g. it changes based on the platform)
Cargo add support for workspace inheritance Tracking issue: #8415 RFC: rust-lang/rfcs#2906 This PR adds all the required support for workspace inheritance within `cargo-add`. It was split up across a few different PRs as it required `snapbox` support from #10581 and a new `toml_edit` version from #10603. `@epage` and I decided to go ahead with this PR and add in some of the changes those PRs made. `@epage's` name on the commits is from helping to rewrite commits and some very minor additions. Changes: - #10585 - Muscraft#1 - Muscraft#3 - Muscraft#2 - Muscraft#4 r? `@epage`
This was written for `cargo_add.rs`, based on `snapbox`. This allows creating a test from a known reproduction case or easily debugging on an existing test case.
This is something the existing test infrastructure supports, so I figured I'd make it mirror it for snapbox. I'm mixed. - It reads more like what a user would type, making it easier to run a test locally or take a manual test case and automate it - It can make it harder to parse the arguments when scanning tests - Without using a crate like `shlex`, the syntax support is unclear
|
@bors r+ |
|
📌 Commit d6e912c has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 7 commits in f63f23ff1f1a12ede8585bbd1bbf0c536e50293d..a44758ac805600edbb6ba51e7e6fb81a6077c0cd 2022-04-28 03:15:50 +0000 to 2022-05-04 02:29:34 +0000 - Add support for `-Zbuild-std` to `cargo fetch` (rust-lang/cargo#10129) - Migrate tests of `cargo-init` to snapbox (rust-lang/cargo#10620) - dedupe toml_edit crate, followup rust-lang/cargo#10603 (rust-lang/cargo#10619) - Update GitHub Actions actions/checkout@v2 to v3 (rust-lang/cargo#10618) - Integrate snapbox in with cargo-test-support (rust-lang/cargo#10581) - Fix zsh completion (rust-lang/cargo#10613) - Update documentation for workspace inheritance (rust-lang/cargo#10611)
What does this PR try to resolve?
#10472 introduced snapbox to cargo's tests in the least intrusive manner by copying some cargo-test-support code. Primarily, this PR works to de-duplicate that code. Secondarily, it makes it possible for snapbox to be used by other cargo tests that can work with its more limited functionality compared to cargo-test-support.
How should we test and review this PR?
This is broken down by commits for smaller chunks to look over with some extra details in some of the commit messages.
As this is effectively refactoring existing tests, them passing is sufficient for testing. The main focus would be on any API design including if there are any practices that we used to do that this continues forward to snapbox that we shouldn't.
Additional information
The cargo contributing guide also needs to be updated but I'm leaving that off for another PR once this is merged so we have a clearer idea of what the API will look like (less churn) and so we can focus the conversation for each PR.