test: add coverage for readTemplates, writeTemplates, getFolderTemplate#1496
Conversation
readTemplates (4 tests): - missing .templates file returns empty hash - single section with multiple fields parsed correctly - multiple sections parsed into separate hash entries - empty section name [] is ignored with a warning - comment lines (#) are skipped writeTemplates (3 tests): - round-trip: write then read yields identical data - empty hash writes successfully and reads back empty - sections are written in alphabetical key order getFolderTemplate (4 tests): - .default_template in the current directory is found - .default_template in a parent directory is found - no .default_template anywhere returns empty string - .default_template containing only a comment returns empty string Total util tests: 123 → 136 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds 12 unit tests covering Util::readTemplates, Util::writeTemplates, and Util::getFolderTemplate, and changes a path-prefix check in Util::getFolderTemplate to use a hard-coded forward slash. ChangesTemplate Utility Tests & getFolderTemplate tweak
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/auto/util/tst_util.cpp`:
- Around line 2567-2568: The current QVERIFY2 check using
content.indexOf("[alpha]") < content.indexOf("[zebra]") can falsely pass if one
or both markers are missing (indexOf returns -1); update the test in
tst_util.cpp to first assert both markers exist (e.g.,
QVERIFY(content.indexOf("[alpha]") != -1) and QVERIFY(content.indexOf("[zebra]")
!= -1) or a combined QVERIFY2 that checks presence) and only then perform the
ordering assertion using content.indexOf("[alpha]") < content.indexOf("[zebra]")
so the test fails if either section is missing.
- Around line 2501-2514: Add an explicit expectation for the warning emitted
when an empty section is encountered: before calling
Util::readTemplates(tmp.path()) in tst_util::readTemplatesEmptySectionIgnored(),
call QTest::ignoreMessage(...) (e.g. QtWarningMsg) with the warning text or a
substring that the Util::readTemplates implementation emits for empty section
names so the test asserts the warning side-effect as well as the parsed output.
🪄 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: 6e3d3a01-c8b8-43f1-9982-7d54fdfe752f
📒 Files selected for processing (1)
tests/auto/util/tst_util.cpp
writeTemplatesSortedKeys: indexOf returns -1 for missing markers, so -1 < positivePos would silently pass. Add explicit presence checks for both [alpha] and [zebra] before the ordering assertion. readTemplatesEmptySectionIgnored: add QTest::ignoreMessage(QtWarningMsg) before calling readTemplates so the expected qWarning is asserted as a test side-effect rather than just leaking to stderr. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
coverage: 54.868% (+0.9%) from 53.977% — test/util-template-tests into main |
QDir::cleanPath() normalizes separators to '/' on all platforms, but QDir::separator() returns '\' on Windows, causing the startsWith guard to never match on Windows and preventing parent-directory traversal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/util.cpp`:
- Around line 684-685: The ancestry check incorrectly appends "/" to
cleanStoreAbs unconditionally, causing double slashes for root store paths and
breaking currentPath.startsWith; update the guard in the loop (where
currentPath, cleanStoreAbs and startsWith are used) to allow either currentPath
== cleanStoreAbs or currentPath.startsWith(cleanStoreAbs + "/") (or equivalently
check startsWith(cleanStoreAbs) and then ensure either equal or the next
character is '/'), so root paths like "/" or "C:/" are handled correctly without
producing "//" or "C://".
🪄 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: ed5f3f1f-7d96-4f8e-8a44-6bc7f8a188bc
📒 Files selected for processing (1)
src/util.cpp
Appending "/" to cleanStoreAbs produces "//" when the store is at the
filesystem root ("/"), causing startsWith to never match. Check the
character at cleanStoreAbs.length() instead to safely distinguish a
proper subdirectory from a sibling with a longer name.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/util.cpp (1)
684-686:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRoot store paths still break the ancestry check.
The new approach still fails when
cleanStoreAbsis a filesystem root like"/"or"C:/". For example, if the password store is at"/"and the current path is"/passwords":
- Line 684:
currentPath.startsWith("/")→ true- Line 685:
10 <= 1→ false- Line 686:
currentPath.at(1)→'p'→'p' != '/'→ trueThe condition evaluates to true and breaks, incorrectly stopping the search even though
"/passwords"is a valid descendant of"/".The issue is that for root paths,
cleanStoreAbs.length()points to the first character of the subdirectory name, not a separator. As per coding guidelines, boundary checks must correctly prevent sibling-path matches across all valid store locations.🐛 Proposed fix using conditional storePrefix
- if (!currentPath.startsWith(cleanStoreAbs) || - currentPath.length() <= cleanStoreAbs.length() || - currentPath.at(cleanStoreAbs.length()) != QChar('/')) { + const QString storePrefix = cleanStoreAbs.endsWith(QLatin1Char('/')) + ? cleanStoreAbs + : cleanStoreAbs + QLatin1Char('/'); + if (!currentPath.startsWith(storePrefix)) { break; }This ensures that root paths like
"/"use"/"as the prefix (no double-slash), while non-root paths like"/home/user/store"correctly use"/home/user/store/".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util.cpp` around lines 684 - 686, The ancestry check fails for root stores because it indexes at cleanStoreAbs.length() assuming a separator there; instead construct a storePrefix that guarantees a trailing separator (storePrefix = cleanStoreAbs.endsWith('/') ? cleanStoreAbs : cleanStoreAbs + '/') and then replace the current three-part condition with a single check that currentPath is either exactly cleanStoreAbs or startsWith(storePrefix) (i.e. if (!(currentPath == cleanStoreAbs || currentPath.startsWith(storePrefix)) ) { ... }). Update the check around currentPath and cleanStoreAbs/currentPath.startsWith to use these symbols so root paths like "/" or "C:/" are handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/util.cpp`:
- Around line 684-686: The ancestry check fails for root stores because it
indexes at cleanStoreAbs.length() assuming a separator there; instead construct
a storePrefix that guarantees a trailing separator (storePrefix =
cleanStoreAbs.endsWith('/') ? cleanStoreAbs : cleanStoreAbs + '/') and then
replace the current three-part condition with a single check that currentPath is
either exactly cleanStoreAbs or startsWith(storePrefix) (i.e. if (!(currentPath
== cleanStoreAbs || currentPath.startsWith(storePrefix)) ) { ... }). Update the
check around currentPath and cleanStoreAbs/currentPath.startsWith to use these
symbols so root paths like "/" or "C:/" are handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 420e20bd-1b8f-4020-9a94-cbe763e9643f
📒 Files selected for processing (1)
src/util.cpp
All other test .pro files use 'LIBS = ... $$LIBS' so that -lqtpass comes before the Windows system libs (-lmpr, -lbcrypt) accumulated from qtpass.pri. mainwindow.pro used 'LIBS +=' which placed the system libs first, causing 'undefined reference to WNetUseConnectionA' on MinGW. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1496 +/- ##
==========================================
+ Coverage 48.60% 49.86% +1.25%
==========================================
Files 44 44
Lines 4386 4386
==========================================
+ Hits 2132 2187 +55
+ Misses 2254 2199 -55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3 participants
Summary
Three
Utilmethods had zero test coverage despite non-trivial parsing logic:readTemplates(5 tests) — INI-style.templatesfile parser:[]ignored (triggers expectedqWarning)#skippedwriteTemplates(3 tests) — writes templates hash to disk:write→readyields identical datagetFolderTemplate(4 tests) — walks directory tree for.default_template:Test plan
make checkpasses locally: 123 → 136 tests, 0 failures (4 skipped = existing Unix-only socket tests)clang-format --style=fileapplied🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Bug Fixes