Skip to content

Use HKEY_CURRENT_USER for testing#2513

Merged
jwillemsen merged 2 commits intomasterfrom
jwi-configtest
Mar 11, 2026
Merged

Use HKEY_CURRENT_USER for testing#2513
jwillemsen merged 2 commits intomasterfrom
jwi-configtest

Conversation

@jwillemsen
Copy link
Copy Markdown
Member

@jwillemsen jwillemsen commented Mar 11, 2026

Use HKEY_CURRENT_USER for testing as HKEY_LOCAL_MACHINE can't be changed when this test runs under a normal user. Also use const/addressof in various places

Summary by CodeRabbit

  • Refactor
    • Updated code initialization patterns and formatting standards in configuration modules for improved consistency and maintainability. These are internal improvements with no impact on user-facing functionality.

Use HKEY_CURRENT_USER for testing as HKEY_LOCAL_MACHINE can't be changed when this test runs under a normal user. Also use const/addressof in various places
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 11, 2026

Walkthrough

Code modernization across two Configuration-related files: replacing legacy null-pointer initializations with modern C++ syntax (nullptr, brace-initialization), applying const qualifiers to local variables, and updating Windows registry root key constants from HKEY_LOCAL_MACHINE to HKEY_CURRENT_USER in tests.

Changes

Cohort / File(s) Summary
Core Configuration Modernization
ACE/ace/Configuration.cpp
Replaced zero and null-initializations with nullptr and brace-init syntax ({}). Added const qualifiers to local variables. No functional changes to control flow or public APIs.
Configuration Test Updates
ACE/tests/Config_Test.cpp
Modernized pointer initializations with brace-init syntax, replaced raw address-of operations with std::addressof(), applied const qualifiers to local variables, and switched registry root key from HKEY_LOCAL_MACHINE to HKEY_CURRENT_USER with updated error messages.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Modern pointers hop with grace,
Nullptr and braces take their place,
The registry roots now point just right,
Const and addressof—what a delight!
Old code refined in style so bright! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main change: switching Windows registry tests from HKEY_LOCAL_MACHINE to HKEY_CURRENT_USER, which aligns with the primary objective documented in the PR description.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jwi-configtest

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ACE/tests/Config_Test.cpp`:
- Around line 525-542: The tests are operating on the real HKEY_CURRENT_USER
Software\ACE tree; change them to use a dedicated test-only subtree with a
unique suffix and only remove that subtree. Modify the
RegConfig/test_subkey_path invocation and the calls to
ACE_Configuration_Win32Registry::resolve_key (root and root_fs) to use a path
like "Software\\ACE\\test_<unique>" (generate <unique> from process id or
timestamp) so test_subkey_path(RegConfig) creates and deletes only that unique
subtree; ensure cleanup logic only deletes that unique subtree rather than the
whole "ACE" key.
- Line 746: Remove the trailing whitespace in the test file Config_Test.cpp that
is causing the fuzz pipeline to fail by editing the offending line to remove any
spaces/tabs at its end so the line ends cleanly with just a newline; ensure no
other lines in that vicinity contain trailing whitespace before committing.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: cc0f2ee8-70ba-4486-bf04-85837dffc173

📥 Commits

Reviewing files that changed from the base of the PR and between 86b3dfc and 5f51784.

📒 Files selected for processing (2)
  • ACE/ace/Configuration.cpp
  • ACE/tests/Config_Test.cpp

@jwillemsen jwillemsen merged commit 7dd3780 into master Mar 11, 2026
69 of 70 checks passed
@jwillemsen jwillemsen deleted the jwi-configtest branch March 11, 2026 18:43
@jwillemsen
Copy link
Copy Markdown
Member Author

@Mergifyio backport ace6tao2

@mergify
Copy link
Copy Markdown

mergify bot commented Mar 11, 2026

backport ace6tao2

✅ Backports have been created

Details

Cherry-pick of 5f51784 has failed:

On branch mergify/bp/ace6tao2/pr-2513
Your branch is up to date with 'origin/ace6tao2'.

You are currently cherry-picking commit 5f51784b33.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   ACE/tests/Config_Test.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   ACE/ace/Configuration.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

jwillemsen added a commit to jwillemsen/ATCD that referenced this pull request Mar 12, 2026
    * ACE/tests/Config_Test.cpp:
jwillemsen added a commit that referenced this pull request Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant