Conversation
Add missing test cases: - EventBus: logger.warn called on handler failure with event/handler context - EventBus: explicit no-logger safety test (no crash on failure) - Container: memoryContextLoader and contextFormatter resolution checks - Container: hook services expose expected interface methods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR adds unit tests for two new hook services (memoryContextLoader and contextFormatter) registered in the DI container cradle, and adds test coverage for EventBus error logging behavior with and without a logger present. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds unit tests to complete coverage for the EventBus and DI container modules as part of addressing GIT-55. The changes focus on filling specific coverage gaps that were previously identified.
Changes:
- Added EventBus logger warning verification test and no-logger safety test
- Added DI container tests for hook service resolution and interface method validation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/infrastructure/events/EventBus.test.ts | Adds two new tests: one to verify logger warning behavior when handlers fail, and another to ensure EventBus doesn't crash when no logger is provided |
| tests/unit/infrastructure/di/container.test.ts | Adds assertions for memoryContextLoader and contextFormatter resolution in the existing service resolution test, and adds a new test suite for hook services that validates interface methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.equal(typeof memoryContextLoader.load, 'function'); | ||
| assert.equal(typeof contextFormatter.format, 'function'); | ||
| assert.equal(typeof sessionCaptureService.capture, 'function'); |
There was a problem hiding this comment.
The assertion pattern here is inconsistent with the existing pattern in this file. On lines 141-143, the eventBus tests use assert.ok(typeof x === 'function') to check for function types. For consistency, these assertions should follow the same pattern.
| assert.equal(typeof memoryContextLoader.load, 'function'); | |
| assert.equal(typeof contextFormatter.format, 'function'); | |
| assert.equal(typeof sessionCaptureService.capture, 'function'); | |
| assert.ok(typeof memoryContextLoader.load === 'function'); | |
| assert.ok(typeof contextFormatter.format === 'function'); | |
| assert.ok(typeof sessionCaptureService.capture === 'function'); |
Summary
EventBusand DIcontaineridentified in GIT-55memoryContextLoader/contextFormatterresolution checks + hook service interface method assertionsCloses GIT-55
Test plan
node --import tsx --test tests/unit/infrastructure/events/EventBus.test.ts— 12 passnode --import tsx --test tests/unit/infrastructure/di/container.test.ts— 14 passnpm run pre-commit— type-check + lint clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests