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

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>

Author: annejan

tests/auto/settings/tst_settings.cpp

@@ -158,8 +158,9 @@ void tst_settings::setAndGetGeometry() {
 
 void tst_settings::getPassStore() {
   QString store = QtPassSettings::getPassStore();
-  QVERIFY2(store.isEmpty() || store.startsWith("/") || store.contains("/"),
-           "Pass store should be empty or a plausible path");
+  const bool plausiblePath = store.isEmpty() || QDir::isAbsolutePath(store) ||
+                             store.contains('/') || store.contains('\\');
+  QVERIFY2(plausiblePath, "Pass store should be empty or a plausible path");
 }
 
 void tst_settings::setAndGetPassStore() {
@@ -512,18 +513,21 @@ void tst_settings::setAndGetMultipleProfiles() {
   QVERIFY(readProfiles.contains("profile2"));
   QVERIFY(readProfiles["profile1"].contains("path"));
   QVERIFY(readProfiles["profile2"].contains("path"));
-  // Verify new git options are in profile (issue #112)
-  QVERIFY(readProfiles["profile1"].contains("useGit"));
-  QVERIFY(readProfiles["profile1"].contains("autoPush"));
-  QVERIFY(readProfiles["profile1"].contains("autoPull"));
-  // Verify git options default to empty (unset) - compare QString directly
-  // instead of toInt() which treats "true" as 0
-  QVERIFY2(readProfiles["profile1"].value("useGit").isEmpty(),
-           "useGit should default to empty");
-  QVERIFY2(readProfiles["profile1"].value("autoPush").isEmpty(),
-           "autoPush should default to empty");
-  QVERIFY2(readProfiles["profile1"].value("autoPull").isEmpty(),
-           "autoPull should default to empty");
+  // Verify new git options are in all profiles (issue #112)
+  const QStringList profileNames = {"profile1", "profile2"};
+  for (const QString &profileName : profileNames) {
+    QVERIFY(readProfiles[profileName].contains("useGit"));
+    QVERIFY(readProfiles[profileName].contains("autoPush"));
+    QVERIFY(readProfiles[profileName].contains("autoPull"));
+    // Verify git options default to empty (unset) - compare QString directly
+    // instead of toInt() which treats "true" as 0
+    QVERIFY2(readProfiles[profileName].value("useGit").isEmpty(),
+             "useGit should default to empty");
+    QVERIFY2(readProfiles[profileName].value("autoPush").isEmpty(),
+             "autoPush should default to empty");
+    QVERIFY2(readProfiles[profileName].value("autoPull").isEmpty(),
+             "autoPull should default to empty");
+  }
 }
 
 void tst_settings::profileGitOptions() {

tests/auto/util/tst_util.cpp

@@ -40,6 +40,9 @@ static constexpr int DISTRIBUTION_MAX_PERCENT = 120;
 static constexpr int PERCENT_BASE = 100;
 static constexpr int RANDOMNESS_TEST_SAMPLE_COUNT = 200;
 static constexpr int RANDOMNESS_TEST_PASSWORD_LENGTH = 32;
+// Permissive chi-square cutoff for df=9: set above the p=0.995 critical
+// value (~23.59) to reduce false test failures while still catching
+// meaningful distribution bias.
 static constexpr double CHI_SQUARE_PERMISSIVE_THRESHOLD_DF9 = 30.0;
 
 /**
@@ -1221,19 +1224,12 @@ void tst_util::isValidKeyIdWithEmail() {
 }
 
 void tst_util::isValidKeyIdInvalid() {
-  // NOTE: 40 is intentional and derived from the current Util::isValidKeyId
-  // implementation detail: its key-id regex accepts 8-40 hexadecimal
-  // characters (optionally with a 0x prefix). This test verifies that a
-  // value just above that upper bound is rejected.
-  //
-  // Keep boundary expectations aligned with production validation rules.
-  // This test checks that one character above the configured max is rejected.
-  // If Util::isValidKeyId's accepted range changes, update this constant.
-  constexpr int ASSUMED_MAX_KEY_ID_LENGTH = 40;
-  constexpr int TOO_LONG_KEY_ID_LENGTH = ASSUMED_MAX_KEY_ID_LENGTH + 1;
+  // Boundary check: one character above the currently accepted maximum
+  // hexadecimal key-id length should be rejected.
+  constexpr int kTooLongKeyIdLength = 41; // current max (40) + 1
   QVERIFY(!Util::isValidKeyId(""));
   QVERIFY(!Util::isValidKeyId("short"));
-  QVERIFY(!Util::isValidKeyId(QString(TOO_LONG_KEY_ID_LENGTH, 'a')));
+  QVERIFY(!Util::isValidKeyId(QString(kTooLongKeyIdLength, 'a')));
   QVERIFY(!Util::isValidKeyId("invalidchars!"));
   QVERIFY(!Util::isValidKeyId("space in key"));
 }
@@ -2080,7 +2076,7 @@ void tst_util::grepImitatePassInvalidRegexEmitsEmpty() {
 // RAII helper restores SSH_AUTH_SOCK + the override setting around each test
 // so we don't pollute the developer's environment or each other's state.
 // ---------------------------------------------------------------------------
-namespace {
+namespace testutils {
 class SshAuthSockGuard {
 public:
   SshAuthSockGuard()
@@ -2103,13 +2099,13 @@ class SshAuthSockGuard {
   QByteArray prevEnv_;
   QString prevOverride_;
 };
-} // namespace
+} // namespace testutils
 
 /**
  * @brief getter/setter roundtrip for sshAuthSockOverride.
  */
 void tst_util::sshAuthSockOverrideRoundtrip() {
-  SshAuthSockGuard guard;
+  testutils::SshAuthSockGuard guard;
   const QString sentinel =
       QStringLiteral("/tmp/qtpass-test-sock-") + QUuid::createUuid().toString();
   QtPassSettings::setSshAuthSockOverride(sentinel);
@@ -2122,7 +2118,7 @@ void tst_util::sshAuthSockOverrideRoundtrip() {
  * @brief default value of getSshAuthSockOverride is empty.
  */
 void tst_util::sshAuthSockOverrideEmptyByDefault() {
-  SshAuthSockGuard guard;
+  testutils::SshAuthSockGuard guard;
   QtPassSettings::setSshAuthSockOverride(QString{});
   QCOMPARE(QtPassSettings::getSshAuthSockOverride(), QString{});
 }
@@ -2132,7 +2128,7 @@ void tst_util::sshAuthSockOverrideEmptyByDefault() {
  *        already set.
  */
 void tst_util::initialiseSshAuthSockHonoursExistingEnv() {
-  SshAuthSockGuard guard;
+  testutils::SshAuthSockGuard guard;
   const QByteArray existing("/tmp/qtpass-existing-sock");
   qputenv("SSH_AUTH_SOCK", existing);
   // Even if an override is configured, the existing env wins.
@@ -2147,7 +2143,7 @@ void tst_util::initialiseSshAuthSockHonoursExistingEnv() {
  *        unset.
  */
 void tst_util::initialiseSshAuthSockUsesOverride() {
-  SshAuthSockGuard guard;
+  testutils::SshAuthSockGuard guard;
   qunsetenv("SSH_AUTH_SOCK");
   const QString override = QStringLiteral("/tmp/qtpass-override-sock");
   QtPassSettings::setSshAuthSockOverride(override);
@@ -2162,7 +2158,7 @@ void tst_util::initialiseSshAuthSockUsesOverride() {
  *        the resulting state is consistent.
  */
 void tst_util::initialiseSshAuthSockNoOverrideNoEnvProbes() {
-  SshAuthSockGuard guard;
+  testutils::SshAuthSockGuard guard;
   qunsetenv("SSH_AUTH_SOCK");
   QtPassSettings::setSshAuthSockOverride(QString{});
   Util::initialiseSshAuthSock();
@@ -2184,7 +2180,7 @@ void tst_util::initialiseSshAuthSockNoOverrideNoEnvProbes() {
  *        the user starts their agent after launching QtPass).
  */
 void tst_util::initialiseSshAuthSockOverrideSkipsAgentValidation() {
-  SshAuthSockGuard guard;
+  testutils::SshAuthSockGuard guard;
   qunsetenv("SSH_AUTH_SOCK");
   // A path that almost certainly has no listener — validation would fail.
   // Override should win regardless.
@@ -2201,7 +2197,7 @@ void tst_util::initialiseSshAuthSockOverrideSkipsAgentValidation() {
  *        it should fall through to the auto-probe.
  */
 void tst_util::initialiseSshAuthSockEmptyOverrideFallsThrough() {
-  SshAuthSockGuard guard;
+  testutils::SshAuthSockGuard guard;
   qunsetenv("SSH_AUTH_SOCK");
   QtPassSettings::setSshAuthSockOverride(QString{});
   Util::initialiseSshAuthSock();