Skip to content

test: add charset validation and configIsValid edge cases#999

Merged
annejan merged 2 commits into
mainfrom
fix/test-improvements
Apr 13, 2026
Merged

test: add charset validation and configIsValid edge cases#999
annejan merged 2 commits into
mainfrom
fix/test-improvements

Conversation

@nogeenhenk
Copy link
Copy Markdown
Contributor

@nogeenhenk nogeenhenk commented Apr 13, 2026

User description

Summary

  • Add charset validation in generateRandomPassword to verify generated passwords only contain characters from the specified charset
  • Extend configIsValid test to verify invalid config when .gpg-id exists but git/gpg executable is missing or invalid

These changes address recommendations from code review.


CodeAnt-AI Description

Verify password generation stays within the requested character set and config checks catch missing or invalid executables

What Changed

  • Password tests now confirm every generated character comes from the selected character set
  • Config validation tests now cover cases where a store has a .gpg-id file but the required Git or GPG executable is missing or invalid

Impact

✅ Safer password checks
✅ Fewer broken config cases slipping through
✅ Clearer validation coverage for setup errors

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Summary by CodeRabbit

  • Tests
    • Strengthened password-generation tests by verifying each character comes from the expected charset across multiple scenarios.
    • Expanded configuration validation tests to cover missing and invalid GPG executable cases, and ensure original settings are restored after test.

- Add charset validation in generateRandomPassword to verify generated
  passwords only contain characters from the specified charset
- Extend configIsValid test to verify invalid config when .gpg-id exists
  but git/gpg executable is missing or invalid
@codeant-ai
Copy link
Copy Markdown
Contributor

codeant-ai Bot commented Apr 13, 2026

CodeAnt AI is reviewing your PR.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 13, 2026

📝 Walkthrough

Walkthrough

Added stricter tests in tests/auto/util/tst_util.cpp: per-character charset assertions in generateRandomPassword() and expanded configIsValid() scenarios to exercise missing .gpg-id, empty and nonexistent GPG executable values (with original value restored).

Changes

Cohort / File(s) Summary
Test file
tests/auto/util/tst_util.cpp
Added per-character validation loops for generated passwords across three charset cases. Expanded configIsValid() test: creates .gpg-id, tests empty and nonexistent GPG executable values, uses RAII-style rollback to restore original GPG executable.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐇 I nibble through tests, one hop, then two,

Each char inspected, no mystery in view.
GPG paths checked, then gently restored,
My assertions stand tidy, not a whisker ignored.
Hooray for precise tests — a carrot reward! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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 and accurately summarizes the main changes: adding charset validation to generateRandomPassword tests and extending configIsValid tests with edge cases.

✏️ 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 fix/test-improvements

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.

@codeant-ai codeant-ai Bot added the size:M This PR changes 30-99 lines, ignoring generated files label Apr 13, 2026
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: 1

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

