test(integration): grep skips files that gpg can't decrypt#1471
Conversation
End-to-end coverage for the env-aware Executor call in
ImitatePass::grepMatchFile (imitatepass.cpp:1055). The function feeds
each .gpg file in the store to `gpg --decrypt` via the env-aware
Executor overload; when gpg exits non-zero (or stdout is empty), the
match list for that file is discarded and grepScanStore continues to
the next file.
The unit-test grepMatchFileFailedDecryptReturnsEmpty already covers
the single-file branch by pointing at a non-existent gpg binary. This
new integration test exercises the multi-file scan with real gpg:
- Insert two legitimate entries containing the "token" substring.
- Plant two .gpg files that aren't valid gpg ciphertext at all (one
with "token" in plaintext bytes, one with binary junk).
- Run Grep("token").
- Assert exactly the two legitimate entries are returned; neither
corrupt file leaks a match.
Build clean, 20/20 integration tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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)
📝 WalkthroughWalkthroughA new integration test slot, ChangesGrep robustness test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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/integration/tst_integration.cpp`:
- Line 402: Replace the bare QVERIFY assertion that checks
QDir(storeDir.path()).mkpath("work") with QVERIFY2 to include a descriptive
failure message; update the call in the test where
QVERIFY(QDir(storeDir.path()).mkpath("work")) is used so it becomes
QVERIFY2(..., "<message>") and craft the message to include context and the
store path (e.g., "failed to create work directory at " + storeDir.path()) so
test failures clearly report which directory creation failed.
- Around line 425-430: The test writes 24 bytes from a 17-byte literal causing a
buffer over-read; fix the corrupt fixture creation by using a sized buffer or
QByteArray of the exact length and pass that length to QFile::write instead of
the incorrect 24. Locate the corrupt file creation where QFile c2(corrupt2) is
opened and c2.write("\xFF\xFE\x00\x01 binary junk\n", 24) is called and replace
it with writing a QByteArray or const char* with the correct size (e.g., compute
strlen or use QByteArrayLiteral) and pass that correct size to c2.write to
eliminate undefined behavior.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2017a496-acdd-49ca-abe8-c3ed72ce1f14
📒 Files selected for processing (1)
tests/auto/integration/tst_integration.cpp
|
coverage: 29.068% (-0.02%) from 29.083% — tests/grep-skips-undecryptable into main |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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 `@tests/auto/integration/tst_integration.cpp`:
- Line 428: Add explicit assertions after the QIODevice write calls (the
c1.write(...) calls) to verify the write succeeded; for each write (the one
writing "not a real gpg payload but contains the token word\n" and the other at
the later occurrence) check the returned qint64 equals the expected byte count
(or is not -1) and use QVERIFY2 with a descriptive message (e.g., "failed to
write test payload to c1") so the test fails clearly on write errors.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5a29fb4-a0f9-4ad4-b052-ed5adb073703
📒 Files selected for processing (1)
tests/auto/integration/tst_integration.cpp
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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 `@tests/auto/integration/tst_integration.cpp`:
- Around line 428-429: The assertion uses the wrong expected byte count for the
payload written via c1.write; update the check that compares bytesWritten1 (from
c1.write("not a real gpg payload but contains the token word\n")) to use the
correct length (52) or compute the expected length dynamically (e.g., via strlen
on the literal) and replace the hardcoded 55 in the QVERIFY2 assertion so
bytesWritten1 == <correct_length> is asserted.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74a594a0-9d19-4414-a5a1-e7daf1a99e58
📒 Files selected for processing (1)
tests/auto/integration/tst_integration.cpp
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
…pp (#1475) CodeRabbit's auto-fix on #1471 captured the QFile::write() return value and asserted it equals the payload size — sensible nit. The resulting QVERIFY2 calls busted the 80-col limit and the super-linter is rejecting unrelated PRs (#1474 head was 7430a35) because clang-format runs over the whole tree. Reformat both calls — break after the condition, matches the style of all the other QVERIFY2 sites added in earlier security PRs. No semantic change. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: clang-format two over-long QVERIFY2 lines in tst_integration.cpp CodeRabbit's auto-fix on #1471 captured the QFile::write() return value and asserted it equals the payload size — sensible nit. The resulting QVERIFY2 calls busted the 80-col limit and the super-linter is rejecting unrelated PRs (#1474 head was 7430a35) because clang-format runs over the whole tree. Reformat both calls — break after the condition, matches the style of all the other QVERIFY2 sites added in earlier security PRs. No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(keygendialog): widget tests for the GPG keygen dialog KeygenDialog (the modal that fronts GPG key-pair generation) had zero test coverage. Added six widget-level tests in a new tests/auto/ keygendialog/ subdirectory, modelled on the existing tests/auto/ importkeydialog/ pattern: - constructionLoadsNonEmptyTemplate: dialog instantiates, plainTextEdit receives the default GPG batch template (covers both the ed25519 and RSA fallback paths via the shared Name-Real:/Name-Email: substring check). - expertCheckboxTogglesTemplateEditor: toggling the "Expert" checkbox flips plainTextEdit between read-only/disabled and editable/enabled. - nameTextUpdatesNameRealLine: typing in the Name field replaces the Name-Real: line in the template. - emailTextUpdatesNameEmailLine: same for Name-Email:. - matchingPassphrasesEnableButtonBox: identical text in both passphrase fields enables the OK button. - mismatchedPassphrasesDisableButtonBox: differing passphrases disable OK. The dialog is normally parented to ConfigDialog; tests pass nullptr to sidestep ConfigDialog construction. The protected done() slot isn't exercised — it dispatches into dialog->genKey() which dereferences the ConfigDialog parent. Build clean, 8/8 keygendialog tests pass, 46/46 ui tests still pass, doxygen unchanged. Wired into tests/auto/auto.pro SUBDIRS and .gitignore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(keygendialog): QVERIFY2 + scanner-safe passphrase fixtures Two reviewer nits on #1476: - Replace bare QVERIFY(obj != nullptr) with QVERIFY2(obj != nullptr, "<name> != nullptr") at every findChild site so a future test failure reports which specific widget couldn't be located. - Swap the "secret" passphrase literals in the two passphrase-matching tests for "testkey123" / "testkey456" — gitleaks-friendly fixtures that still exercise the equality / inequality paths. No behavioural change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(keygendialog): descriptive QVERIFY2 messages instead of tautological ones CodeRabbit nit on the previous follow-up: "checkBox != nullptr" as a failure message just restates the condition; QVERIFY2 already prints the condition. Switch to the existing "<widget-name> widget must exist" style already used on line 47 for plainTextEdit. No behavioural change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
End-to-end coverage for the env-aware `Executor::executeBlocking` call in `ImitatePass::grepMatchFile` (imitatepass.cpp:1055). The function feeds each `.gpg` file in the store to `gpg --decrypt` via the env-aware Executor overload; when gpg exits non-zero (or stdout is empty), the match list for that file is discarded and `grepScanStore` continues to the next file.
The existing unit test `grepMatchFileFailedDecryptReturnsEmpty` (tst_util) covers the single-file branch by pointing at a non-existent gpg binary. This new integration test exercises the multi-file scan with real gpg + a real keyring:
Test plan
Notes
This is a behavioural / smoke test of the full env-aware grep path, not a guard-isolation test — `grepMatchFile` is naturally robust to empty plaintext via its empty-line skip in the match loop, so even with the `rc != 0` early-return defanged the integration test still passes. The dedicated unit test covers the guard branch.
🤖 Generated with Claude Code
Summary by CodeRabbit