Conversation
📝 WalkthroughWalkthroughAdds TestSnsPublisher and TestSnsPublishOptions, a test utility enabling flexible SNS publishing (topicArn, topicName, consumer, or publisher) with FIFO support, accompanying comprehensive tests, and public re-exports in package exports. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Code
participant TSNSP as TestSnsPublisher
participant STS as STS Client
participant SNS as SNS Client
participant Topic as SNS Topic
participant Queue as SQS Queue
participant Consumer as SQS Consumer
Test->>TSNSP: publish(payload, options)
alt options.topicName
TSNSP->>STS: Resolve topic name -> ARN
STS-->>TSNSP: Return ARN
else options.consumer / options.publisher
TSNSP->>TSNSP: Extract ARN from consumer/publisher
else options.topicArn
TSNSP->>TSNSP: Use provided ARN
end
TSNSP->>SNS: PublishCommand(payload, ARN, FIFO attrs)
SNS->>Topic: Deliver message
Topic->>Queue: Forward to subscribed queue
Consumer->>Queue: Poll for messages
Queue-->>Consumer: Return message
Consumer-->>Test: Verify payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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/sns/lib/sns/fakes/TestSnsPublisher.ts (1)
13-53: Consider extracting common FIFO fields to reduce repetition.
MessageGroupIdandMessageDeduplicationIdare duplicated across all four union variants. You could extract them into a base type and intersect:♻️ Suggested refactor
+type FifoOptions = { + MessageGroupId?: string + MessageDeduplicationId?: string +} + export type TestSnsPublishOptions = - | { - topicArn: string - topicName?: never - consumer?: never - publisher?: never - MessageGroupId?: string - MessageDeduplicationId?: string - } - | { - topicName: string - topicArn?: never - consumer?: never - publisher?: never - MessageGroupId?: string - MessageDeduplicationId?: string - } + | ({ topicArn: string; topicName?: never; consumer?: never; publisher?: never } & FifoOptions) + | ({ topicName: string; topicArn?: never; consumer?: never; publisher?: never } & FifoOptions) // ... same for consumer and publisher variants🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/lib/sns/fakes/TestSnsPublisher.ts` around lines 13 - 53, Extract the duplicated FIFO fields into a shared base type and intersect it with the existing union to remove repetition: create a base type (e.g., SnsFifoFields) that declares MessageGroupId?: string and MessageDeduplicationId?: string, then update TestSnsPublishOptions to be the union of the four existing variant types intersected with that base (keep existing variant names/structure referencing AbstractSnsSqsConsumer and AbstractSnsPublisher unchanged so callers still match the same discriminants). This keeps the same public shape while centralizing FIFO fields for easier maintenance.packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts (1)
69-92: Consider extracting the repeated SQS polling helper.The pattern of creating a
Consumer, starting it,waitAndRetry-ing, stopping it, and parsing the SNS envelope is repeated six times. A small helper would reduce boilerplate:async function collectMessagesFromQueue( sqsClient: SQSClient, queueUrl: string, ): Promise<unknown[]> { const receivedMessages: SQSMessage[] = [] const consumer = Consumer.create({ queueUrl, sqs: sqsClient, handleMessage: async (message: SQSMessage) => { receivedMessages.push(message) return message }, }) consumer.start() await waitAndRetry(() => receivedMessages.length > 0) consumer.stop() return receivedMessages.map((m) => { const envelope = JSON.parse(m.Body!) return JSON.parse(envelope.Message) }) }Also applies to: 108-127, 145-164, 188-207, 251-270, 317-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts` around lines 69 - 92, Extract the repeated SQS polling + parse logic into a helper function (e.g., collectMessagesFromQueue) and replace the six duplicated blocks with calls to it; the helper should create a receivedMessages array, call Consumer.create({ queueUrl, sqs: sqsClient, handleMessage: async (message: SQSMessage) => { receivedMessages.push(message); return message } }), start the consumer, await waitAndRetry(() => receivedMessages.length > 0), stop the consumer, and return the parsed SNS messages by JSON.parse-ing message.Body and then JSON.parse-ing the envelope.Message. Update each test case that currently uses Consumer.create / waitAndRetry / consumer.start / consumer.stop / JSON.parse(...) to call collectMessagesFromQueue(sqsClient, queueUrl) and assert against the returned parsed messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts`:
- Around line 1-18: The imports are out of Biome's organizeImports order: move
the local utility imports (deserializeSNSMessage, subscribeToTopic, assertTopic,
deleteTopic) so they come before the deeper relative test imports
(SnsSqsPermissionConsumer, PERMISSIONS_ADD_MESSAGE_SCHEMA,
SnsPermissionPublisher, Dependencies, registerDependencies, TestSnsPublisher),
then run Biome's auto-fix to apply the correct ordering (npx `@biomejs/biome`
check --write packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts).
---
Nitpick comments:
In `@packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts`:
- Around line 69-92: Extract the repeated SQS polling + parse logic into a
helper function (e.g., collectMessagesFromQueue) and replace the six duplicated
blocks with calls to it; the helper should create a receivedMessages array, call
Consumer.create({ queueUrl, sqs: sqsClient, handleMessage: async (message:
SQSMessage) => { receivedMessages.push(message); return message } }), start the
consumer, await waitAndRetry(() => receivedMessages.length > 0), stop the
consumer, and return the parsed SNS messages by JSON.parse-ing message.Body and
then JSON.parse-ing the envelope.Message. Update each test case that currently
uses Consumer.create / waitAndRetry / consumer.start / consumer.stop /
JSON.parse(...) to call collectMessagesFromQueue(sqsClient, queueUrl) and assert
against the returned parsed messages.
In `@packages/sns/lib/sns/fakes/TestSnsPublisher.ts`:
- Around line 13-53: Extract the duplicated FIFO fields into a shared base type
and intersect it with the existing union to remove repetition: create a base
type (e.g., SnsFifoFields) that declares MessageGroupId?: string and
MessageDeduplicationId?: string, then update TestSnsPublishOptions to be the
union of the four existing variant types intersected with that base (keep
existing variant names/structure referencing AbstractSnsSqsConsumer and
AbstractSnsPublisher unchanged so callers still match the same discriminants).
This keeps the same public shape while centralizing FIFO fields for easier
maintenance.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts (2)
77-80: Stale SQS consumer on test failure — wrap start/stop in try/finally.This
Consumer.create/start/stoppattern is repeated six times (lines 77–80, 116–119, 153–156, 196–199, 261–262, 325–328). IfwaitAndRetrytimes out and throws,consumer.stop()is never called. The stale poller continues draining from the queue and can consume messages intended by later tests, causing intermittent failures.The same problem applies to the un-awaited
afterEach-invisible resourcesregularPublisher.close()(line 206) andconsumer.close()(lines 269, 365) — both only execute on the happy path.♻️ Example fix for one occurrence (apply the same pattern to all six)
- consumer.start() - - await waitAndRetry(() => receivedMessages.length > 0) - consumer.stop() + consumer.start() + try { + await waitAndRetry(() => receivedMessages.length > 0) + } finally { + consumer.stop() + }For the framework-level resources in the "publish with publisher" test:
- await publisher.publish({ test: 'publisher' }, { publisher: regularPublisher }) - /* ... Consumer create/start/stop ... */ - expect(receivedMessages).toHaveLength(1) - /* ... */ - await regularPublisher.close() + try { + await publisher.publish({ test: 'publisher' }, { publisher: regularPublisher }) + /* ... Consumer create/start/stop ... */ + expect(receivedMessages).toHaveLength(1) + /* ... */ + } finally { + await regularPublisher.close() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts` around lines 77 - 80, Tests create an SQS poller via Consumer.create and call consumer.start(), but if waitAndRetry(...) throws the test will exit before consumer.stop() (and other cleanup like regularPublisher.close() and consumer.close()) runs, leaving a stale consumer; wrap each consumer.start()/consumer.stop() pair in a try/finally so consumer.stop() is always invoked (and await it if it returns a promise), and ensure we also await/guard calls to regularPublisher.close() and consumer.close() (e.g., in the test-level try/finally or a shared afterEach that awaits the closers) for all occurrences around Consumer.create/consumer.start/consumer.stop, waitAndRetry, regularPublisher.close and consumer.close.
67-76: Extract repeatedConsumer.createboilerplate into a helper.The same 8-line block (biome-ignore comment,
Consumer.create, handler) appears six times. A small helper would reduce noise and make the fix for the try/finally issue above easier to apply consistently.♻️ Proposed helper
+function createTestConsumer(sqsClient: SQSClient, queueUrl: string, bucket: SQSMessage[]) { + return Consumer.create({ + queueUrl, + sqs: sqsClient, + // biome-ignore lint/suspicious/useAwait: Consumer.create requires async handler + handleMessage: async (message: SQSMessage) => { + bucket.push(message) + return message + }, + }) +}Usage at each call site:
- const receivedMessages: SQSMessage[] = [] - const consumer = Consumer.create({ - queueUrl, - sqs: sqsClient, - // biome-ignore lint/suspicious/useAwait: Consumer.create requires async handler - handleMessage: async (message: SQSMessage) => { - receivedMessages.push(message) - return message - }, - }) + const receivedMessages: SQSMessage[] = [] + const consumer = createTestConsumer(sqsClient, queueUrl, receivedMessages)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts` around lines 67 - 76, Extract the repeated Consumer.create boilerplate into a single helper function (e.g., createTestConsumer) that accepts the queueUrl, sqsClient and a receivedMessages array (or returns a consumer with an injected handler), encapsulates the biome-ignore lint comment, calls Consumer.create with queueUrl and sqs, and sets handleMessage to the async handler that pushes the message into receivedMessages and returns it; replace each inline Consumer.create block with calls to this helper (refer to Consumer.create, handleMessage and receivedMessages in the diff).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts`:
- Around line 1-16: Imports in TestSnsPublisher.spec.ts are not ordered
consistently; reorder the import statements so external packages (e.g.,
'@aws-sdk/*', '@lokalise/*', 'sqs-consumer', 'vitest') come first, then
third-party toolkit modules (e.g., '@message-queue-toolkit/sqs'), then
types/local test utilities and finally local project modules (e.g.,
SnsSqsPermissionConsumer, SnsPermissionPublisher, registerDependencies,
subscribeToTopic, assertTopic/deleteTopic, TestSnsPublisher); within each group
sort alphabetically (e.g., SNSClient, SQSClient, STSClient together;
waitAndRetry; SQSMessage, assertQueue/deleteQueue; AwilixContainer; Consumer;
vitest imports), ensuring consistent grouping and alphabetical order across the
file to satisfy the import-order rule.
---
Nitpick comments:
In `@packages/sns/lib/sns/fakes/TestSnsPublisher.spec.ts`:
- Around line 77-80: Tests create an SQS poller via Consumer.create and call
consumer.start(), but if waitAndRetry(...) throws the test will exit before
consumer.stop() (and other cleanup like regularPublisher.close() and
consumer.close()) runs, leaving a stale consumer; wrap each
consumer.start()/consumer.stop() pair in a try/finally so consumer.stop() is
always invoked (and await it if it returns a promise), and ensure we also
await/guard calls to regularPublisher.close() and consumer.close() (e.g., in the
test-level try/finally or a shared afterEach that awaits the closers) for all
occurrences around Consumer.create/consumer.start/consumer.stop, waitAndRetry,
regularPublisher.close and consumer.close.
- Around line 67-76: Extract the repeated Consumer.create boilerplate into a
single helper function (e.g., createTestConsumer) that accepts the queueUrl,
sqsClient and a receivedMessages array (or returns a consumer with an injected
handler), encapsulates the biome-ignore lint comment, calls Consumer.create with
queueUrl and sqs, and sets handleMessage to the async handler that pushes the
message into receivedMessages and returns it; replace each inline
Consumer.create block with calls to this helper (refer to Consumer.create,
handleMessage and receivedMessages in the diff).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests