test(daemon): add KeychainSecretStore unit tests - #112
Conversation
Cover keytar CRUD, import failures, SEA dlopen path, and node:sea branches at 100% line/branch coverage. Inline keytar in vitest so dynamic import() is mockable in unit tests.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request adds comprehensive ChangesKeychain test coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #112 +/- ##
==========================================
+ Coverage 67.78% 68.55% +0.76%
==========================================
Files 37 37
Lines 4973 4973
Branches 1568 1568
==========================================
+ Hits 3371 3409 +38
+ Misses 1056 1023 -33
+ Partials 546 541 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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 `@packages/daemon/src/daemon/secrets/keychain.test.ts`:
- Line 83: Replace both zero-delay timer waits in
packages/daemon/src/daemon/secrets/keychain.test.ts at lines 83 and 97 with
await vi.dynamicImportSettled(), ensuring tests deterministically wait for
KeychainSecretStore’s loadKeytar dynamic import before resetting or injecting
state.
- Around line 63-65: Update the keychain test setup and both helpers: after
vi.doUnmock('keytar'), re-register the default keytar mock with vi.doMock before
importing ./keychain.js, and replace each zero-delay timer with await
vi.dynamicImportSettled() so constructor loading completes before modifying
store internals.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a96f22a-8c21-43f8-85c2-afee00397091
📒 Files selected for processing (2)
packages/daemon/src/daemon/secrets/keychain.test.tspackages/daemon/vitest.config.ts
Re-register default keytar mock after doUnmock/resetModules so constructor loadKeytar() never hits the native module. Replace zero-delay timers with vi.dynamicImportSettled() for deterministic constructor import settling.
|



Summary
KeychainSecretStorecovering get/set/delete/has, keytar import failures, SEAdlopenloading, andnode:seadetection branches.keytarin vitest config soimport('keytar')is mockable in unit tests.doUnmock/resetModules, and usevi.dynamicImportSettled()instead of zero-delay timers for constructor import settling.packages/daemon/src/daemon/secrets/keychain.tswith no production code changes.Commits
test(daemon): add KeychainSecretStore unit tests— 26 tests; keychain coverage 28% → 100%test(daemon): stabilize keychain test mock lifecycle— address CodeRabbit review: re-register keytar mock inbeforeEach, usevi.dynamicImportSettled()for deterministic constructor loadingTest plan
npm run lint -w packages/daemonnpm run test:coverage -w packages/daemon -- src/daemon/secrets/keychain.test.tsnpm test -w packages/daemon(1108 tests)