Skip to content

fix(test): test delete Windows registry accidently - #4918

Open
baka-gourd wants to merge 5 commits into
rust-lang:mainfrom
baka-gourd:windows-registry-per-test-uuid
Open

fix(test): test delete Windows registry accidently#4918
baka-gourd wants to merge 5 commits into
rust-lang:mainfrom
baka-gourd:windows-registry-per-test-uuid

Conversation

@baka-gourd

Copy link
Copy Markdown

fix #4915

This PR is intended to fix the issue mentioned in the ticket regarding the accidental deletion of existing registry entries. However, while investigating the problem, we also discovered a concurrency issue in RegistryGuard, so the scope of the fix is quite extensive.

  • Add per-test Windows registry isolation using a unique test UUID.
  • Route Windows registry helpers through test-specific registry subkeys when a UUID is present.
  • Update RegistryGuard to support multiple registry values and clean up the per-test registry subtree.
  • Forward the registry UUID through the test Config and CliTestContext.
  • Update Windows unit and integration tests to use UUID-scoped registry guards.
  • Keep PATH registry checks and Programs entry checks isolated in tests.
  • Treat missing registry keys or values as non-fatal during registry cleanup.
  • No new UUID dependency is introduced; UUIDs are generated from rand.

@baka-gourd
baka-gourd force-pushed the windows-registry-per-test-uuid branch from a758fde to c4635ae Compare June 21, 2026 13:53
Comment thread src/cli/self_update/windows.rs Outdated
@Cloud0310

Copy link
Copy Markdown
Contributor

Another thing here is, can we make code format coming along with commits instead of doing formatting in the end commit here.

@rustbot

This comment has been minimized.

@baka-gourd
baka-gourd force-pushed the windows-registry-per-test-uuid branch from 9be4399 to 360e048 Compare July 2, 2026 12:21
@rustbot

This comment has been minimized.

@baka-gourd
baka-gourd requested a review from Cloud0310 July 2, 2026 12:54
@rami3l rami3l self-assigned this Jul 3, 2026
@rustbot

This comment has been minimized.

@rami3l
rami3l requested review from rami3l and removed request for Cloud0310 August 3, 2026 14:31
Comment thread src/cli/self_update/windows.rs Outdated
@baka-gourd
baka-gourd force-pushed the windows-registry-per-test-uuid branch from 360e048 to dcd1d78 Compare August 5, 2026 11:20
@rustbot

This comment has been minimized.

Comment thread src/cli/self_update.rs
Comment thread src/cli/self_update.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/test/clitools.rs Outdated

@rami3l rami3l left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@baka-gourd I think this looks quite interesting but would require more work before it can be merged.

Three general principles of our committing style:

  1. Each commit should do one single thing, whether that is refactoring, introducing a breaking change, or adding a new feature.
  2. Each commit should ensure that all test cases are passing. If running all tests is not practical in your local environment, you should have run at least all related tests and have made sure that they are passing.
  3. When a commit indicates a behavioral change that might cause some tests to fail, you should update those influenced tests in the same commit so that they pass again.

View changes since this review

@baka-gourd
baka-gourd force-pushed the windows-registry-per-test-uuid branch from 6366e7f to 78a2fde Compare August 5, 2026 15:06
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
@baka-gourd
baka-gourd force-pushed the windows-registry-per-test-uuid branch 3 times, most recently from 5bccc07 to 25f7553 Compare August 6, 2026 13:50
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread tests/suite/cli_paths.rs Outdated
Comment thread tests/suite/cli_paths.rs Outdated
Comment thread src/test/clitools.rs Outdated
Comment thread src/test/clitools.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
@baka-gourd
baka-gourd force-pushed the windows-registry-per-test-uuid branch 2 times, most recently from 517c610 to 91589c2 Compare August 6, 2026 14:48
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs
@baka-gourd

Copy link
Copy Markdown
Author

The CI error isn't my fault; CI pulled the wrong commit😓

@Cloud0310

Copy link
Copy Markdown
Contributor

Lmao, a re-run seems don't fix up the problem with buggy github actions. 😆

@rami3l

rami3l commented Aug 6, 2026

Copy link
Copy Markdown
Member

@baka-gourd I think a rebase would help though.

@baka-gourd
baka-gourd force-pushed the windows-registry-per-test-uuid branch from 91589c2 to e40d5d6 Compare August 7, 2026 04:52
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
Comment thread src/test/clitools.rs Outdated
Comment thread src/test/clitools.rs Outdated
Comment thread tests/suite/cli_paths.rs
@baka-gourd
baka-gourd force-pushed the windows-registry-per-test-uuid branch from e40d5d6 to 4baac20 Compare August 7, 2026 09:37
@rustbot

rustbot commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rami3l rami3l left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM and thank you so much :)

View changes since this review

@baka-gourd

Copy link
Copy Markdown
Author
-let environment = process.registry_environment_key()?;
+let environment = process.registry_key("Environment", CURRENT_USER)?;

I don't think that's a good idea. Even though it only has one function, using inline code will lead to a lot of duplication, which makes it harder to modify later.

@rami3l

rami3l commented Aug 7, 2026

Copy link
Copy Markdown
Member
-let environment = process.registry_environment_key()?;

+let environment = process.registry_key("Environment", CURRENT_USER)?;

I don't think that's a good idea. Even though it only has one function, using inline code will lead to a lot of duplication, which makes it harder to modify later.

Sorry there might be some GitHub availability issues going on; I'm okay with both the inlined and the non-inlined versions of this call, as long as it's consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Current tests run on Windows will accidently clears installed registry values by normal installation of rustup

4 participants