Skip to content

test: address five reviewer nits on tst_settings and tst_util#1502

Merged
annejan merged 1 commit into
mainfrom
chore/tst-settings-and-util-nits
May 17, 2026
Merged

test: address five reviewer nits on tst_settings and tst_util#1502
annejan merged 1 commit into
mainfrom
chore/tst-settings-and-util-nits

Conversation

@annejan
Copy link
Copy Markdown
Member

@annejan annejan commented May 17, 2026

Summary

Five of seven recent CodeRabbit findings on `tst_settings.cpp` and `tst_util.cpp` were valid; applying them and skipping two with rationale.

Applied

File Change
`tst_settings.cpp` `getPassStore`: tighten the "plausible path" predicate from `startsWith('/') || contains('/')` (Unix-only, redundant) to `QDir::isAbsolutePath || contains('/') || contains('\\')` so Windows paths with backslashes also satisfy.
`tst_settings.cpp` `setAndGetSavestate`: loop over both `profile1` and `profile2` for the git-options assertions instead of just `profile1`.
`tst_util.cpp` `CHI_SQUARE_PERMISSIVE_THRESHOLD_DF9`: add a comment explaining the threshold (above p=0.995 critical value ~23.59 for df=9).
`tst_util.cpp` `isValidKeyIdInvalid`: collapse the 6-line `ASSUMED_MAX_KEY_ID_LENGTH` / `TOO_LONG_KEY_ID_LENGTH` dance into one `kTooLongKeyIdLength = 41` constexpr.
`tst_util.cpp` `SshAuthSockGuard`: move from anonymous `namespace {}` to named `namespace testutils`. All 7 usage sites qualified.

Skipped, with reason

  • `getPasswordConfigurationDefault` — CodeRabbit wanted to compare against a default-constructed `PasswordConfiguration`. Verified locally: this fails when the developer's persistent `QSettings` hold a non-default config. The test class itself warns "Non-portable mode detected: tests may modify persistent user settings". Keeping the permissive `>= 0` check.
  • `isUseGit` assertion flip — CodeRabbit's rewrite would assert that `isUseGit` auto-detects `.git` regardless of the `default` arg. Verified against `qtpasssettings.cpp:707`: the auto-detect branch is guarded by `&& defaultValue`, so it only fires when `default=true`. Current assertions match production; the rewrite would break them.

Test plan

  • `qmake6 -r && make -j4` clean
  • `tst_settings` — 113/113
  • `tst_util` — 136/136
  • `clang-format` applied

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Improved validation logic for settings configuration testing.
    • Extended test coverage for git-related configuration options.
    • Enhanced test utility organization and code clarity with better documentation and structured naming conventions.

Review Change Stack

Five of seven CodeRabbit findings on tst_settings.cpp and tst_util.cpp
were valid; applying them and skipping two with a rationale.

tst_settings.cpp:

- getPassStore: tighten the "plausible path" predicate. The old
  `startsWith('/') || contains('/')` was redundant and Unix-only;
  switch to `QDir::isAbsolutePath || contains('/') || contains('\\')`
  so Windows paths with backslashes also satisfy the check.
- setAndGetSavestate (profile-keys check): loop over both profile1 and
  profile2 instead of asserting just on profile1. Same git-options
  assertions now cover both entries.

tst_util.cpp:

- CHI_SQUARE_PERMISSIVE_THRESHOLD_DF9: add a comment explaining the
  threshold derivation (above p=0.995 critical value ~23.59 for df=9
  to reduce false failures without losing real bias detection).
- isValidKeyIdInvalid: collapse the 6-line ASSUMED_MAX_KEY_ID_LENGTH /
  TOO_LONG_KEY_ID_LENGTH dance into a single `kTooLongKeyIdLength = 41`
  constexpr with a one-line comment.
- SshAuthSockGuard: move from `namespace {}` to a named
  `namespace testutils`. Qualifies all seven usage sites.

Skipped:

- tst_settings::getPasswordConfigurationDefault: CodeRabbit wanted to
  compare against a default-constructed PasswordConfiguration. Verified:
  this fails when the developer's persistent QSettings hold a non-default
  config (the test class itself warns "Non-portable mode detected: tests
  may modify persistent user settings"). The current `>= 0` check is
  intentionally permissive for non-isolated test runs. Keep it.
- tst_settings isUseGit assertion flip: CodeRabbit's suggested rewrite
  changes the assertions to claim isUseGit auto-detects .git regardless
  of the default arg. Verified against qtpasssettings.cpp:707 — the
  auto-detect branch is guarded by `&& defaultValue`, so it only fires
  when default=true. The current assertions match the production
  behaviour. Suggested rewrite would break them.

Build clean, 113/113 settings + 136/136 util tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 997593e2-9675-465d-9029-38d73ecf281d

📥 Commits

Reviewing files that changed from the base of the PR and between 13f7782 and e8eaf44.

📒 Files selected for processing (2)
  • tests/auto/settings/tst_settings.cpp
  • tests/auto/util/tst_util.cpp

📝 Walkthrough

Walkthrough

This PR refines the QtPass test suite across two files: it strengthens path validation in settings tests and reorganizes utility tests with improved documentation, consolidated boundary constants, and namespace-qualified helper functions.

Changes

Settings and Utility Test Updates

Layer / File(s) Summary
Pass store path validation enhancement
tests/auto/settings/tst_settings.cpp
The getPassStore() test now accepts absolute paths via QDir::isAbsolutePath() or strings containing either / or \ characters, replacing prior logic that only checked leading / or presence of /.
Multi-profile git configuration testing
tests/auto/settings/tst_settings.cpp
The setAndGetMultipleProfiles() test now iterates over both profile1 and profile2, verifying that git-related options (useGit, autoPush, autoPull) exist and default to empty strings for each profile.
Randomness test documentation
tests/auto/util/tst_util.cpp
Clarifying comments are added to explain the permissive chi-square cutoff rationale for the randomness distribution check.
Key ID length boundary constant refactoring
tests/auto/util/tst_util.cpp
The isValidKeyIdInvalid test consolidates boundary constants into a single kTooLongKeyIdLength = 41 constant for over-length rejection validation.
SSH guard namespace reorganization
tests/auto/util/tst_util.cpp
The SshAuthSockGuard helper is moved from an anonymous namespace to a named testutils namespace, and all SSH_AUTH_SOCK override and initialization tests are updated to reference testutils::SshAuthSockGuard.

Possibly related PRs

  • IJHack/QtPass#1102: Updates tests/auto/util/tst_util.cpp around isValidKeyIdInvalid key-id-length boundary constants.
  • IJHack/QtPass#1106: Modifies isValidKeyIdInvalid() test key-id-length boundary constants and documentation.
  • IJHack/QtPass#1155: Updates setAndGetMultipleProfiles() test to assert default values for git-related profile fields.

Suggested labels

size:XS

Poem

🐰 A whisper through the warren fair,
Where tests now dance with greater care—
Paths validated broad and true,
Git options checked for each, on cue,
Namespaces tidy, constants lean,
The finest test suite ever seen!


🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the main change: addressing reviewer feedback on two test files (tst_settings and tst_util), which aligns directly with the PR content.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/tst-settings-and-util-nits

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.

@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 55.418%. remained the same — chore/tst-settings-and-util-nits into main

@annejan annejan merged commit 024a5de into main May 17, 2026
24 of 25 checks passed
@annejan annejan deleted the chore/tst-settings-and-util-nits branch May 17, 2026 18:49
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.

2 participants