Improve coverage and prepare AMQP 23.0.0 release #375
Conversation
WalkthroughAdded v8 coverage-ignore directives across AMQP error paths, relocated a method inside AbstractAmqpPublisher, added multiple tests for error/deprecation cases and queue utilities, updated Vitest config (maxWorkers, coverage thresholds), and bumped package version and dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
🧹 Nitpick comments (2)
packages/amqp/lib/utils/AmqpQueueUtils.spec.ts (2)
105-116: Consider adding success path tests forensureAmqpQueue.The test suite only covers the error case when neither config is provided. Consider adding tests for:
- Success with
creationConfig(callsassertQueue)- Success with
locatorConfig(verifies queue exists)This would align with the PR objective to improve coverage and match the comprehensive testing pattern used for
ensureExchange.
118-129: Consider adding success path tests forensureAmqpTopicSubscription.Similar to
ensureAmqpQueue, only the error case is tested. Consider adding:
- Success with
creationConfig(asserts exchange, binds queue)- Success with
locatorConfig(verifies exchange exists)This would provide more complete coverage consistent with the PR objective.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/amqp/lib/AbstractAmqpPublisher.ts(3 hunks)packages/amqp/lib/AbstractAmqpService.ts(3 hunks)packages/amqp/lib/AmqpConnectionManager.ts(2 hunks)packages/amqp/lib/AmqpQueuePublisherManager.spec.ts(1 hunks)packages/amqp/lib/AmqpTopicPublisherManager.spec.ts(1 hunks)packages/amqp/lib/amqpConnectionResolver.ts(1 hunks)packages/amqp/lib/errors/AmqpConsumerErrorResolver.spec.ts(1 hunks)packages/amqp/lib/utils/AmqpQueueUtils.spec.ts(1 hunks)packages/amqp/package.json(2 hunks)packages/amqp/test/consumers/AmqpPermissionConsumer.spec.ts(1 hunks)packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts(1 hunks)packages/amqp/vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/amqp/lib/utils/AmqpQueueUtils.spec.ts (2)
packages/amqp/test/utils/testAmqpConfig.ts (1)
TEST_AMQP_CONFIG(3-10)packages/amqp/lib/utils/amqpQueueUtils.ts (4)
checkExchangeExists(30-45)ensureAmqpQueue(47-61)ensureAmqpTopicSubscription(63-84)deleteAmqpQueue(102-122)
packages/amqp/test/consumers/AmqpPermissionConsumer.spec.ts (2)
packages/amqp/test/consumers/AmqpPermissionConsumer.ts (1)
AmqpPermissionConsumer(39-115)packages/core/lib/index.ts (2)
objectToBuffer(107-107)waitAndRetry(109-109)
packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts (1)
packages/amqp/test/publishers/AmqpPermissionPublisher.ts (1)
AmqpPermissionPublisher(19-54)
🪛 GitHub Check: general (20.x, @message-queue-toolkit/amqp) / build
packages/amqp/lib/utils/AmqpQueueUtils.spec.ts
[failure] 7-7: lib/utils/AmqpQueueUtils.spec.ts
Error: Cannot find module './AmqpQueueUtils.ts' imported from '/home/runner/work/message-queue-toolkit/message-queue-toolkit/packages/amqp/lib/utils/AmqpQueueUtils.spec.ts'
❯ lib/utils/AmqpQueueUtils.spec.ts:7:1
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { code: 'ERR_MODULE_NOT_FOUND' }
🪛 GitHub Check: general (22.x, @message-queue-toolkit/amqp) / build
packages/amqp/lib/utils/AmqpQueueUtils.spec.ts
[failure] 7-7: lib/utils/AmqpQueueUtils.spec.ts
Error: Cannot find module './AmqpQueueUtils.ts' imported from '/home/runner/work/message-queue-toolkit/message-queue-toolkit/packages/amqp/lib/utils/AmqpQueueUtils.spec.ts'
❯ lib/utils/AmqpQueueUtils.spec.ts:7:1
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { code: 'ERR_MODULE_NOT_FOUND' }
🪛 GitHub Check: general (24.x, @message-queue-toolkit/amqp) / build
packages/amqp/lib/utils/AmqpQueueUtils.spec.ts
[failure] 7-7: lib/utils/AmqpQueueUtils.spec.ts
Error: Cannot find module './AmqpQueueUtils.ts' imported from '/home/runner/work/message-queue-toolkit/message-queue-toolkit/packages/amqp/lib/utils/AmqpQueueUtils.spec.ts'
❯ lib/utils/AmqpQueueUtils.spec.ts:7:1
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { code: 'ERR_MODULE_NOT_FOUND' }
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: kafka (22.x) / build
- GitHub Check: kafka (24.x) / build
🔇 Additional comments (20)
packages/amqp/lib/AmqpConnectionManager.ts (1)
30-46: Appropriate use of v8 ignore for async event handlers.The coverage exclusion for the
errorandcloseevent handlers is reasonable since these callbacks are triggered asynchronously by the underlying AMQP library during connection failures, making them difficult to exercise reliably in unit tests.Minor observation (pre-existing, not blocking): The
this.reconnectsActivecheck on line 41 is redundant since it's already verified in the outer condition on line 39.packages/amqp/lib/amqpConnectionResolver.ts (1)
29-44: Appropriate use of v8 ignore for retry logic.The coverage exclusion for the catch block and retry logic is reasonable. Testing connection failure scenarios with actual retry delays would result in slow and potentially flaky tests. The retry logic itself (incrementing backoff with
retryTime, max attempt check) is straightforward.packages/amqp/lib/AbstractAmqpService.ts (1)
89-94: Appropriate use of v8 ignore for error handling paths.The coverage exclusions are appropriate for:
- The catch block (lines 89-94) handling channel creation failures
- The
closeanderrorevent handlers (lines 107-120) that are triggered asynchronouslyThese follow the same pattern as the other AMQP files in this PR and exclude hard-to-test error recovery code paths from coverage metrics.
Also applies to: 107-120
packages/amqp/package.json (2)
3-3: LGTM! Dependency updates align with the 23.0.0 release.The version bump to 23.0.0 and associated dependency updates are appropriate for a major release. The updates include:
- Minor updates to existing dependencies
- Major version bump for Vitest (v3 → v4) with matching coverage provider
- Zod patch update within v4
Also applies to: 31-31, 40-40, 45-47, 51-53
34-34: Verify the actual version requirement change and compatibility needs.The peer dependency requirement for
@message-queue-toolkit/coreis currently>=25.0.0. If this was changed from>=22.0.0in this PR, confirm whether breaking changes between versions 22-25 actually require this minimum version. If so, consider using^25.0.0instead to allow minor/patch updates within the v25.x range while preventing incompatible major versions.packages/amqp/lib/AbstractAmqpPublisher.ts (2)
85-86: LGTM! v8 ignore directive for hard-to-test error path.The v8 ignore comment on the lazy init error handler is appropriate, as this error path occurs when async initialization fails after fire-and-forget publish. This is difficult to reliably trigger in tests without complex mocking.
144-167: LGTM! Coverage exclusion for publisher-unsupported methods.The v8 ignore block correctly excludes abstract methods that are intentionally unimplemented for publishers (they're only used by consumers). The new test in AmqpPermissionPublisher.spec.ts (lines 113-130) verifies these methods throw the expected errors.
packages/amqp/lib/AmqpQueuePublisherManager.spec.ts (2)
128-134: LGTM! Good test for deprecated API.This test correctly validates that the deprecated
publish()method throws an error guiding users to usepublishSync()instead. Clear deprecation path.
136-147: LGTM! Good validation for unknown queue error.This test ensures proper error handling when attempting to publish to a queue that hasn't been registered with the publisher manager. The error message is clear and actionable.
packages/amqp/test/consumers/AmqpPermissionConsumer.spec.ts (2)
422-433: LGTM! Good edge case test for malformed messages.This test validates that the consumer gracefully handles empty message bodies without processing them or crashing. The test appropriately verifies no side effects occurred (counter unchanged).
435-449: LGTM! Good validation for schema enforcement.This test ensures that messages missing required fields (like
id) are properly rejected by schema validation. The test correctly verifies the error is captured by the error resolver.packages/amqp/lib/AmqpTopicPublisherManager.spec.ts (2)
93-99: LGTM! Consistent deprecation handling.This test mirrors the queue publisher manager's deprecation test, ensuring consistent behavior across publisher types. The error message guides users to the correct method.
101-112: LGTM! Good validation for unknown exchange error.This test ensures proper error handling when attempting to publish to an exchange that hasn't been registered. The error message clearly identifies the missing exchange.
packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts (2)
101-111: LGTM! Good validation for configuration requirements.This test ensures the publisher constructor properly validates that either
locatorConfigorcreationConfigprovides aqueueName. Clear error message for misconfiguration.
113-130: LGTM! Comprehensive coverage for abstract method stubs.This test validates that all abstract methods meant only for consumers throw the expected "Not implemented for publisher" error. This provides test coverage for the methods excluded by v8 ignore directives in AbstractAmqpPublisher.ts (lines 144-167), confirming they behave as documented.
packages/amqp/lib/errors/AmqpConsumerErrorResolver.spec.ts (1)
1-86: LGTM! Comprehensive error handling test coverage.This new test file provides excellent coverage for
AmqpConsumerErrorResolver.processError, validating:
- Correct error type mapping (SyntaxError → MessageInvalidFormatError, ZodError → MessageValidationError)
- Standardized error handling with proper errorCode extraction
- Graceful fallback for unexpected error types (null, undefined, strings, unknown objects)
The tests are well-structured and cover all major error scenarios.
packages/amqp/vitest.config.ts (2)
10-10: LGTM! Updated to Vitest v4 configuration.The change from
poolOptions: { threads: { singleThread: true } }tomaxWorkers: 1aligns with Vitest v4's updated configuration API while maintaining the same single-threaded test execution behavior.
16-19: LGTM! Coverage thresholds increased with supporting tests.The modest coverage threshold increases (lines: 89→90%, branches: 79→80%, statements: 89→90%) are backed by the comprehensive test additions in this PR, including error handling tests, edge case validation, and deprecated API coverage.
packages/amqp/lib/utils/AmqpQueueUtils.spec.ts (2)
28-79: LGTM!Comprehensive test coverage for
ensureExchangewith proper resource cleanup and good coverage of both success and error paths.
131-200: LGTM!Excellent comprehensive coverage of
deleteAmqpQueuewith all code paths tested, proper environment mocking, and thorough resource cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts (1)
113-129: Coverage test for abstract methods is functional.This test successfully covers the abstract method error paths. The implementation is clean and properly manages resources (init/close).
Minor observation: These tests target abstract method implementations rather than observable publisher behavior. Since the PR explicitly aims to improve coverage and these methods should never be invoked on publishers in practice, this approach achieves its goal. Alternative approaches like integration tests might naturally cover error paths through actual usage, but that would require more complex setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts (1)
packages/amqp/test/publishers/AmqpPermissionPublisher.ts (1)
AmqpPermissionPublisher(19-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: general (24.x, @message-queue-toolkit/s3-payload-store) / build
- GitHub Check: general (20.x, @message-queue-toolkit/redis-message-deduplication-store) / build
- GitHub Check: general (20.x, @message-queue-toolkit/metrics) / build
- GitHub Check: general (22.x, @message-queue-toolkit/redis-message-deduplication-store) / build
- GitHub Check: general (22.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: general (22.x, @message-queue-toolkit/gcs-payload-store) / build
- GitHub Check: general (24.x, @message-queue-toolkit/redis-message-deduplication-store) / build
- GitHub Check: general (24.x, @message-queue-toolkit/metrics) / build
- GitHub Check: general (20.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (22.x, @message-queue-toolkit/outbox-core) / build
- GitHub Check: general (20.x, @message-queue-toolkit/s3-payload-store) / build
- GitHub Check: general (24.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (24.x, @message-queue-toolkit/outbox-core) / build
- GitHub Check: general (22.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (22.x, @message-queue-toolkit/schemas) / build
- GitHub Check: general (24.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: general (20.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (20.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: kafka (22.x) / build
- GitHub Check: kafka (24.x) / build
🔇 Additional comments (1)
packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts (1)
101-111: LGTM! Good defensive validation test.This test properly validates that the constructor enforces the queueName requirement across both configuration options. The use of
@ts-expect-erroris appropriate for intentionally passing invalid configs to test runtime validation.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.