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

Add a test for regressions in selecting the correct workspace root #10862

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

Muscraft
Copy link
Member

This adds a test to check for regressions in selecting the correct workspace when there are nested workspaces.

#10846 solved a problem with nested workspace resolution that was caused by #10776. @ehuss suggested that a test should be added to ensure that this issue does not pop up again.

I ensured that this worked by testing against commit before #10846. Sporadically I would get an error that was the same as described in #10846.

error: package `{path}/cargo/target/tmp/cit/t0/foo/sub/foo/Cargo.toml` is a member of the wrong workspace
expected: {path}/cargo/target/tmp/cit/t0/foo/sub/Cargo.toml
actual:   {path}/cargo/target/tmp/cit/t0/foo/Cargo.toml

I then tested it on the commit with the fix and the test passed every time.


While this does add a test to catch any regression I am worried that it will not catch it every time. It was noted in #10846 that this error would sometimes happen but not every time, in my testing I found this to be true as well. Since this is caused by the HashMap order changing each run, switching to something ordered like BTreeMap should catch any regressions every run (if the implementation were to ever change). I'm not sure if this is necessary so I figured I would note the concern here.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 14, 2022
@epage
Copy link
Contributor

epage commented Jul 14, 2022

Thanks for doing this! I had considered pushing for a test in the previous PR but due to the sporadic nature, I was concerned about blocking on the PR for perfection when the fix itself was needed.

@epage
Copy link
Contributor

epage commented Jul 14, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jul 14, 2022

📌 Commit 6070b0a has been approved by epage

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 14, 2022
@bors
Copy link
Contributor

bors commented Jul 14, 2022

⌛ Testing commit 6070b0a with merge 8827baa...

@bors
Copy link
Contributor

bors commented Jul 14, 2022

☀️ Test successful - checks-actions
Approved by: epage
Pushing 8827baa to master...

@bors bors merged commit 8827baa into rust-lang:master Jul 14, 2022
@Muscraft Muscraft deleted the workspace-selection-test branch July 14, 2022 03:40
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2022
Update cargo

7 commits in b1dd22e668af5279e13a071ad4b17435bd6bfa4c..8827baaa781b37872134c1ba692a6f0aeb37890e
2022-07-09 14:48:50 +0000 to 2022-07-14 02:56:51 +0000
- Add a test for regressions in selecting the correct workspace root (rust-lang/cargo#10862)
- clarify profile used for 'cargo install --debug' (rust-lang/cargo#10861)
- servers should use 404 (rust-lang/cargo#10860)
- test(add): Ensure comments are preserved (rust-lang/cargo#10849)
- Fix nested workspace resolution (rust-lang/cargo#10846)
- Small tweaks to the future-incompat docs. (rust-lang/cargo#10856)
- Fixed extra period typo (rust-lang/cargo#10847)
@ehuss ehuss added this to the 1.64.0 milestone Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants