test(cli): Track --help output #11912

epage · 2023-03-29T21:31:56Z

What does this PR try to resolve?

This makes it easier to evaluate the usability of PRs, like #11905

This follows the pattern of cargo add and cargo remove of putting these ui tests in cargo_<cmd>/ directories. init didn't follow this pattern, so it was renamed to cargo_init/. cargo_config.rs was going to conflict with this, it was merged in.

We can evaluate other <cmd>.rs files at a later point and consolidate.

How should we test and review this PR?

The main risks are

Are all files linked together (main.rs -> <cmd>/mod.rs -> <cmd>/<help>.rs
Are all help/mod.rss pointing to the right command

rustbot · 2023-03-29T21:32:04Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

ehuss · 2023-03-31T13:43:35Z

I'm a little concerned that this introduces two modules for almost every command, and it may not be clear how to manage tests with that structure. That is, there is a yank.rs and a cargo_yank/mod.rs, and it may not be clear why that separation is made, or guidance on how to organize tests. Is there some way we can avoid that separation? What about moving the corresponding yank.rs to cargo_yank/mod.rs?

ehuss · 2023-03-31T13:44:47Z

tests/testsuite/cargo/help/mod.rs

@@ -0,0 +1,12 @@
+use cargo_test_support::curr_dir;


Just curious, is there a reason curr_dir is not in cargo_test_support::prelude?

Not really. I can add it if wanted.

That might help with some of the boilerplate. It looks like it is used for essentially every UI test. This is unrelated to this PR, though, and can be done any time.

epage · 2023-03-31T16:01:25Z

I'm a little concerned that this introduces two modules for almost every command, and it may not be clear how to manage tests with that structure. That is, there is a yank.rs and a cargo_yank/mod.rs, and it may not be clear why that separation is made, or guidance on how to organize tests. Is there some way we can avoid that separation? What about moving the corresponding yank.rs to cargo_yank/mod.rs?

I had originally wanted to punt on yank.rs vs cargo_yank.rs for now and called this out in the PR description. My assumption in doing so is there would be some untangling of relationships that might not be obvious. Also my primary goal is implementing #11905. and I didn't want to get too distracted with other steps along the way.

ehuss · 2023-03-31T16:49:16Z

If you want to defer on figuring out the module structure, would it be possible to just add the tests to the existing modules in the mean time? I assume stdout_matches_path can point to some directory with a more manual path (like curr_dir!().join("cargo_yank/help/stdout.log"))?

I would prefer to avoid moving forward with something that puts it in an incomplete state without some idea or strategy for where it is intended to end up.

epage · 2023-03-31T20:26:47Z

If you want to defer on figuring out the module structure, would it be possible to just add the tests to the existing modules in the mean time? I assume stdout_matches_path can point to some directory with a more manual path (like curr_dir!().join("cargo_yank/help/stdout.log"))?

This does not get us out of being in an inconsistent, intermediate state since this would then make the UI tests inconsistent with each other. Since those all follow a very similar pattern, I would like to keep them as such for easily modifying them in the future.

I would prefer to avoid moving forward with something that puts it in an incomplete state without some idea or strategy for where it is intended to end up.

Except thats what software evolution is, is to always be in some kind of incomplete state as you work towards your ideal. To take a stance you can't improve anything until everything is improved leaves you always in a worse place than small, incremental progress.

This was meant to be the simple, quick PR to unblock other help work. I feel like these discussions are already exceeding a reasonable time box for that scope but stalls out future improvements in other areas.

bors · 2023-05-23T14:04:25Z

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

This is split out of rust-lang#11912 and is prep for adding more UI tests. Generally our UI tests are in a directory named after the full cargo command (`cargo config`). These tend to use `snapbox`. Here we are tests for the `cargo config` command not written by `snapbox` in a `cargo_config.rs` file. This conflicts with adding snapbox UI tests later in a `cargo_config/` folder. Upon looking at this file, it appears to be UI tests, so I think it would make sense to move them into the `cargo_config/` folder. Definitely wouldn't make sense to move them into `config.rs` since that is general config testing.

refactor(test): Move cargo-config into a dir This is split out of #11912 and is prep for adding more UI tests. Generally our UI tests are in a directory named after the full cargo command (`cargo config`). These tend to use `snapbox`. Here we are tests for the `cargo config` command not written by `snapbox` in a `cargo_config.rs` file. This conflicts with adding snapbox UI tests later in a `cargo_config/` folder. Upon looking at this file, it appears to be UI tests, so I think it would make sense to move them into the `cargo_config/` folder. Definitely wouldn't make sense to move them into `config.rs` since that is general config testing.

This makes it easier to evaluate the usability of PRs, like rust-lang#11905

epage · 2023-07-24T22:16:25Z

Discussion on test organization has moved to https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Brainstorming.20on.20test.20organization with the consent of @ehuss

epage · 2023-07-24T23:43:00Z

@bors r=ehuss

With the above Zulip discussion, @ehuss told me to merge this once it was ready

bors · 2023-07-24T23:43:01Z

📌 Commit 5101372 has been approved by ehuss

It is now in the queue for this repository.

bors · 2023-07-24T23:43:07Z

⌛ Testing commit 5101372 with merge e2fbcd9...

bors · 2023-07-25T00:26:42Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing e2fbcd9 to master...

Update cargo 14 commits in 7ac9416d82cd4fc5e707c9ec3574d22dff6466e5..c91a693e7977e33a1064b63a5daf5fb689f01651 2023-07-24 14:29:38 +0000 to 2023-07-31 00:26:46 +0000 - fix: align `cargo --help` text (rust-lang/cargo#12418) - fix: normalize relative git submodule urls with `ssh://` (rust-lang/cargo#12411) - test: relax help text assertion (rust-lang/cargo#12416) - test: relax assertions of panic handler message format (rust-lang/cargo#12413) - fix(package): Recognize and normalize `cargo.toml` (rust-lang/cargo#12399) - Clarify `lto` setting passing `-Clinker-plugin-lto` (rust-lang/cargo#12407) - doc: add missing reference to `CARGO_PROFILE_<name>_STRIP` in environment variables docs (rust-lang/cargo#12408) - Update curl-sys to pull in curl 8.2.1 (rust-lang/cargo#12406) - docs: raise awareness of resolver used inside workspace (rust-lang/cargo#12388) - chore: update `home` to 0.5.7 (rust-lang/cargo#12401) - Update curl-sys to pull in curl 8.2.0 (rust-lang/cargo#12400) - test(cli): Track --help output (rust-lang/cargo#11912) - refactor(test): Move cargo-config into a dir (rust-lang/cargo#12398) - refactor(tests): Name init ui tests more consistently (rust-lang/cargo#12397) r? `@ghost`

rustbot assigned ehuss Mar 29, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2023

epage force-pushed the help branch 3 times, most recently from 7e5a07b to e5b9f23 Compare March 31, 2023 12:23

ehuss reviewed Mar 31, 2023

View reviewed changes

weihanglo mentioned this pull request Apr 21, 2023

--help output use line wrap #12013

Merged

epage mentioned this pull request May 8, 2023

Make cargo --list combine commands and their aliases #12093

Closed

This was referenced May 18, 2023

Tweak build help to clarify role of --bin #12157

Merged

fix(cli): Make --help easier to browse #11905

Merged

epage force-pushed the help branch from e5b9f23 to 0e6ad5b Compare June 6, 2023 21:39

epage mentioned this pull request Jul 24, 2023

refactor(tests): Name init ui tests more consistently #12397

Merged

epage mentioned this pull request Jul 24, 2023

refactor(test): Move cargo-config into a dir #12398

Merged

test(cli): Track --help output

5101372

This makes it easier to evaluate the usability of PRs, like rust-lang#11905

epage force-pushed the help branch from 0e6ad5b to 5101372 Compare July 24, 2023 22:15

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 24, 2023

bors merged commit e2fbcd9 into rust-lang:master Jul 25, 2023
19 checks passed

epage deleted the help branch July 25, 2023 00:39

weihanglo mentioned this pull request Jul 31, 2023

Update cargo rust-lang/rust#114277

Merged

ehuss added this to the 1.73.0 milestone Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(cli): Track --help output #11912

test(cli): Track --help output #11912

epage commented Mar 29, 2023

rustbot commented Mar 29, 2023

ehuss commented Mar 31, 2023

ehuss Mar 31, 2023

epage Mar 31, 2023

ehuss Mar 31, 2023

epage commented Mar 31, 2023

ehuss commented Mar 31, 2023

epage commented Mar 31, 2023

bors commented May 23, 2023

epage commented Jul 24, 2023

epage commented Jul 24, 2023

bors commented Jul 24, 2023

bors commented Jul 24, 2023

bors commented Jul 25, 2023

test(cli): Track --help output #11912

test(cli): Track --help output #11912

Conversation

epage commented Mar 29, 2023

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Mar 29, 2023

ehuss commented Mar 31, 2023

ehuss Mar 31, 2023

Choose a reason for hiding this comment

epage Mar 31, 2023

Choose a reason for hiding this comment

ehuss Mar 31, 2023

Choose a reason for hiding this comment

epage commented Mar 31, 2023

ehuss commented Mar 31, 2023

epage commented Mar 31, 2023

bors commented May 23, 2023

epage commented Jul 24, 2023

epage commented Jul 24, 2023

bors commented Jul 24, 2023

bors commented Jul 24, 2023

bors commented Jul 25, 2023