Inline comments:
In `@tests/auto/util/tst_util.cpp`:
- Around line 617-630: The test is mutating the wrong setting and can leak state
on failure: replace the incorrect QtPassSettings::setGitExecutable(QString())
call with QtPassSettings::setGpgExecutable(QString()) when testing missing GPG,
and ensure original values are always restored even if assertions fail by using
a scoped rollback (e.g., QScopedValueRollback<QString> or a try/finally-style
restore) for QtPassSettings::setGpgExecutable and
QtPassSettings::setGitExecutable around the Util::configIsValid() checks so the
originals saved via QtPassSettings::getGitExecutable() and getGpgExecutable()
are reliably restored.
🪄 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 (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 011bdcf1-9382-4e3c-87a9-cbcc84061f92

📥 Commits

Reviewing files that changed from the base of the PR and between c6f2f11 and d0142b8.

📒 Files selected for processing (1)
  • tests/auto/util/tst_util.cpp

Comment thread tests/auto/util/tst_util.cpp Outdated
Comment thread tests/auto/util/tst_util.cpp Outdated
Comment thread tests/auto/util/tst_util.cpp Outdated
@codeant-ai
Copy link
Copy Markdown
Contributor

codeant-ai Bot commented Apr 13, 2026

CodeAnt AI finished reviewing your PR.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 13, 2026

Coverage Status

coverage: 21.098% (+0.09%) from 21.004% — fix/test-improvements into main

- Fix logic error: now tests GPG executable instead of Git (configIsValid
  validates gpg/pass depending on settings, not git)
- Use scoped struct destructor to guarantee gpg executable is restored
  even if assertions fail
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.

♻️ Duplicate comments (1)
tests/auto/util/tst_util.cpp (1)

617-632: ⚠️ Potential issue | 🟠 Major

configIsValid() test still depends on runtime mode and can validate the wrong executable.

Line 623 through Line 632 always mutates gpgExecutable, but Util::configIsValid() checks passExecutable when QtPassSettings::isUsePass() is true. That makes this test mode-dependent and can miss the intended edge case.

🔧 Suggested fix
-  QString originalGpgExecutable = QtPassSettings::getGpgExecutable();
-  struct GpgRollback {
-    QString value;
-    ~GpgRollback() { QtPassSettings::setGpgExecutable(value); }
-  } gpgRollback{originalGpgExecutable};
+  struct ExecutableRollback {
+    QString passValue;
+    QString gpgValue;
+    ~ExecutableRollback() {
+      QtPassSettings::setPassExecutable(passValue);
+      QtPassSettings::setGpgExecutable(gpgValue);
+    }
+  } executableRollback{QtPassSettings::getPassExecutable(),
+                       QtPassSettings::getGpgExecutable()};
+
+  const bool usePass = QtPassSettings::isUsePass();

-  QtPassSettings::setGpgExecutable(QString());
+  if (usePass) {
+    QtPassSettings::setPassExecutable(QString());
+  } else {
+    QtPassSettings::setGpgExecutable(QString());
+  }
   isValid = Util::configIsValid();
   QVERIFY2(!isValid, "Expected invalid config when .gpg-id exists but gpg "
                      "executable is missing");

-  QtPassSettings::setGpgExecutable(
-      QStringLiteral("definitely_nonexistent_gpg_binary_12345"));
+  if (usePass) {
+    QtPassSettings::setPassExecutable(
+        QStringLiteral("definitely_nonexistent_pass_binary_12345"));
+  } else {
+    QtPassSettings::setGpgExecutable(
+        QStringLiteral("definitely_nonexistent_gpg_binary_12345"));
+  }
   isValid = Util::configIsValid();
   QVERIFY2(!isValid, "Expected invalid config when .gpg-id exists but gpg "
                      "executable is invalid");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/auto/util/tst_util.cpp` around lines 617 - 632, The test mutates
QtPassSettings::setGpgExecutable but Util::configIsValid() consults
passExecutable when QtPassSettings::isUsePass() is true, making the test
mode-dependent; update the test to either force the mode so gpgExecutable is
actually used (call QtPassSettings::setUsePass(false) with a rollback guard)
before calling Util::configIsValid(), or instead mutate the pass executable via
QtPassSettings::setPassExecutable (and add a rollback) so the invalid-executable
assertions exercise Util::configIsValid() deterministically; ensure you
reference QtPassSettings::setGpgExecutable, QtPassSettings::setPassExecutable,
QtPassSettings::setUsePass, QtPassSettings::isUsePass and Util::configIsValid
when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@tests/auto/util/tst_util.cpp`:
- Around line 617-632: The test mutates QtPassSettings::setGpgExecutable but
Util::configIsValid() consults passExecutable when QtPassSettings::isUsePass()
is true, making the test mode-dependent; update the test to either force the
mode so gpgExecutable is actually used (call QtPassSettings::setUsePass(false)
with a rollback guard) before calling Util::configIsValid(), or instead mutate
the pass executable via QtPassSettings::setPassExecutable (and add a rollback)
so the invalid-executable assertions exercise Util::configIsValid()
deterministically; ensure you reference QtPassSettings::setGpgExecutable,
QtPassSettings::setPassExecutable, QtPassSettings::setUsePass,
QtPassSettings::isUsePass and Util::configIsValid when making the change.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Pro

Run ID: 65dab5e1-80d1-4109-aaf0-d8652a717c8e

📥 Commits

Reviewing files that changed from the base of the PR and between d0142b8 and 1aec25f.

📒 Files selected for processing (1)
  • tests/auto/util/tst_util.cpp

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 27.27%. Comparing base (c6f2f11) to head (1aec25f).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #999      +/-   ##
==========================================
+ Coverage   27.12%   27.27%   +0.15%     
==========================================
  Files          39       39              
  Lines        3314     3314              
==========================================
+ Hits          899      904       +5     
+ Misses       2415     2410       -5     
Flag Coverage Δ
qtpass 27.27% <ø> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Labels

size:M This PR changes 30-99 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants