feat: Use test name for dir when running tests #11738

Muscraft · 2023-02-18T22:20:11Z

Tests for Cargo currently put their generated data in target/tmp/cit/t{}. This can make it very hard to debug tests if multiple are falling. Having the tests be named would make this much easier when debugging.

This comment explains why cargo used a number instead of the test name for the unique identifier. I found a way to get the test name to be the unique identifier. I did this by having cargo-test-macro get the name of the test when the macro is looking for where the function body starts. It passes the name of the test and the file that the test comes from into the boilerplate it was already adding.

It uses the file and the test name as the directory to place the tests generated data data into, i.e, target/tmp/cit/testsuite/workspaces/workspace_in_git.

Note: I also found t{} to be frustrating when trying to get the size of a test. There is no good way to get the size of a test within target/tmp/cit, without running the tests one at a time and checking the size like so:

for test in tests:
    subprocess.run(["cargo", "test", "-p", "cargo", test, "--", "--exact"])
    test_size = subprocess.run(["du", "-sk", "target/tmp/cit/t0"], stdout=subprocess.PIPE, text=True)
    size_dict[test] = int(test_size.stdout.split()[0])

It made it very hard to try and find which tests were causing most of the size of the target dir.

rustbot · 2023-02-18T22:20:15Z

r? @epage

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

tests/testsuite/build_script.rs

weihanglo

Thank you. The tests output looks pretty good!

crates/cargo-test-macro/src/lib.rs

weihanglo · 2023-02-19T12:20:56Z

Could you update relevant doc here as wellhttps://doc.crates.io/contrib/tests/writing.html? Thanks!

Cleanup tests When working on #11738 I noticed a few tests that needed to be cleaned up. It [was suggested](#11738 (comment)) to split the test changes into their own PR. - `bad_config::bad1` - This was trying to match `config` which would get matched in the wrong place due to the test name having `config` in it - Fixed by making the match `/config` - `build_script::panic_abort_with_build_scripts` - This test was failing as it was making sure `panic` was not in the output. After this change, it could match the test name and fail. - To fix this I updated it to look at `panic=abort` which appears to be what it was originally looking for (#5711). `@ehuss` please let me know if I misread what the test was testing. - `cargo_command::cargo_subcommand_args` - This test had `#[test]` above `#[cargo_test]` which caused it to throw an error removing it fixed the issue During this time I also ran into issues with `cargo_remove` tests being named `fn case()` which is different from how tests are normally named. I talked with `@epage` and it was decided that `fn case()` should probably be used for all `snapbox` tests since the unique test name is already the folder name. I went ahead and renamed all tests within `cargo_add/` and `init/` to match this style. This PR should be reviewed commit by commit.

crates/cargo-test-macro/src/lib.rs

weihanglo

This looks good to me btw. Thank you for putting efforts on this!

crates/cargo-test-macro/src/lib.rs

tests/testsuite/main.rs

weihanglo

Looks nice! Thank you.

epage · 2023-02-28T15:07:09Z

@bors r+

bors · 2023-02-28T15:07:13Z

📌 Commit 835117d has been approved by epage

It is now in the queue for this repository.

bors · 2023-02-28T15:07:21Z

⌛ Testing commit 835117d with merge c06fb5f...

feat: Use test name for dir when running tests Tests for Cargo currently put their generated data in `target/tmp/cit/t{}`. This can make it very hard to debug tests if multiple are falling. Having the tests be named would make this much easier when debugging. [This comment](https://github.com/rust-lang/cargo/blob/eff8d693b99077b0b7f70e4bc414fdca53597085/crates/cargo-test-support/src/paths.rs#L54-L64) explains why cargo used a number instead of the test name for the unique identifier. I found a way to get the test name to be the unique identifier. I did this by having `cargo-test-macro` get the name of the test when the macro is looking for where the function body starts. It passes the name of the test and the file that the test comes from into the boilerplate it was already adding. It uses the file and the test name as the directory to place the tests generated data data into, i.e, `target/tmp/cit/testsuite/workspaces/workspace_in_git`. ![Screenshot 2023-02-20 at 20 55 30](https://user-images.githubusercontent.com/23045215/220236036-e1dcbe53-033e-4db7-a6a3-a689eae97386.png) Note: I also found `t{}` to be frustrating when trying to get the size of a test. There is no good way to get the size of a test within `target/tmp/cit`, without running the tests one at a time and checking the size like so: ```python for test in tests: subprocess.run(["cargo", "test", "-p", "cargo", test, "--", "--exact"]) test_size = subprocess.run(["du", "-sk", "target/tmp/cit/t0"], stdout=subprocess.PIPE, text=True) size_dict[test] = int(test_size.stdout.split()[0]) ``` It made it very hard to try and find which tests were causing most of the size of the `target` dir.

bors · 2023-02-28T15:40:32Z

💔 Test failed - checks-actions

bors · 2023-03-01T18:39:00Z

⌛ Testing commit 019aeed with merge 64b0e79...

ehuss · 2023-03-01T19:24:51Z

I'd like to mention that this will make development more difficult for my workflow. I usually leave a terminal in the t0 directory, and run a single test, and then interact with it. With this change, I now need to figure out which directory the test is going to be placed, cd into it, and reset my environment variables each time I go to a different test. I can perhaps make a script to do that, looking at the most recently modified directory, but that adds friction to the process.

I understand there are benefits here, but I wanted to highlight that it will cause some difficulties.

bors · 2023-03-01T19:26:55Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 64b0e79 to master...

epage · 2023-03-01T19:36:10Z

I think this is a case where we can't satisfy everyone's workflow. I've found the current approach is hard to work as go to inspect a specific failure from a test run, I can't predict what directory will be used. Now, I can always predict it. This also opens us up to test parallelism (#5609) once its implemented and to cargo nextest in the short term.

23 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..c1334b059c6dcceab3c10c81413f79bb832c8d9d 2023-02-28 19:39:39 +0000 to 2023-03-07 19:21:50 +0000 - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) Note that some 3rd-party licensing allowed list changed due to the introducion of `gix` dependency

This reverts commit 64b0e79, reversing changes made to 9580786.

Revert "#11738" - Use test name for dir when running tests

2 commits in 1334b059c6dcceab3c10c81413f79bb832c8d9d..7d3033d2e59383fd76193daf9423c3d141972a7d 2023-03-07 19:21:50 +0000 to 2023-03-08 17:05:08 +0000 - Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810)

Update cargo 25 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..7d3033d2e59383fd76193daf9423c3d141972a7d 2023-02-28 19:39:39 +0000 to 2023-03-08 17:05:08 +0000 - Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810) - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) --- ~~This update is primarily for making rust-lang/cargo#11630 into 1.69~~ (will file a beta backport then). However, just look into the licenses and dependencies permitted list, it looks a bit unfortunate but inevitable I guess? r? `@ehuss` cc `@Muscraft`

rustbot assigned epage Feb 18, 2023

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 Feb 18, 2023

epage reviewed Feb 19, 2023

View reviewed changes

tests/testsuite/build_script.rs Outdated Show resolved Hide resolved

weihanglo reviewed Feb 19, 2023

View reviewed changes

crates/cargo-test-macro/src/lib.rs Outdated Show resolved Hide resolved

crates/cargo-test-macro/src/lib.rs Outdated Show resolved Hide resolved

Muscraft mentioned this pull request Feb 20, 2023

Cleanup tests #11745

Merged

Muscraft force-pushed the name-tests branch from 420dfcb to fbf7a1b Compare February 21, 2023 02:54

rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Feb 21, 2023

weihanglo reviewed Feb 21, 2023

View reviewed changes

crates/cargo-test-macro/src/lib.rs Outdated Show resolved Hide resolved

crates/cargo-test-macro/src/lib.rs Outdated Show resolved Hide resolved

weihanglo approved these changes Feb 21, 2023

View reviewed changes

epage reviewed Feb 23, 2023

View reviewed changes

crates/cargo-test-macro/src/lib.rs Outdated Show resolved Hide resolved

epage reviewed Feb 23, 2023

View reviewed changes

crates/cargo-test-macro/src/lib.rs Outdated Show resolved Hide resolved

epage mentioned this pull request Feb 24, 2023

Make some blocking tests non-blocking #11650

Merged

Muscraft force-pushed the name-tests branch from fbf7a1b to 4e64b2b Compare February 24, 2023 18:33

epage reviewed Feb 24, 2023

View reviewed changes

tests/testsuite/main.rs Outdated Show resolved Hide resolved

Muscraft force-pushed the name-tests branch from 4e64b2b to 719a4cd Compare February 24, 2023 21:12

weihanglo approved these changes Feb 25, 2023

View reviewed changes

Muscraft force-pushed the name-tests branch from 719a4cd to 835117d Compare February 27, 2023 16:27

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 Feb 28, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 28, 2023

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 Mar 1, 2023

bors merged commit 64b0e79 into rust-lang:master Mar 1, 2023

Muscraft deleted the name-tests branch March 1, 2023 19:29

weihanglo mentioned this pull request Mar 2, 2023

Update cargo rust-lang/rust#108665

Merged

weihanglo added a commit to weihanglo/cargo that referenced this pull request Mar 8, 2023

Revert "Auto merge of rust-lang#11738 - Muscraft:name-tests, r=epage"

be6b6bf

This reverts commit 64b0e79, reversing changes made to 9580786.

weihanglo added a commit to weihanglo/cargo that referenced this pull request Mar 8, 2023

Revert "rust-lang#11738" - Use test name for dir when running tests

f3778f9

This reverts commit 64b0e79, reversing changes made to 9580786.

weihanglo mentioned this pull request Mar 8, 2023

Revert "#11738" - Use test name for dir when running tests #11812

Merged

bors added a commit that referenced this pull request Mar 8, 2023

Auto merge of #11812 - weihanglo:revert-11738, r=epage

7d3033d

Revert "#11738" - Use test name for dir when running tests

ehuss added this to the 1.70.0 milestone Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use test name for dir when running tests #11738

feat: Use test name for dir when running tests #11738

Muscraft commented Feb 18, 2023 •

edited

Loading

rustbot commented Feb 18, 2023

weihanglo left a comment

weihanglo commented Feb 19, 2023

weihanglo left a comment

weihanglo left a comment

epage commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Mar 1, 2023

ehuss commented Mar 1, 2023

bors commented Mar 1, 2023

epage commented Mar 1, 2023

feat: Use test name for dir when running tests #11738

feat: Use test name for dir when running tests #11738

Conversation

Muscraft commented Feb 18, 2023 • edited Loading

rustbot commented Feb 18, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Feb 19, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

epage commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Mar 1, 2023

ehuss commented Mar 1, 2023

bors commented Mar 1, 2023

epage commented Mar 1, 2023

Muscraft commented Feb 18, 2023 •

edited

Loading