test(daemon): expand abbenay-service and consumer-auth unit coverage - #111
Conversation
Add comprehensive gRPC handler tests for abbenay-service and edge-case coverage for consumer-auth. Use real auth paths and withEnv cleanup instead of tautological simulations and authorizeConsumer spies.
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR expands daemon server test coverage for conversions, authorization, consumer authentication, unary handlers, streaming handlers, validation, persistence, and gRPC error mapping. ChangesDaemon server test coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 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❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (66.66%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
+ Coverage 68.57% 78.02% +9.45%
==========================================
Files 37 38 +1
Lines 4973 4979 +6
Branches 1568 1569 +1
==========================================
+ Hits 3410 3885 +475
+ Misses 1023 542 -481
- Partials 540 552 +12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/daemon/src/daemon/server/abbenay-service.test.ts (3)
871-884: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore fake timers and global spies in a
finallyblock.
Shutdowninstalls fake timers and spies onprocess.emit. If any assertion between the setup and the teardown fails,vi.useRealTimers()andemitSpy.mockRestore()never run. Fake timers then leak into later tests in the same file and cause unrelated failures. The same pattern exists at lines 1382-1414 (VSCodeStream) and 2288-2322.Use
try { ... } finally { ... }, or move the timer setup and teardown intobeforeEach/afterEachhooks in a nesteddescribe.♻️ Proposed change for the `Shutdown` test
it('Shutdown acknowledges and schedules SIGTERM', async () => { vi.useFakeTimers(); const emitSpy = vi.spyOn(process, 'emit').mockReturnValue(true as never); - const state = createMockState(); - const service = createServiceHandlers(state); - - const { error } = await invokeUnary(service.Shutdown, {}); - expect(error).toBeNull(); - vi.advanceTimersByTime(150); - expect(emitSpy).toHaveBeenCalledWith('SIGTERM', 'SIGTERM'); - - emitSpy.mockRestore(); - vi.useRealTimers(); + try { + const state = createMockState(); + const service = createServiceHandlers(state); + + const { error } = await invokeUnary(service.Shutdown, {}); + expect(error).toBeNull(); + vi.advanceTimersByTime(150); + expect(emitSpy).toHaveBeenCalledWith('SIGTERM', 'SIGTERM'); + } finally { + emitSpy.mockRestore(); + vi.useRealTimers(); + } });Also applies to: 1382-1414, 2288-2322
🤖 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 `@packages/daemon/src/daemon/server/abbenay-service.test.ts` around lines 871 - 884, Update the Shutdown, VSCodeStream, and corresponding test around the later referenced range so fake timers and process.emit spies are always cleaned up in a finally block, even when setup or assertions fail; preserve the existing test assertions and restore emitSpy before calling vi.useRealTimers().
121-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract
withEnvinto a shared test helper.
consumer-auth.test.tsdefines the samewithEnvfunction at lines 121-130 of that file. Move it to a shared test utility module and import it in both files.🤖 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 `@packages/daemon/src/daemon/server/abbenay-service.test.ts` around lines 121 - 130, Move the duplicated withEnv helper from abbenay-service.test.ts and consumer-auth.test.ts into a shared test utility module, preserving its environment restoration behavior, then import and use the shared withEnv in both test files and remove their local definitions.
2168-2213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd assertions to the coverage-only invocations.
Lines 2168-2170 and 2195-2212 call handlers without checking any result. These calls raise line coverage but do not verify behavior. A regression in
SummarizeSession,GetSession,ListSessions,DeleteSession,ReconnectMcpServer,ConfigureProvider, orRemoveProvidercamelCase field handling would still pass.Assert at minimum that each call returns no error, and that the camelCase field reached the mock. Example:
expect(state.mcpClientPool.reconnect).toHaveBeenCalledWith('dyn-ok').🤖 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 `@packages/daemon/src/daemon/server/abbenay-service.test.ts` around lines 2168 - 2213, Add assertions to the coverage-only invocations in the test: verify Register, ListModels, SummarizeSession, GetSession, ListSessions, DeleteSession, ReconnectMcpServer, ConfigureProvider, and RemoveProvider return without errors, and assert relevant mocks receive the expected camelCase-derived values, including state.mcpClientPool.reconnect called with 'dyn-ok'.
🤖 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/server/consumer-auth.test.ts`:
- Around line 111-115: Update the test for buildConsumerAuthContext to pass an
explicit environment map when invoking it, and apply the same deterministic
environment setup to matchConsumerByToken tests. Ensure ABBENAY_ALLOW_OPEN_AUTH
and NOT_SET_ENV cannot leak from process.env by explicitly setting or clearing
them and restoring any modified values after each test.
---
Nitpick comments:
In `@packages/daemon/src/daemon/server/abbenay-service.test.ts`:
- Around line 871-884: Update the Shutdown, VSCodeStream, and corresponding test
around the later referenced range so fake timers and process.emit spies are
always cleaned up in a finally block, even when setup or assertions fail;
preserve the existing test assertions and restore emitSpy before calling
vi.useRealTimers().
- Around line 121-130: Move the duplicated withEnv helper from
abbenay-service.test.ts and consumer-auth.test.ts into a shared test utility
module, preserving its environment restoration behavior, then import and use the
shared withEnv in both test files and remove their local definitions.
- Around line 2168-2213: Add assertions to the coverage-only invocations in the
test: verify Register, ListModels, SummarizeSession, GetSession, ListSessions,
DeleteSession, ReconnectMcpServer, ConfigureProvider, and RemoveProvider return
without errors, and assert relevant mocks receive the expected camelCase-derived
values, including state.mcpClientPool.reconnect called with 'dyn-ok'.
🪄 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: e28f6c12-4f6b-4267-82db-26bf142a1679
📒 Files selected for processing (2)
packages/daemon/src/daemon/server/abbenay-service.test.tspackages/daemon/src/daemon/server/consumer-auth.test.ts
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Pass explicit env maps in consumer-auth tests, extract shared withEnv helper, wrap fake-timer tests in try/finally, and assert camelCase handler invocations instead of coverage-only calls.
|
Addressed CodeRabbit review feedback in a1d4e66:
|
|
4 participants
Summary
abbenay-service.test.tswith gRPC handler coverage (auth, streaming, MCP, config, sessions, providers)consumer-authedge-case tests (env variants, token matching, missing env values)authorizeConsumerspies; use real auth paths andwithEnvcleanupwithEnvhelper, fake-timertry/finally, handler assertion coverageCommits
test(daemon): expand abbenay-service and consumer-auth unit coverage— comprehensive handler tests and consumer-auth edge casestest(daemon): address CodeRabbit review on daemon server tests— explicit env maps, sharedwithEnv, timer cleanup, camelCase handler assertionsCoverage
abbenay-service.tsconsumer-auth.tsTest plan
npx eslint src/daemon/server/abbenay-service.test.ts src/daemon/server/consumer-auth.test.ts src/daemon/server/test-env.tsnpx vitest run src/daemon/server/abbenay-service.test.ts src/daemon/server/consumer-auth.test.tsnpm test(full monorepo)npm run ci:build