Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refactors the testing infrastructure to support dual-mode AWS service mocking: LocalStack and Fauxqs. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
actor Test
participant Setup as Vitest Setup
participant FauxQS as FauxQS Server
participant TestAdmin as TestAwsResourceAdmin
participant AwsSDK as AWS SDK Clients
Test->>Setup: Initialize test environment
Setup->>Setup: Check QUEUE_BACKEND
alt QUEUE_BACKEND = "fauxqs" (default)
Setup->>FauxQS: ensureFauxqsServer()
FauxQS->>FauxQS: Start server on port 4566
else QUEUE_BACKEND = "localstack"
Setup->>Setup: Skip FauxQS (use LocalStack)
end
Test->>TestAdmin: Create test resource admin
TestAdmin->>TestAdmin: Inject FauxQS server & AWS clients
Test->>TestAdmin: createQueue(name, opts)
alt Server available
TestAdmin->>FauxQS: server.createQueue(region, name, ...)
FauxQS-->>TestAdmin: Queue reference
else Using AWS SDK
TestAdmin->>AwsSDK: assertQueue(sqsClient, ...)
AwsSDK-->>TestAdmin: Queue created in LocalStack
end
TestAdmin-->>Test: Queue ready
Test->>Test: Execute test logic
Test->>TestAdmin: deleteQueues(...names)
alt Server available
TestAdmin->>FauxQS: server.deleteQueue(region, name)
else Using AWS SDK
TestAdmin->>AwsSDK: deleteQueue(sqsClient, ...)
end
TestAdmin-->>Test: Cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (5)
packages/s3-payload-store/test/utils/globalSetup.ts (1)
9-11: Sameteardown()null-safety issue as inpackages/sqs/test/utils/globalSetup.ts.
serverwill beundefinedifstartFauxqs()throws, causing a maskedTypeErrorinteardown().🛡️ Proposed fix
export async function teardown() { - await server.stop() + await server?.stop() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/s3-payload-store/test/utils/globalSetup.ts` around lines 9 - 11, The teardown() function calls server.stop() without guarding against server being undefined, which will throw if start failed; update teardown() (the teardown function in this file) to check that the server variable (the server instance created by startFauxqs/startServer) is defined before calling server.stop(), and only invoke stop() when server is non-null/undefined (or wrap the shutdown in a safe try/catch that no-ops if server is missing) so teardown() no longer throws a masked TypeError.packages/sqs/test/utils/globalSetup.ts (2)
9-11:teardown()will throw ifsetup()fails.
serveris uninitialized at module load. IfstartFauxqs()rejects (e.g., port 4566 already bound),serverremainsundefined. Vitest guaranteesteardown()runs even whensetup()throws, soserver.stop()will then throw aTypeError, hiding the original error.🛡️ Proposed fix
export async function teardown() { - await server.stop() + await server?.stop() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/utils/globalSetup.ts` around lines 9 - 11, teardown() currently calls server.stop() unconditionally which throws if setup() failed and server wasn't initialized; update teardown to only attempt stopping when server is defined and has a stop method (e.g., if (server && typeof server.stop === 'function') await server.stop()), so that teardown won't mask the original setup error thrown by startFauxqs()/setup().
3-11: Consolidate duplicateglobalSetup.tsfiles and extract hardcoded port to environment configuration.Three packages have byte-for-byte identical
globalSetup.tsfiles (packages/sqs,packages/s3-payload-store, andpackages/sns) that hardcode port 4566. While current CI configuration runs package tests serially with per-package filtering, this duplication creates maintenance burden and brittleness: changes must be propagated across three files, and the hardcoded port makes the setup fragile if test execution patterns change or tests are run concurrently locally.Extract the setup to a shared utility and parameterize the port to improve maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/utils/globalSetup.ts` around lines 3 - 11, The three identical globalSetup.ts files duplicate test setup and hardcode port 4566; consolidate by moving the start/stop logic into a shared test utility (e.g., create a single helper that wraps startFauxqs) and update the package globalSetup.ts files (functions setup, teardown and the server variable) to call that shared helper; change the helper to accept the port via environment/config (process.env.TEST_SQS_PORT or a test-config module) instead of using the literal 4566 so tests can run concurrently or be configured per environment.packages/sns/test/utils/globalSetup.ts (1)
1-11:globalSetup.tsis copy-pasted verbatim across three packages.
packages/sns/test/utils/globalSetup.ts,packages/sqs/test/utils/globalSetup.ts, andpackages/s3-payload-store/test/utils/globalSetup.tsare byte-for-byte identical. Extracting this into a shared helper (e.g., a@message-queue-toolkit/test-utilspackage or a top-leveltest/workspace utility) would eliminate three-way drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/test/utils/globalSetup.ts` around lines 1 - 11, The three identical test setup files duplicate logic for starting/stopping fauxqs; extract that into a single shared test utility and import it from each package to avoid drift. Create a new module exporting startFauxqs-based helpers (e.g., startTestFauxqs/stopTestFauxqs or a default exported setup/teardown) that encapsulate the current server variable and calls to startFauxqs({ port: 4566, logger: false, host: 'localstack' }) and server.stop(), then replace the package-local functions (setup, teardown) and the local server variable with imports of the new helpers (referencing startFauxqs, setup, teardown, and server in your refactor) from the shared test-utils package or top-level test workspace and update tests to use the centralized helpers.packages/sqs/test/utils/testAwsConfig.ts (1)
14-22:TEST_S3_CONFIGis duplicated verbatim across three packages.
packages/sqs/test/utils/testAwsConfig.ts,packages/sns/test/utils/testSnsConfig.ts, andpackages/s3-payload-store/test/utils/testS3Config.tsall declare an identicalTEST_S3_CONFIG. A shared test-utilities package (or a single file exported from a common location) would eliminate the need to keep these in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/utils/testAwsConfig.ts` around lines 14 - 22, The TEST_S3_CONFIG constant (typed as S3ClientConfig and using createLocalhostHandler) is duplicated across the three test modules; extract this constant into a single shared test-utilities export (e.g., export TEST_S3_CONFIG from a common test-utils module), re-export or import any needed types/helpers (S3ClientConfig, createLocalhostHandler) from existing packages, then replace the local TEST_S3_CONFIG definitions in the three modules with imports from the new shared module and delete the duplicate declarations.
🤖 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/test/utils/globalSetup.ts`:
- Around line 9-11: The teardown() function calls server.stop() but server may
be undefined if setup() never assigned it; add a null guard so teardown only
calls stop when server is set (e.g., check server !== undefined or if (server)
before awaiting stop) to avoid TypeError. Update the teardown() implementation
(referencing teardown(), setup(), and the server variable) to safely handle an
uninitialized server.
- Around line 5-11: The setup() uses a hardcoded port when calling startFauxqs({
port: 4566, ... }) which causes EADDRINUSE in concurrent test runs; change
setup() to pick a non-conflicting port (either pass port: 0 to auto-assign if
startFauxqs supports it, or read a package-specific env var like
process.env.FAUXQS_PORT with a fallback unique port for this package) and ensure
teardown() still stops the same server instance; update references in setup,
teardown, and the startFauxqs call to use the chosen dynamic/env-driven port.
In `@packages/sqs/test/utils/globalSetup.ts`:
- Line 6: The TEST_AWS_CONFIG for SQS and SNS is missing requestHandler
configuration so requests to localhost:4566 aren't intercepted; update the
TEST_AWS_CONFIG object in both places to include requestHandler:
createLocalhostHandler() (use the same pattern as the S3 config) so the SQS/SNS
clients use createLocalhostHandler() when instantiated.
---
Duplicate comments:
In `@packages/sns/test/utils/testSnsConfig.ts`:
- Around line 15-23: TEST_S3_CONFIG is duplicated across test packages;
consolidate it into a single shared test utility and update callers to import
it: extract the TEST_S3_CONFIG constant (including its S3ClientConfig type,
endpoint, region, credentials, and requestHandler: createLocalhostHandler())
into a common test helper module (e.g., a shared-test-utils file/library),
export it, and then replace the local definitions in the packages that currently
define TEST_S3_CONFIG (the file that contains the current TEST_S3_CONFIG
constant and the other test files that duplicate it) with imports from the new
shared module so all tests use the single exported TEST_S3_CONFIG. Ensure
createLocalhostHandler() is available or re-exported from the shared module and
preserve the S3ClientConfig typing when exporting.
---
Nitpick comments:
In `@packages/s3-payload-store/test/utils/globalSetup.ts`:
- Around line 9-11: The teardown() function calls server.stop() without guarding
against server being undefined, which will throw if start failed; update
teardown() (the teardown function in this file) to check that the server
variable (the server instance created by startFauxqs/startServer) is defined
before calling server.stop(), and only invoke stop() when server is
non-null/undefined (or wrap the shutdown in a safe try/catch that no-ops if
server is missing) so teardown() no longer throws a masked TypeError.
In `@packages/sns/test/utils/globalSetup.ts`:
- Around line 1-11: The three identical test setup files duplicate logic for
starting/stopping fauxqs; extract that into a single shared test utility and
import it from each package to avoid drift. Create a new module exporting
startFauxqs-based helpers (e.g., startTestFauxqs/stopTestFauxqs or a default
exported setup/teardown) that encapsulate the current server variable and calls
to startFauxqs({ port: 4566, logger: false, host: 'localstack' }) and
server.stop(), then replace the package-local functions (setup, teardown) and
the local server variable with imports of the new helpers (referencing
startFauxqs, setup, teardown, and server in your refactor) from the shared
test-utils package or top-level test workspace and update tests to use the
centralized helpers.
In `@packages/sqs/test/utils/globalSetup.ts`:
- Around line 9-11: teardown() currently calls server.stop() unconditionally
which throws if setup() failed and server wasn't initialized; update teardown to
only attempt stopping when server is defined and has a stop method (e.g., if
(server && typeof server.stop === 'function') await server.stop()), so that
teardown won't mask the original setup error thrown by startFauxqs()/setup().
- Around line 3-11: The three identical globalSetup.ts files duplicate test
setup and hardcode port 4566; consolidate by moving the start/stop logic into a
shared test utility (e.g., create a single helper that wraps startFauxqs) and
update the package globalSetup.ts files (functions setup, teardown and the
server variable) to call that shared helper; change the helper to accept the
port via environment/config (process.env.TEST_SQS_PORT or a test-config module)
instead of using the literal 4566 so tests can run concurrently or be configured
per environment.
In `@packages/sqs/test/utils/testAwsConfig.ts`:
- Around line 14-22: The TEST_S3_CONFIG constant (typed as S3ClientConfig and
using createLocalhostHandler) is duplicated across the three test modules;
extract this constant into a single shared test-utilities export (e.g., export
TEST_S3_CONFIG from a common test-utils module), re-export or import any needed
types/helpers (S3ClientConfig, createLocalhostHandler) from existing packages,
then replace the local TEST_S3_CONFIG definitions in the three modules with
imports from the new shared module and delete the duplicate declarations.
| export async function setup() { | ||
| server = await startFauxqs({ port: 4566, logger: false, host: 'localstack' }) | ||
| } | ||
|
|
||
| export async function teardown() { | ||
| await server.stop() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all globalSetup.ts files and check their port configurations
fd 'globalSetup.ts' --exec cat {} +Repository: kibertoad/message-queue-toolkit
Length of output: 988
🏁 Script executed:
# Check for turbo.json and CI configuration
fd 'turbo.json' --type f --exec cat {} \;
fd '.github' --type d --exec find {} -name '*.yml' -o -name '*.yaml' \; 2>/dev/null | head -20Repository: kibertoad/message-queue-toolkit
Length of output: 588
🏁 Script executed:
# Check if there are any other test configuration files that might define ports
fd 'jest.config|vitest.config|test.*setup' --type f | head -20Repository: kibertoad/message-queue-toolkit
Length of output: 553
🏁 Script executed:
# Find and read GitHub Actions workflows
fd '.github/workflows' -type d --exec find {} -name '*.yml' -o -name '*.yaml' \;Repository: kibertoad/message-queue-toolkit
Length of output: 245
🏁 Script executed:
# Check root package.json for test script
cat package.json | head -50Repository: kibertoad/message-queue-toolkit
Length of output: 755
🏁 Script executed:
# Check the vitest configs for the three packages
cat packages/sns/vitest.config.ts
cat packages/sqs/vitest.config.ts
cat packages/s3-payload-store/vitest.config.tsRepository: kibertoad/message-queue-toolkit
Length of output: 1763
🏁 Script executed:
# Check if test task is defined in package.json scripts for each package
fd 'package.json' -path '*/packages/sns/*' -o -path '*/packages/sqs/*' -o -path '*/packages/s3-payload-store/*' --exec cat {} \;Repository: kibertoad/message-queue-toolkit
Length of output: 243
🏁 Script executed:
# Find all GitHub Actions workflow files
find .github/workflows -name '*.yml' -o -name '*.yaml' 2>/dev/nullRepository: kibertoad/message-queue-toolkit
Length of output: 197
🏁 Script executed:
# Read specific workflow files
find .github/workflows -type f \( -name '*.yml' -o -name '*.yaml' \) -exec cat {} \;Repository: kibertoad/message-queue-toolkit
Length of output: 18704
🏁 Script executed:
# Check individual package.json files for test scripts
cat packages/sns/package.json | grep -A 20 '"scripts"'
cat packages/sqs/package.json | grep -A 20 '"scripts"'
cat packages/s3-payload-store/package.json | grep -A 20 '"scripts"'Repository: kibertoad/message-queue-toolkit
Length of output: 2394
Hardcoded FauxQS port 4566 is vulnerable in concurrent test scenarios.
While the current GitHub Actions CI isolates each package test in a separate job (and thus separate runner), the code has a latent vulnerability: if tests from packages/sns, packages/sqs, and packages/s3-payload-store run concurrently on the same machine—whether via npm run test:ci locally without filtering, a future CI refactor, or a monorepo test runner consolidation—they will race to bind port 4566 and cause EADDRINUSE failures.
All three packages' globalSetup.ts files hardcode startFauxqs({ port: 4566, ... }).
Recommend assigning distinct ports per package (e.g., 4566/4567/4568) or using a dynamic/free port assignment (e.g., port: 0 for auto-assignment if the library supports it).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sns/test/utils/globalSetup.ts` around lines 5 - 11, The setup() uses
a hardcoded port when calling startFauxqs({ port: 4566, ... }) which causes
EADDRINUSE in concurrent test runs; change setup() to pick a non-conflicting
port (either pass port: 0 to auto-assign if startFauxqs supports it, or read a
package-specific env var like process.env.FAUXQS_PORT with a fallback unique
port for this package) and ensure teardown() still stops the same server
instance; update references in setup, teardown, and the startFauxqs call to use
the chosen dynamic/env-driven port.
| export async function teardown() { | ||
| await server.stop() | ||
| } |
There was a problem hiding this comment.
teardown() will throw if setup() never assigned server.
server is declared but uninitialized; if setup() rejects before the await resolves, server remains undefined and teardown() will throw TypeError: Cannot read properties of undefined (reading 'stop'). While Vitest does not call teardown when setup throws, adding a null guard is defensive and aligns with the same risk present in all three identical globalSetup files.
🛡️ Proposed fix
export async function teardown() {
- await server.stop()
+ await server?.stop()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function teardown() { | |
| await server.stop() | |
| } | |
| export async function teardown() { | |
| await server?.stop() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sns/test/utils/globalSetup.ts` around lines 9 - 11, The teardown()
function calls server.stop() but server may be undefined if setup() never
assigned it; add a null guard so teardown only calls stop when server is set
(e.g., check server !== undefined or if (server) before awaiting stop) to avoid
TypeError. Update the teardown() implementation (referencing teardown(),
setup(), and the server variable) to safely handle an uninitialized server.
| let server: Awaited<ReturnType<typeof startFauxqs>> | ||
|
|
||
| export async function setup() { | ||
| server = await startFauxqs({ port: 4566, logger: false, host: 'localstack' }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the SQS test globalSetup.ts file
cat -n packages/sqs/test/utils/globalSetup.tsRepository: kibertoad/message-queue-toolkit
Length of output: 416
🏁 Script executed:
# Look for S3 test setup to compare the handler approach
find . -path "*/packages/s3*" -name "*globalSetup*" -o -path "*/packages/s3*" -name "*setup*" | head -10Repository: kibertoad/message-queue-toolkit
Length of output: 128
🏁 Script executed:
# Search for createLocalhostHandler usage
rg "createLocalhostHandler" --type ts --type js -A 3 -B 3Repository: kibertoad/message-queue-toolkit
Length of output: 2665
🏁 Script executed:
# Check for FauxQS configuration in package.json or dependencies
rg "fauxqs" -i --type jsonRepository: kibertoad/message-queue-toolkit
Length of output: 249
🏁 Script executed:
# Look for any hosts or DNS configuration files
find . -name "hosts" -o -name ".hosts" -o -name "docker-compose*" -o -name ".env*" | grep -v node_modules | head -20Repository: kibertoad/message-queue-toolkit
Length of output: 133
🏁 Script executed:
# View the full SQS test config
cat -n packages/sqs/test/utils/testAwsConfig.tsRepository: kibertoad/message-queue-toolkit
Length of output: 823
🏁 Script executed:
# Check docker-compose.yml for any localstack DNS setup
cat -n docker-compose.yml | head -50Repository: kibertoad/message-queue-toolkit
Length of output: 1945
🏁 Script executed:
# Check SNS test config as well
cat -n packages/sns/test/utils/testSnsConfig.tsRepository: kibertoad/message-queue-toolkit
Length of output: 907
Add missing requestHandler: createLocalhostHandler() to SQS and SNS client configs.
The SQS and SNS test configs currently omit requestHandler: createLocalhostHandler(), while the S3 config correctly includes it. Without this handler, requests to localhost:4566 won't be properly intercepted and routed. Add requestHandler: createLocalhostHandler() to TEST_AWS_CONFIG in both packages/sqs/test/utils/testAwsConfig.ts (after line 11) and packages/sns/test/utils/testSnsConfig.ts (after line 12), matching the pattern used in the S3 configs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sqs/test/utils/globalSetup.ts` at line 6, The TEST_AWS_CONFIG for
SQS and SNS is missing requestHandler configuration so requests to
localhost:4566 aren't intercepted; update the TEST_AWS_CONFIG object in both
places to include requestHandler: createLocalhostHandler() (use the same pattern
as the S3 config) so the SQS/SNS clients use createLocalhostHandler() when
instantiated.
…lkit into feat/fauxsq # Conflicts: # docker-compose.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/package.json`:
- Line 58: The dependency "fauxqs" is invalid and must be removed or corrected
in the package.json dependency lists for the affected packages (the SNS, SQS and
S3-payload-store packages); locate the "fauxqs": "^1.7.0" dependency entries in
each package.json, either delete the dependency if it's unused or replace it
with the correct npm package name/version if this was a typo, then run a fresh
install (npm ci / npm install) to verify the lockfile and build, and commit the
updated package.json (and package-lock.json/yarn.lock) changes.
packages/sns/package.json
Outdated
| "awilix": "^12.0.5", | ||
| "awilix": "^12.1.1", | ||
| "awilix-manager": "^6.1.0", | ||
| "fauxqs": "^1.7.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if fauxqs is defined as a workspace package
echo "=== Searching for fauxqs in package.json files ==="
find . -name "package.json" -type f ! -path "*/node_modules/*" -exec grep -l "fauxqs" {} \;
echo ""
echo "=== Checking for fauxqs package name declaration ==="
rg '"name"\s*:\s*"[^"]*fauxqs' --type json
echo ""
echo "=== Checking root-level workspaces configuration ==="
head -n 200 package.json | grep -A 20 "workspaces"
echo ""
echo "=== Listing all packages in the repo ==="
fd "package.json" --max-depth 3 --exclude "node_modules" -exec grep -H '"name"' {} \; | head -20Repository: kibertoad/message-queue-toolkit
Length of output: 1241
🌐 Web query:
fauxqs npm package
💡 Result:
I couldn’t find any public npm package named fauxqs (no npm package page or repo shows up in search results), so it may be:
- a typo (e.g., you meant something else like
fauxauth(npmjs.com)), or - a private/internal package (not publicly indexed), or
- unpublished / deleted / renamed.
To confirm from your machine:
npm view fauxqs
npm search fauxqsIf you tell me what it’s supposed to do (or paste the dependency line from package.json / an install error), I can help identify the intended package.
Citations:
🏁 Script executed:
#!/bin/bash
echo "=== packages/sns/package.json - fauxqs reference ==="
grep -n "fauxqs" packages/sns/package.json
echo ""
echo "=== packages/sqs/package.json - fauxqs reference ==="
grep -n "fauxqs" packages/sqs/package.json
echo ""
echo "=== packages/s3-payload-store/package.json - fauxqs reference ==="
grep -n "fauxqs" packages/s3-payload-store/package.json
echo ""
echo "=== Checking if fauxqs might be defined under different name ==="
find packages -name "package.json" ! -path "*/node_modules/*" -exec grep -H "querystring\|query-string\|qs" {} \; | grep -v node_modules | head -5Repository: kibertoad/message-queue-toolkit
Length of output: 666
🏁 Script executed:
#!/bin/bash
echo "=== Checking for lock files with fauxqs reference ==="
if [ -f "package-lock.json" ]; then
grep -c "fauxqs" package-lock.json 2>/dev/null || echo "No package-lock.json entries"
fi
if [ -f "pnpm-lock.yaml" ]; then
grep -c "fauxqs" pnpm-lock.yaml 2>/dev/null || echo "No pnpm-lock.yaml entries"
fi
echo ""
echo "=== Checking git log for fauxqs ==="
git log --all --oneline --grep="fauxqs" 2>/dev/null | head -5 || echo "No git commits mentioning fauxqs"
echo ""
echo "=== Checking if fauxqs could be a typo for 'qs' ==="
rg "\"qs\"" packages/*/package.json --type json | head -10Repository: kibertoad/message-queue-toolkit
Length of output: 218
Remove or replace the invalid fauxqs dependency across all affected packages.
The fauxqs package does not exist on the npm registry and is not defined as a workspace package in this repository. It's currently referenced as "fauxqs": "^1.7.0" in:
- packages/sns/package.json (line 58)
- packages/sqs/package.json (line 53)
- packages/s3-payload-store/package.json (line 44)
This will cause npm install to fail. Either remove the dependency if it's unused, or correct it to the intended package name if it's a typo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sns/package.json` at line 58, The dependency "fauxqs" is invalid and
must be removed or corrected in the package.json dependency lists for the
affected packages (the SNS, SQS and S3-payload-store packages); locate the
"fauxqs": "^1.7.0" dependency entries in each package.json, either delete the
dependency if it's unused or replace it with the correct npm package
name/version if this was a typo, then run a fresh install (npm ci / npm install)
to verify the lockfile and build, and commit the updated package.json (and
package-lock.json/yarn.lock) changes.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| opts?: { attributes?: Record<string, string>; tags?: Record<string, string> }, | ||
| ) { | ||
| if (this.server) { | ||
| return this.server.createQueue(name, { |
There was a problem hiding this comment.
all these methods would also work if we just used SDK client same way we do for localstack part, but I wanted to ensure we have feature parity on the programmatic API side (spoilers: there were gaps which needed to be filled)
it is also somewhat faster, but not to an extent that would make recommend converting any existing repos that make SDK calls to use programmatic API instead.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/sqs/lib/fakes/TestSqsPublisher.spec.ts (1)
16-18:⚠️ Potential issue | 🟠 MajorHardcoded LocalStack URLs break all
queueUrl-consuming tests under FauxqsThe URL constants bake in the LocalStack hostname and AWS account ID:
http://sqs.eu-west-1.localstack:4566/000000000000/<queue>Even though queue creation is now routed through
testAdmin.createQueue, every test that uses these constants for publishing or for the verification consumer (all "publish with queueUrl" cases, and the verification consumers in the "queueName", "consumer", "publisher", and FIFO suites) will fail whenQUEUE_BACKEND=fauxqsbecause Fauxqs exposes queues at a different base URL.The URL should be resolved dynamically — e.g., from the return value of
testAdmin.createQueueor viaGetQueueUrlCommand— rather than constructed from a hard-coded LocalStack address.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/lib/fakes/TestSqsPublisher.spec.ts` around lines 16 - 18, The tests currently hardcode queueUrl and fifoQueueUrl strings which embed a LocalStack hostname/account and break when QUEUE_BACKEND=fauxqs; update TestSqsPublisher.spec.ts to resolve queue URLs dynamically instead of constructing them: after creating queues via testAdmin.createQueue (or by calling GetQueueUrlCommand) capture and use the returned queueUrl for publishing and verification, and replace all usages of the queueUrl and fifoQueueUrl constants with those dynamic values so tests work for both LocalStack and Fauxqs backends.packages/sqs/test/publishers/SqsPermissionPublisher.fifo.spec.ts (1)
275-332:⚠️ Potential issue | 🟡 MinorMissing S3 bucket cleanup after the offloading test.
'fifo-payload-offloading-test-bucket'is created here but never cleaned up. The offloaded payload object uploaded to S3 accumulates across repeated LocalStack runs (no-op for Fauxqs since it's in-memory per run). Add anemptyBucketscall to the suite'safterEachto keep the test self-contained.🧹 Proposed fix — add bucket cleanup to
afterEach+ const s3BucketName = 'fifo-payload-offloading-test-bucket' + const queueName = 'test-fifo-queue.fifo' let diContainer: AwilixContainer<Dependencies> let sqsClient: SQSClient let testAdmin: TestAwsResourceAdmin beforeEach(async () => { diContainer = await registerDependencies() sqsClient = diContainer.cradle.sqsClient testAdmin = diContainer.cradle.testAdmin await testAdmin.deleteQueues(queueName) }) afterEach(async () => { await testAdmin.deleteQueues(queueName) + await testAdmin.emptyBuckets(s3BucketName) await diContainer.cradle.awilixManager.executeDispose() await diContainer.dispose() })And remove the local
const s3BucketNamedeclaration from inside the test body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/publishers/SqsPermissionPublisher.fifo.spec.ts` around lines 275 - 332, The test creates an S3 bucket ('fifo-payload-offloading-test-bucket') via testAdmin.createBucket but never cleans it up; remove the local const s3BucketName from the test and add a cleanup call (testAdmin.emptyBuckets or equivalent) in the suite's afterEach so the offloaded objects uploaded by the S3PayloadStore (used in payloadStoreConfig and SqsPermissionPublisherFifo) are removed between runs; ensure the afterEach references the same bucket name used when creating the bucket or empties all test buckets.packages/sqs/test/consumers/SqsPermissionConsumer.startupResourcePolling.spec.ts (1)
59-59:⚠️ Potential issue | 🟡 MinorClarify queue URL resolution mechanism with fauxqs—currently working but undocumented dependency.
The
queueUrlis hardcoded tohttp://sqs.eu-west-1.localstack:4566/…while the SQS client is configured withendpoint: 'http://localhost:4566'. The tests pass because fauxqs resolves queues by the path component (queue name) rather than requiring exact URL matching. However, this behavior is implicit and not documented. This pattern is repeated across 25+ test files (packages/sqs and packages/sns), making it a widespread undocumented assumption about fauxqs behavior.Consider adding a comment documenting that fauxqs resolves queues by path/name to make this dependency explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/consumers/SqsPermissionConsumer.startupResourcePolling.spec.ts` at line 59, The hardcoded queueUrl constant (queueUrl) relies on an implicit fauxqs behavior where queues are resolved by the path/name, which differs from the SQS client endpoint configuration (endpoint: 'http://localhost:4566'); add a concise inline comment next to the queueUrl definition in SqsPermissionConsumer.startupResourcePolling.spec.ts explaining that fauxqs matches by path/queue name (so exact host in the URL is not required), mention this pattern is repeated across tests in packages/sqs and packages/sns, and optionally include a one-line pointer to fauxqs docs or a note to change the URL if the endpoint host changes to avoid future confusion.packages/sns/test/consumers/SnsSqsPermissionConsumer.startupResourcePolling.spec.ts (1)
63-65:⚠️ Potential issue | 🟡 MinorHardcoded
queueUrlbreaks fauxqs backend — usetestAdmin.createQueue()return valueLine 65 hardcodes the LocalStack URL format (
http://sqs.eu-west-1.localstack:4566/…). WhenQUEUE_BACKENDis fauxqs,testAdmin.createQueue()returns a different URL, so:
locatorConfig.queueUrlpoints to the wrong endpoint — the consumer fails to locate the queue.expect(consumer.subscriptionProps.queueUrl).toBe(queueUrl)(e.g., lines 162, 260, 294, 327) will fail.All
testAdmin.createQueue(queueName)call-sites in this file ignore the returned URL, which already follows the correct pattern elsewhere (const { queueUrl } = await testAdmin.createQueue(queueName)inSnsPermissionPublisher.spec.tsL217 andSnsPermissionPublisher.multiStorePayloadOffloading.spec.tsL63).🐛 Suggested fix pattern (apply to each call-site)
- const queueUrl = `http://sqs.eu-west-1.localstack:4566/000000000000/${queueName}` ... - await testAdmin.createQueue(queueName) + const { queueUrl } = await testAdmin.createQueue(queueName) const consumer = new TestStartupResourcePollingConsumer(diContainer.cradle, { locatorConfig: { ... queueUrl, // now backend-agnosticAlso applies to: 100-100, 156-156, 167-167, 192-192, 227-227, 270-270, 306-306, 332-332, 392-392, 549-549, 599-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/test/consumers/SnsSqsPermissionConsumer.startupResourcePolling.spec.ts` around lines 63 - 65, The tests hardcode queueUrl which breaks fauxqs; update every testAdmin.createQueue(queueName) call in this file to capture and use its returned URL (e.g., const { queueUrl } = await testAdmin.createQueue(queueName)) and replace usages of the hardcoded queueUrl variable and any assignments to locatorConfig.queueUrl and expectations like expect(consumer.subscriptionProps.queueUrl).toBe(queueUrl) to use the returned queueUrl instead so the consumer locates the correct endpoint under both LocalStack and fauxqs backends.packages/sns/test/consumers/SnsSqsPermissionConsumer.spec.ts (1)
230-233:⚠️ Potential issue | 🟠 MajorFix SNS client configuration and extend
testAdmin.createTopicto support tags.These three tests call
assertTopic(snsClient, stsClient, { Name: topicNome, Tags: [...] })directly through HTTP, but the SNS client configuration has a critical gap: it lacks the fauxqsrequestHandlerthat S3 and SQS use. This means HTTP SNS calls likely bypass fauxqs's HTTP handlers entirely.Combined with the fact that
testAdmin.createTopic()doesn't accept atagsparameter, there's no abstracted way to create tagged topics consistently. WhenbeforeEachcallstestAdmin.deleteTopics(), it uses the programmatic API (server.deleteTopic()), which may not find HTTP-created topics if fauxqs doesn't synchronize state between HTTP handlers and its in-memory store.Required fix: Either:
- Add
requestHandler: createLocalhostHandler()to the SNS client config (matching SQS/S3 pattern), then extendtestAdmin.createTopic(name, opts?: { tags? })to pass tags toserver.createTopic(), OR- Confirm that fauxqs SNS HTTP and programmatic APIs share state, then update the tests to use the extended
testAdminabstraction.Also applies to: 268-271, 299-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/test/consumers/SnsSqsPermissionConsumer.spec.ts` around lines 230 - 233, The SNS tests create topics via HTTP using assertTopic(snsClient, stsClient, { Name: topicNome, Tags: [...] }) but the snsClient is missing the fauxqs requestHandler so HTTP requests bypass fauxqs handlers; add requestHandler: createLocalhostHandler() to the SNS client configuration (matching SQS/S3 clients) so HTTP calls hit fauxqs, and extend testAdmin.createTopic(name, opts?: { tags?: Tag[] }) to forward tags to server.createTopic(...) so tests can create tagged topics via the testAdmin abstraction; alternatively, if you prefer keeping HTTP creation, ensure fauxqs HTTP and programmatic stores are shared, but the recommended fix is to update snsClient config and testAdmin.createTopic to accept and pass tags.packages/sqs/test/consumers/SqsPermissionConsumer.spec.ts (1)
33-43:⚠️ Potential issue | 🟠 MajorHardcoded localstack queue URL may break under fauxqs backend.
Line 33 hardcodes
http://sqs.eu-west-1.localstack:4566/000000000000/${queueName}which is a localstack-specific format. Since this PR aims to run tests against bothfauxqsandlocalstack(per the CI matrix), this URL will likely differ when using fauxqs. Tests at Lines 71, 84, 115, 156, 208, 257, 288, 325, 391 all assert.toBe(queueUrl)against this hardcoded value.Consider deriving the expected URL from
testAdmin.createQueue(queueName)return value, or from the test AWS configuration, so it adapts to the active backend.#!/bin/bash # Check if there's a test config or fauxqs config that might produce matching URLs echo "=== Check TEST_AWS_CONFIG for endpoint configuration ===" rg -n 'TEST_AWS_CONFIG' --type=ts -A 10 echo "=== Check fauxqs endpoint/URL format ===" rg -n 'fauxqs' --type=ts -C 3 -g '!node_modules' echo "=== Check if queueUrl is hardcoded elsewhere ===" rg -n 'sqs.eu-west-1.localstack' --type=ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/consumers/SqsPermissionConsumer.spec.ts` around lines 33 - 43, The test hardcodes queueUrl (`const queueUrl = `http://sqs.eu-west-1.localstack:4566/000000000000/${queueName}`) which breaks when running against fauxqs; update the tests to derive the expected queue URL dynamically (use the value returned by testAdmin.createQueue(queueName) or obtain the endpoint/base URL from the test AWS configuration in the DI container) and replace direct comparisons to queueUrl with that derived value; locate usages around the spec (variables queueUrl, sqsClient, testAdmin and assertions that call .toBe(queueUrl)) and ensure createQueue/deleteQueues calls are awaited and their returned URL or config is used for assertions so the tests work for both localstack and fauxqs.
🧹 Nitpick comments (12)
packages/gcp-pubsub/test/utils/gcsUtils.ts (1)
14-25: LGTM — optional: parallelize bucket cleanupThe logic is correct. The
continueguard, per-bucket existence check, andignoreNotFoundon individual deletes are all appropriate for a test utility.If this ends up used with many buckets, consider flattening to a single
Promise.allto run bucket cleanup concurrently:♻️ Optional parallel refactor
-export async function emptyBuckets(storage: Storage, ...bucketNames: string[]) { - for (const bucketName of bucketNames) { - const bucket = storage.bucket(bucketName) - const [exists] = await bucket.exists() - - if (!exists) { - continue - } - - const [files] = await bucket.getFiles() - await Promise.all(files.map((file) => file.delete({ ignoreNotFound: true }))) - } -} +export async function emptyBuckets(storage: Storage, ...bucketNames: string[]) { + await Promise.all( + bucketNames.map(async (bucketName) => { + const bucket = storage.bucket(bucketName) + const [exists] = await bucket.exists() + if (!exists) return + const [files] = await bucket.getFiles() + await Promise.all(files.map((file) => file.delete({ ignoreNotFound: true }))) + }), + ) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gcp-pubsub/test/utils/gcsUtils.ts` around lines 14 - 25, The current emptyBuckets function sequentially processes each bucket; to parallelize, map over bucketNames and create an async cleanup promise per bucket that calls storage.bucket(bucketName), awaits bucket.exists(), short-circuits if not exists, then awaits bucket.getFiles() and Promise.all(files.map(f => f.delete({ ignoreNotFound: true }))); finally return Promise.all on that array of bucket cleanup promises. Update emptyBuckets to return that single Promise so all bucket deletions run concurrently while preserving the existing checks (bucket.exists, bucket.getFiles, file.delete with ignoreNotFound).packages/sqs/lib/fakes/TestSqsPublisher.spec.ts (1)
11-11:deleteQueueinbeforeEachbypasses the Fauxqs-awaretestAdmin.deleteQueues()abstractionLines 30-31 call
deleteQueue(sqsClient, ...)directly while the rest of the provisioning was migrated to go throughtestAdmin(which routes toserver.deleteQueuefor Fauxqs vs. the SQS SDK for LocalStack). If Fauxqs requires routing deletions through the server handle rather than via the SQS-protocol endpoint, these teardown calls will silently no-op or error underQUEUE_BACKEND=fauxqs, leaving stale queues between test runs.♻️ Suggested fix
- await deleteQueue(sqsClient, queueName) - await deleteQueue(sqsClient, fifoQueueName) + await testAdmin.deleteQueues(queueName, fifoQueueName)Also applies to: 30-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/lib/fakes/TestSqsPublisher.spec.ts` at line 11, The beforeEach teardown currently calls deleteQueue(sqsClient, ...) which bypasses the Fauxqs-aware abstraction; replace those direct deleteQueue calls with the testAdmin.deleteQueues(...) call used elsewhere so teardown routes through server.deleteQueue for QUEUE_BACKEND=fauxqs; update the TestSqsPublisher.spec.ts beforeEach to invoke testAdmin.deleteQueues with the same queue names/urls (and remove or stop importing deleteQueue) so teardown behaves correctly under both Fauxqs and LocalStack.packages/sqs/test/utils/s3Utils.ts (2)
15-30:emptyBucketsis duplicated verbatim acrosspackages/snsandpackages/sqsThe implementation at lines 15–30 is byte-for-byte identical to
packages/sns/test/utils/s3Utils.tslines 15–30. Consider extracting it to a shared test-utilities package to keep both copies in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/utils/s3Utils.ts` around lines 15 - 30, The function emptyBuckets is duplicated verbatim; extract this helper into a single shared test utilities module and have both packages import it instead of maintaining separate copies: create a shared export (e.g., export async function emptyBuckets(s3: S3, ...bucketNames: string[])) in the common test-utils package, move the current implementation (including the try/catch that checks NoSuchBucket) there, update both packages to import that symbol instead of their local versions, and remove the duplicated definitions so only the shared implementation remains.
18-18: UselistObjectsV2instead oflistObjects
ListObjectsis deprecated by AWS; useListObjectsV2for new development. While the current 1,000-object limit is acceptable for test buckets in practice,listObjectsV2with proper pagination (IsTruncatedchecking) would be more robust if bucket sizes grow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/utils/s3Utils.ts` at line 18, Replace the deprecated s3.listObjects call with s3.listObjectsV2 in the helper that currently calls "const objects = await s3.listObjects({ Bucket: bucketName })", and implement proper pagination: call listObjectsV2 with Bucket and optional ContinuationToken, loop while IsTruncated is true, accumulate returned Contents into a single array, and pass the accumulated list to the rest of the function; ensure you reference/return the same variable names (e.g., objects or contents) so callers of the helper (in s3Utils.ts) continue to work.packages/sns/test/publishers/SnsPermissionPublisher.payloadOffloading.spec.ts (1)
37-55:testAdminwiring is correct; consider addingemptyBucketscleanup inafterAll.The
TestAwsResourceAdminexposes anemptyBucketsmethod, andcreateBucketis called inbeforeAllwith no corresponding cleanup inafterAll. For Fauxqs this is harmless (server teardown cleans up), but for LocalStack, offloaded payload objects accumulate ins3BucketNameacross test runs. Adding an explicit cleanup keeps both backends symmetric:🧹 Suggested `afterAll` addition
afterAll(async () => { + await testAdmin.emptyBuckets(s3BucketName) const { awilixManager } = diContainer.cradle await awilixManager.executeDispose() await diContainer.dispose() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/test/publishers/SnsPermissionPublisher.payloadOffloading.spec.ts` around lines 37 - 55, The test creates an S3 bucket via TestAwsResourceAdmin in beforeAll but doesn't clean it up; add an afterAll that calls testAdmin.emptyBuckets(s3BucketName) (or the appropriate emptyBuckets method on the TestAwsResourceAdmin instance named testAdmin) to remove offloaded payload objects after tests complete so LocalStack runs don't accumulate data; place this cleanup in the same test file alongside the existing beforeAll/afterAll hooks.packages/sns/test/utils/fauxqsInstance.ts (2)
1-21:fauxqsInstance.tsis duplicated verbatim frompackages/sqs/test/utils/fauxqsInstance.ts.Both files are identical (
isLocalstackflag,serversingleton,ensureFauxqsServer,getFauxqsServer). The duplication will drift. Consider extracting a sharedtest-utilspackage in the monorepo, or having SNS import the SQS utility directly if a package-level dependency is acceptable for test utilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/test/utils/fauxqsInstance.ts` around lines 1 - 21, The fauxqsInstance.ts file is a verbatim duplicate; consolidate the test utility by removing duplication and having SNS tests reuse the single implementation: either move the shared logic (isLocalstack, server singleton, ensureFauxqsServer, getFauxqsServer) into a common test-utils package in the monorepo and update imports, or have the SNS package import the existing implementation from packages/sqs/test/utils/fauxqsInstance.ts (add a test-only dependency if needed); update any import paths in SNS tests and delete the duplicate file so there is a single source of truth.
17-21:getFauxqsServer()logs on every call — consider removing or guarding the log.This function is called during DI container setup (once per test file), so it will emit a log for every test file in the suite. Given that
ensureFauxqsServer()already logs state transitions, the per-call log ingetFauxqsServeradds limited signal. The same pattern exists inpackages/sqs/test/utils/fauxqsInstance.ts.♻️ Suggested simplification
export function getFauxqsServer(): FauxqsServer | undefined { - console.log( - `[fauxqs] getFauxqsServer: ${server ? 'instance available' : 'undefined (localstack mode)'}`, - ) return server }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/test/utils/fauxqsInstance.ts` around lines 17 - 21, Remove the unconditional console.log in getFauxqsServer (which currently logs server state on every call) and either delete the log entirely or guard it behind a debug/verbose flag (e.g., if (process.env.DEBUG_FAUXQS) { ... }) or replace it with a logger that respects log levels; update both getFauxqsServer implementations that reference the server variable and ensureFauxqsServer so normal test runs are quiet while detailed logging can be enabled when needed.packages/sns/test/utils/testAdmin.ts (1)
10-100:TestAwsResourceAdminis largely duplicated frompackages/sqs/test/utils/testAdmin.ts.
createQueue,deleteQueues,createBucket, andemptyBucketsare byte-for-byte identical between the SNS and SQS versions. The SNS variant simply addscreateTopic/deleteTopicsand makessnsClient/stsClientrequired. Consider extracting a shared base class (e.g. in a monorepo-leveltest-utilspackage, or by importing the SQS class as a base) so queue/bucket logic lives in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/test/utils/testAdmin.ts` around lines 10 - 100, TestAwsResourceAdmin duplicates SQS test admin logic; extract the shared queue/bucket behavior into a common base class (or move into a monorepo test-utils module) and have the SNS TestAwsResourceAdmin extend/use it. Specifically, factor out the duplicated methods createQueue, deleteQueues, createBucket, and emptyBuckets into a BaseTestAwsResourceAdmin (or shared helper) that accepts server, sqsClient, s3, and region, then update the SNS TestAwsResourceAdmin to keep its SNS-specific fields (snsClient, stsClient) and implement createTopic/deleteTopics while delegating queue/bucket calls to the new base/shared implementation.packages/sns/test/utils/snsSubscriber.spec.ts (1)
34-41: Optional: move AWS resource cleanup before container disposal
deleteQueues/deleteTopicsare called after bothawilixManager.executeDispose()anddiContainer.dispose(). This works today because the SQS/SNS clients have no dispose hooks, but it inverts the safer pattern used elsewhere in the suite (e.g.,startupResourcePolling.spec.tscleans up inbeforeEachbefore disposal). If a dispose hook is ever added to these clients, cleanup would silently fail.♻️ Suggested reorder
afterEach(async () => { + await testAdmin.deleteQueues(QUEUE_NAME) + await testAdmin.deleteTopics(TOPIC_NAME) const { awilixManager } = diContainer.cradle await awilixManager.executeDispose() await diContainer.dispose() - - await testAdmin.deleteQueues(QUEUE_NAME) - await testAdmin.deleteTopics(TOPIC_NAME) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sns/test/utils/snsSubscriber.spec.ts` around lines 34 - 41, Move the AWS resource cleanup (calls to testAdmin.deleteQueues(QUEUE_NAME) and testAdmin.deleteTopics(TOPIC_NAME)) to run before container disposal so they execute while the SNS/SQS clients are still available; specifically, in the afterEach in snsSubscriber.spec.ts, call testAdmin.deleteQueues and testAdmin.deleteTopics before invoking awilixManager.executeDispose() and diContainer.dispose(). Keep the same error handling/await semantics and ensure the cleanup order mirrors startupResourcePolling.spec.ts so future dispose hooks on the clients don't prevent deleting AWS resources.packages/sqs/test/utils/fauxqsInstance.ts (2)
1-22:fauxqsInstance.tsis identical topackages/sns/test/utils/fauxqsInstance.ts— extract to a shared helper.Both the SQS and SNS packages contain byte-for-byte duplicate implementations of
ensureFauxqsServer/getFauxqsServer. Any future change (port, host, logger config) must be applied twice. Consider moving these to a shared internal package (e.g.packages/test-utils) or at minimum a shared workspace helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/utils/fauxqsInstance.ts` around lines 1 - 22, The fauxqsInstance.ts functions ensureFauxqsServer and getFauxqsServer (and the shared server variable and startFauxqs import) are duplicated across packages; extract them into a single shared test utility module (e.g., packages/test-utils) and have both SQS and SNS tests import that module. Move the constants (isLocalstack), the server variable, and the functions ensureFauxqsServer and getFauxqsServer plus the startFauxqs import into the new shared file, update imports in packages/sqs/test/utils/fauxqsInstance.ts and packages/sns/test/utils/fauxqsInstance.ts to re-export or forward to the shared helper, and ensure behavior (port 4566, host 'localstack', logger:false and console logs) remains identical after the change.
17-21:getFauxqsServer()logs on every call — can be noisy withbeforeEach-scoped containers.In test files that call
registerDependencies()insidebeforeEach(e.g.SqsEventBridgeConsumer.spec.ts), a new DI container is created per test. AlthoughtestAdminusesSINGLETON_CONFIGwithin a single container,getFauxqsServer()is invoked once per container, emitting a log line for every test. At test-suite scale this adds significant log noise.Consider either removing this log (the startup log in
ensureFauxqsServeralready covers the "server is running" signal) or downgrading to a debug-level logger.✨ Proposed change
export function getFauxqsServer(): FauxqsServer | undefined { - console.log( - `[fauxqs] getFauxqsServer: ${server ? 'instance available' : 'undefined (localstack mode)'}`, - ) return server }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/utils/fauxqsInstance.ts` around lines 17 - 21, getFauxqsServer() currently emits a console.log on every call, causing noisy test output when DI containers are recreated; remove the console.log line (or replace it with a debug-level log call) inside getFauxqsServer so it no longer logs on each invocation, and rely on the existing startup log in ensureFauxqsServer for the server-running signal; locate the console.log in the getFauxqsServer function and either delete it or change it to a debug call (e.g., processLogger.debug or a configured logger) to suppress test noise.packages/sqs/test/utils/testAdmin.ts (1)
10-32: UnusedsnsClientandstsClientfields.These fields are stored but never referenced by any method. If they're planned for future SNS topic/STS operations, consider deferring their addition until actually needed (YAGNI), or add a brief comment indicating intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqs/test/utils/testAdmin.ts` around lines 10 - 32, The TestAwsResourceAdmin class stores unused fields snsClient and stsClient; remove these unused properties (snsClient and stsClient) and their assignments in the constructor, or if you intend to keep them, add a brief TODO comment above the private declarations indicating planned SNS/STS usage; update the constructor parameter list to remove the corresponding opts.snsClient and opts.stsClient entries (or keep them but document intent) and ensure no other code references TestAwsResourceAdmin.snsClient or .stsClient so the class no longer contains dead members.
🤖 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/test/utils/s3Utils.ts`:
- Around line 25-27: The catch block currently swallows all exceptions because
the body for "if (e instanceof NoSuchBucket) {}" is empty and non-NoSuchBucket
errors are not rethrown; update the catch to only suppress NoSuchBucket and
rethrow any other error (e.g., change to "if (e instanceof NoSuchBucket) { /*
ignore */ } else { throw e }" or equivalently "if (!(e instanceof NoSuchBucket))
throw e") so that errors like AccessDenied or network failures are propagated;
locate the catch in packages/sns/test/utils/s3Utils.ts around the S3 operation
that references NoSuchBucket and implement the rethrow logic there.
In `@packages/sns/test/utils/testAdmin.ts`:
- Around line 84-100: The createBucket and emptyBuckets methods use a non-null
assertion on this.s3 (calling assertBucket(this.s3!, name) and
emptyBuckets(this.s3!, ...names)) which will throw if the admin was constructed
without an S3 instance; add an explicit runtime guard that checks for this.s3
before calling the helper functions and throw a clear error or return when
missing (e.g., in TestAwsResourceAdmin.createBucket and
TestAwsResourceAdmin.emptyBuckets, after the server branch, if (!this.s3) throw
new Error("S3 client required for non-server operations") or similar) so callers
get a clear message instead of a TypeError; keep references to assertBucket and
emptyBuckets helper calls intact.
In
`@packages/sqs/test/publishers/SqsPermissionPublisher.multiStorePayloadOffloading.spec.ts`:
- Around line 48-49: The tests create S3 buckets via testAdmin.createBucket
(s3BucketNameStore1, s3BucketNameStore2) in beforeAll but never clean them up;
add an afterAll that calls testAdmin.emptyBuckets (or
testAdmin.emptyBuckets([s3BucketNameStore1, s3BucketNameStore2])) to remove
contents and avoid accumulation when QUEUE_BACKEND=localstack — apply the same
cleanup for the other bucket-creation block referenced around the second
occurrence (lines ~74-78) so both test setups call testAdmin.emptyBuckets in
their respective afterAll hooks.
In `@packages/sqs/test/utils/s3Utils.ts`:
- Around line 25-27: In the catch block that currently reads "catch (e) { if (e
instanceof NoSuchBucket) { }", change the logic so only NoSuchBucket is
swallowed and all other errors are rethrown: detect NoSuchBucket and ignore it,
otherwise throw the caught error (e) so AccessDenied/throttling/other failures
are surfaced; update the catch in the function surrounding the S3 bucket
teardown (the try/catch handling bucket deletion) to follow this behavior.
In `@packages/sqs/test/utils/testAdmin.ts`:
- Around line 64-70: createBucket (and likewise emptyBuckets) currently uses a
non-null assertion on this.s3 which will throw if neither server nor s3 were
provided; add an early guard in createBucket (and emptyBuckets) that checks if
this.server is falsy and this.s3 is falsy, and in that case throw or return a
clear, descriptive error (e.g., "S3 client not available: provide server or s3")
instead of using this.s3! and calling assertBucket; reference the methods
createBucket and emptyBuckets and the helper assertBucket when adding the guard.
- Around line 52-62: The server-side calls in deleteQueues, createBucket, and
emptyBuckets are invoked fire-and-forget; update deleteQueues to await each
this.server.deleteQueue(...) (or collect and await Promise.all over queueNames),
update createBucket to await this.server.createBucket(...), and update
emptyBuckets to await each this.server.emptyBucket(...) (or await Promise.all)
so the async server operations complete before the test cleanup proceeds; refer
to the deleteQueues, createBucket, and emptyBuckets functions and their
this.server.deleteQueue(), this.server.createBucket(), this.server.emptyBucket()
calls to apply the changes.
In `@packages/sqs/test/utils/testAwsConfig.ts`:
- Around line 26-36: The top-level await import('fauxqs') that assigns
createLocalhostHandler into s3Config runs at module evaluation and can fail if
the dev dependency is absent; change this to a lazy dynamic import by moving the
import into a function that constructs/returns the s3Config at call-time (e.g.,
add an async function like buildLocalS3Config or getS3Config that does const {
createLocalhostHandler } = await import('fauxqs') and then returns the s3Config
object with requestHandler: createLocalhostHandler()); update callers to use
that function so createLocalhostHandler is only imported when needed and not
during module initialization, keeping the symbol names createLocalhostHandler
and s3Config for easy location.
In `@turbo.json`:
- Line 12: Parallel test execution causes both packages to call startFauxqs({
port: 4566, host: 'localstack' }) and race to bind the same port; fix by either
(A) assigning per-package ports and updating client endpoints (e.g., change
startFauxqs call in packages/sqs and packages/sns to different port numbers and
update any client initialization that relies on QUEUE_BACKEND/localstack
endpoint) or (B) make turbo run "test:coverage" sequential when fauxqs is used
by adding a dependsOn guard in turbo.json so packages run one after another when
QUEUE_BACKEND=localstack; locate and update the startFauxqs calls in your
vitest.setup.ts/globalSetup and the turbo.json "test:coverage" entry
accordingly.
---
Outside diff comments:
In `@packages/sns/test/consumers/SnsSqsPermissionConsumer.spec.ts`:
- Around line 230-233: The SNS tests create topics via HTTP using
assertTopic(snsClient, stsClient, { Name: topicNome, Tags: [...] }) but the
snsClient is missing the fauxqs requestHandler so HTTP requests bypass fauxqs
handlers; add requestHandler: createLocalhostHandler() to the SNS client
configuration (matching SQS/S3 clients) so HTTP calls hit fauxqs, and extend
testAdmin.createTopic(name, opts?: { tags?: Tag[] }) to forward tags to
server.createTopic(...) so tests can create tagged topics via the testAdmin
abstraction; alternatively, if you prefer keeping HTTP creation, ensure fauxqs
HTTP and programmatic stores are shared, but the recommended fix is to update
snsClient config and testAdmin.createTopic to accept and pass tags.
In
`@packages/sns/test/consumers/SnsSqsPermissionConsumer.startupResourcePolling.spec.ts`:
- Around line 63-65: The tests hardcode queueUrl which breaks fauxqs; update
every testAdmin.createQueue(queueName) call in this file to capture and use its
returned URL (e.g., const { queueUrl } = await testAdmin.createQueue(queueName))
and replace usages of the hardcoded queueUrl variable and any assignments to
locatorConfig.queueUrl and expectations like
expect(consumer.subscriptionProps.queueUrl).toBe(queueUrl) to use the returned
queueUrl instead so the consumer locates the correct endpoint under both
LocalStack and fauxqs backends.
In `@packages/sqs/lib/fakes/TestSqsPublisher.spec.ts`:
- Around line 16-18: The tests currently hardcode queueUrl and fifoQueueUrl
strings which embed a LocalStack hostname/account and break when
QUEUE_BACKEND=fauxqs; update TestSqsPublisher.spec.ts to resolve queue URLs
dynamically instead of constructing them: after creating queues via
testAdmin.createQueue (or by calling GetQueueUrlCommand) capture and use the
returned queueUrl for publishing and verification, and replace all usages of the
queueUrl and fifoQueueUrl constants with those dynamic values so tests work for
both LocalStack and Fauxqs backends.
In `@packages/sqs/test/consumers/SqsPermissionConsumer.spec.ts`:
- Around line 33-43: The test hardcodes queueUrl (`const queueUrl =
`http://sqs.eu-west-1.localstack:4566/000000000000/${queueName}`) which breaks
when running against fauxqs; update the tests to derive the expected queue URL
dynamically (use the value returned by testAdmin.createQueue(queueName) or
obtain the endpoint/base URL from the test AWS configuration in the DI
container) and replace direct comparisons to queueUrl with that derived value;
locate usages around the spec (variables queueUrl, sqsClient, testAdmin and
assertions that call .toBe(queueUrl)) and ensure createQueue/deleteQueues calls
are awaited and their returned URL or config is used for assertions so the tests
work for both localstack and fauxqs.
In
`@packages/sqs/test/consumers/SqsPermissionConsumer.startupResourcePolling.spec.ts`:
- Line 59: The hardcoded queueUrl constant (queueUrl) relies on an implicit
fauxqs behavior where queues are resolved by the path/name, which differs from
the SQS client endpoint configuration (endpoint: 'http://localhost:4566'); add a
concise inline comment next to the queueUrl definition in
SqsPermissionConsumer.startupResourcePolling.spec.ts explaining that fauxqs
matches by path/queue name (so exact host in the URL is not required), mention
this pattern is repeated across tests in packages/sqs and packages/sns, and
optionally include a one-line pointer to fauxqs docs or a note to change the URL
if the endpoint host changes to avoid future confusion.
In `@packages/sqs/test/publishers/SqsPermissionPublisher.fifo.spec.ts`:
- Around line 275-332: The test creates an S3 bucket
('fifo-payload-offloading-test-bucket') via testAdmin.createBucket but never
cleans it up; remove the local const s3BucketName from the test and add a
cleanup call (testAdmin.emptyBuckets or equivalent) in the suite's afterEach so
the offloaded objects uploaded by the S3PayloadStore (used in payloadStoreConfig
and SqsPermissionPublisherFifo) are removed between runs; ensure the afterEach
references the same bucket name used when creating the bucket or empties all
test buckets.
---
Duplicate comments:
In `@packages/sqs/test/publishers/SqsPermissionPublisher.spec.ts`:
- Around line 29-34: Tests hardcode the Localstack URL and assert against that
literal (seen around assertions referencing the hardcoded format); update the
tests to derive the expected Localstack endpoint dynamically instead of using
the literal. In SqsPermissionPublisher.spec.ts use an existing runtime value
(for example diContainer.cradle.testAdmin or diContainer.cradle.sqsClient) to
build the expected URL—e.g. call testAdmin.getEndpoint() or read
sqsClient.config.endpoint or an env var the test harness provides—and replace
all assertions that compare to the hardcoded URL (the comparisons around
queueName and the listed assertion locations) to use that derived value so the
tests work under fauxqs/localstack configurations.
---
Nitpick comments:
In `@packages/gcp-pubsub/test/utils/gcsUtils.ts`:
- Around line 14-25: The current emptyBuckets function sequentially processes
each bucket; to parallelize, map over bucketNames and create an async cleanup
promise per bucket that calls storage.bucket(bucketName), awaits
bucket.exists(), short-circuits if not exists, then awaits bucket.getFiles() and
Promise.all(files.map(f => f.delete({ ignoreNotFound: true }))); finally return
Promise.all on that array of bucket cleanup promises. Update emptyBuckets to
return that single Promise so all bucket deletions run concurrently while
preserving the existing checks (bucket.exists, bucket.getFiles, file.delete with
ignoreNotFound).
In
`@packages/sns/test/publishers/SnsPermissionPublisher.payloadOffloading.spec.ts`:
- Around line 37-55: The test creates an S3 bucket via TestAwsResourceAdmin in
beforeAll but doesn't clean it up; add an afterAll that calls
testAdmin.emptyBuckets(s3BucketName) (or the appropriate emptyBuckets method on
the TestAwsResourceAdmin instance named testAdmin) to remove offloaded payload
objects after tests complete so LocalStack runs don't accumulate data; place
this cleanup in the same test file alongside the existing beforeAll/afterAll
hooks.
In `@packages/sns/test/utils/fauxqsInstance.ts`:
- Around line 1-21: The fauxqsInstance.ts file is a verbatim duplicate;
consolidate the test utility by removing duplication and having SNS tests reuse
the single implementation: either move the shared logic (isLocalstack, server
singleton, ensureFauxqsServer, getFauxqsServer) into a common test-utils package
in the monorepo and update imports, or have the SNS package import the existing
implementation from packages/sqs/test/utils/fauxqsInstance.ts (add a test-only
dependency if needed); update any import paths in SNS tests and delete the
duplicate file so there is a single source of truth.
- Around line 17-21: Remove the unconditional console.log in getFauxqsServer
(which currently logs server state on every call) and either delete the log
entirely or guard it behind a debug/verbose flag (e.g., if
(process.env.DEBUG_FAUXQS) { ... }) or replace it with a logger that respects
log levels; update both getFauxqsServer implementations that reference the
server variable and ensureFauxqsServer so normal test runs are quiet while
detailed logging can be enabled when needed.
In `@packages/sns/test/utils/snsSubscriber.spec.ts`:
- Around line 34-41: Move the AWS resource cleanup (calls to
testAdmin.deleteQueues(QUEUE_NAME) and testAdmin.deleteTopics(TOPIC_NAME)) to
run before container disposal so they execute while the SNS/SQS clients are
still available; specifically, in the afterEach in snsSubscriber.spec.ts, call
testAdmin.deleteQueues and testAdmin.deleteTopics before invoking
awilixManager.executeDispose() and diContainer.dispose(). Keep the same error
handling/await semantics and ensure the cleanup order mirrors
startupResourcePolling.spec.ts so future dispose hooks on the clients don't
prevent deleting AWS resources.
In `@packages/sns/test/utils/testAdmin.ts`:
- Around line 10-100: TestAwsResourceAdmin duplicates SQS test admin logic;
extract the shared queue/bucket behavior into a common base class (or move into
a monorepo test-utils module) and have the SNS TestAwsResourceAdmin extend/use
it. Specifically, factor out the duplicated methods createQueue, deleteQueues,
createBucket, and emptyBuckets into a BaseTestAwsResourceAdmin (or shared
helper) that accepts server, sqsClient, s3, and region, then update the SNS
TestAwsResourceAdmin to keep its SNS-specific fields (snsClient, stsClient) and
implement createTopic/deleteTopics while delegating queue/bucket calls to the
new base/shared implementation.
In `@packages/sqs/lib/fakes/TestSqsPublisher.spec.ts`:
- Line 11: The beforeEach teardown currently calls deleteQueue(sqsClient, ...)
which bypasses the Fauxqs-aware abstraction; replace those direct deleteQueue
calls with the testAdmin.deleteQueues(...) call used elsewhere so teardown
routes through server.deleteQueue for QUEUE_BACKEND=fauxqs; update the
TestSqsPublisher.spec.ts beforeEach to invoke testAdmin.deleteQueues with the
same queue names/urls (and remove or stop importing deleteQueue) so teardown
behaves correctly under both Fauxqs and LocalStack.
In `@packages/sqs/test/utils/fauxqsInstance.ts`:
- Around line 1-22: The fauxqsInstance.ts functions ensureFauxqsServer and
getFauxqsServer (and the shared server variable and startFauxqs import) are
duplicated across packages; extract them into a single shared test utility
module (e.g., packages/test-utils) and have both SQS and SNS tests import that
module. Move the constants (isLocalstack), the server variable, and the
functions ensureFauxqsServer and getFauxqsServer plus the startFauxqs import
into the new shared file, update imports in
packages/sqs/test/utils/fauxqsInstance.ts and
packages/sns/test/utils/fauxqsInstance.ts to re-export or forward to the shared
helper, and ensure behavior (port 4566, host 'localstack', logger:false and
console logs) remains identical after the change.
- Around line 17-21: getFauxqsServer() currently emits a console.log on every
call, causing noisy test output when DI containers are recreated; remove the
console.log line (or replace it with a debug-level log call) inside
getFauxqsServer so it no longer logs on each invocation, and rely on the
existing startup log in ensureFauxqsServer for the server-running signal; locate
the console.log in the getFauxqsServer function and either delete it or change
it to a debug call (e.g., processLogger.debug or a configured logger) to
suppress test noise.
In `@packages/sqs/test/utils/s3Utils.ts`:
- Around line 15-30: The function emptyBuckets is duplicated verbatim; extract
this helper into a single shared test utilities module and have both packages
import it instead of maintaining separate copies: create a shared export (e.g.,
export async function emptyBuckets(s3: S3, ...bucketNames: string[])) in the
common test-utils package, move the current implementation (including the
try/catch that checks NoSuchBucket) there, update both packages to import that
symbol instead of their local versions, and remove the duplicated definitions so
only the shared implementation remains.
- Line 18: Replace the deprecated s3.listObjects call with s3.listObjectsV2 in
the helper that currently calls "const objects = await s3.listObjects({ Bucket:
bucketName })", and implement proper pagination: call listObjectsV2 with Bucket
and optional ContinuationToken, loop while IsTruncated is true, accumulate
returned Contents into a single array, and pass the accumulated list to the rest
of the function; ensure you reference/return the same variable names (e.g.,
objects or contents) so callers of the helper (in s3Utils.ts) continue to work.
In `@packages/sqs/test/utils/testAdmin.ts`:
- Around line 10-32: The TestAwsResourceAdmin class stores unused fields
snsClient and stsClient; remove these unused properties (snsClient and
stsClient) and their assignments in the constructor, or if you intend to keep
them, add a brief TODO comment above the private declarations indicating planned
SNS/STS usage; update the constructor parameter list to remove the corresponding
opts.snsClient and opts.stsClient entries (or keep them but document intent) and
ensure no other code references TestAwsResourceAdmin.snsClient or .stsClient so
the class no longer contains dead members.
| } catch (e) { | ||
| if (e instanceof NoSuchBucket) { | ||
| } |
There was a problem hiding this comment.
Non-NoSuchBucket errors are silently swallowed
The catch block catches all exceptions, but the if (e instanceof NoSuchBucket) {} body is empty and there is no else/throw. Any error that is not NoSuchBucket (e.g., AccessDenied, network failure) falls through the if and exits the catch block without being rethrown — silently lost. The intent is clearly to only ignore NoSuchBucket, but the condition is inverted.
🐛 Proposed fix
- } catch (e) {
- if (e instanceof NoSuchBucket) {
- }
- }
+ } catch (e) {
+ if (!(e instanceof NoSuchBucket)) {
+ throw e
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e) { | |
| if (e instanceof NoSuchBucket) { | |
| } | |
| } catch (e) { | |
| if (!(e instanceof NoSuchBucket)) { | |
| throw e | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sns/test/utils/s3Utils.ts` around lines 25 - 27, The catch block
currently swallows all exceptions because the body for "if (e instanceof
NoSuchBucket) {}" is empty and non-NoSuchBucket errors are not rethrown; update
the catch to only suppress NoSuchBucket and rethrow any other error (e.g.,
change to "if (e instanceof NoSuchBucket) { /* ignore */ } else { throw e }" or
equivalently "if (!(e instanceof NoSuchBucket)) throw e") so that errors like
AccessDenied or network failures are propagated; locate the catch in
packages/sns/test/utils/s3Utils.ts around the S3 operation that references
NoSuchBucket and implement the rethrow logic there.
| async createBucket(name: string) { | ||
| if (this.server) { | ||
| this.server.createBucket(name) | ||
| return | ||
| } | ||
| return await assertBucket(this.s3!, name) | ||
| } | ||
|
|
||
| async emptyBuckets(...names: string[]) { | ||
| if (this.server) { | ||
| for (const name of names) { | ||
| this.server.emptyBucket(name) | ||
| } | ||
| return | ||
| } | ||
| return await emptyBuckets(this.s3!, ...names) | ||
| } |
There was a problem hiding this comment.
this.s3! will throw at runtime if s3 was not provided and the localstack path is taken.
s3 is declared optional (s3?: S3) in the constructor. In createBucket and emptyBuckets, the non-server branch calls assertBucket(this.s3!, name) / emptyBuckets(this.s3!, ...). If a caller constructs TestAwsResourceAdmin without an S3 instance and later exercises bucket operations against localstack, this will be a runtime TypeError that TypeScript's non-null assertion hides.
🛡️ Add an explicit guard
async createBucket(name: string) {
if (this.server) {
this.server.createBucket(name)
return
}
+ if (!this.s3) throw new Error('TestAwsResourceAdmin: s3 client is required for localstack bucket operations')
return await assertBucket(this.s3!, name)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async createBucket(name: string) { | |
| if (this.server) { | |
| this.server.createBucket(name) | |
| return | |
| } | |
| return await assertBucket(this.s3!, name) | |
| } | |
| async emptyBuckets(...names: string[]) { | |
| if (this.server) { | |
| for (const name of names) { | |
| this.server.emptyBucket(name) | |
| } | |
| return | |
| } | |
| return await emptyBuckets(this.s3!, ...names) | |
| } | |
| async createBucket(name: string) { | |
| if (this.server) { | |
| this.server.createBucket(name) | |
| return | |
| } | |
| if (!this.s3) throw new Error('TestAwsResourceAdmin: s3 client is required for localstack bucket operations') | |
| return await assertBucket(this.s3!, name) | |
| } | |
| async emptyBuckets(...names: string[]) { | |
| if (this.server) { | |
| for (const name of names) { | |
| this.server.emptyBucket(name) | |
| } | |
| return | |
| } | |
| return await emptyBuckets(this.s3!, ...names) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sns/test/utils/testAdmin.ts` around lines 84 - 100, The createBucket
and emptyBuckets methods use a non-null assertion on this.s3 (calling
assertBucket(this.s3!, name) and emptyBuckets(this.s3!, ...names)) which will
throw if the admin was constructed without an S3 instance; add an explicit
runtime guard that checks for this.s3 before calling the helper functions and
throw a clear error or return when missing (e.g., in
TestAwsResourceAdmin.createBucket and TestAwsResourceAdmin.emptyBuckets, after
the server branch, if (!this.s3) throw new Error("S3 client required for
non-server operations") or similar) so callers get a clear message instead of a
TypeError; keep references to assertBucket and emptyBuckets helper calls intact.
| await testAdmin.createBucket(s3BucketNameStore1) | ||
| await testAdmin.createBucket(s3BucketNameStore2) |
There was a problem hiding this comment.
S3 buckets are created in beforeAll but never cleaned up in afterAll.
With fauxqs (in-memory, process-scoped) this is harmless, but when QUEUE_BACKEND=localstack the bucket contents accumulate across test runs. testAdmin.emptyBuckets already exists for exactly this purpose.
🧹 Proposed cleanup in `afterAll`
afterAll(async () => {
+ await testAdmin.emptyBuckets(s3BucketNameStore1, s3BucketNameStore2)
const { awilixManager } = diContainer.cradle
await awilixManager.executeDispose()
await diContainer.dispose()
})Also applies to: 74-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/sqs/test/publishers/SqsPermissionPublisher.multiStorePayloadOffloading.spec.ts`
around lines 48 - 49, The tests create S3 buckets via testAdmin.createBucket
(s3BucketNameStore1, s3BucketNameStore2) in beforeAll but never clean them up;
add an afterAll that calls testAdmin.emptyBuckets (or
testAdmin.emptyBuckets([s3BucketNameStore1, s3BucketNameStore2])) to remove
contents and avoid accumulation when QUEUE_BACKEND=localstack — apply the same
cleanup for the other bucket-creation block referenced around the second
occurrence (lines ~74-78) so both test setups call testAdmin.emptyBuckets in
their respective afterAll hooks.
| } catch (e) { | ||
| if (e instanceof NoSuchBucket) { | ||
| } |
There was a problem hiding this comment.
Non-NoSuchBucket errors are silently swallowed — same as packages/sns/test/utils/s3Utils.ts
Same inverted catch-logic: all errors that are not NoSuchBucket silently exit the catch without being rethrown, masking real failures (e.g., AccessDenied, throttling) and leaving buckets dirty.
🐛 Proposed fix
- } catch (e) {
- if (e instanceof NoSuchBucket) {
- }
- }
+ } catch (e) {
+ if (!(e instanceof NoSuchBucket)) {
+ throw e
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e) { | |
| if (e instanceof NoSuchBucket) { | |
| } | |
| } catch (e) { | |
| if (!(e instanceof NoSuchBucket)) { | |
| throw e | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sqs/test/utils/s3Utils.ts` around lines 25 - 27, In the catch block
that currently reads "catch (e) { if (e instanceof NoSuchBucket) { }", change
the logic so only NoSuchBucket is swallowed and all other errors are rethrown:
detect NoSuchBucket and ignore it, otherwise throw the caught error (e) so
AccessDenied/throttling/other failures are surfaced; update the catch in the
function surrounding the S3 bucket teardown (the try/catch handling bucket
deletion) to follow this behavior.
| async deleteQueues(...queueNames: string[]) { | ||
| if (this.server) { | ||
| for (const name of queueNames) { | ||
| this.server.deleteQueue(name, { region: this.region }) | ||
| } | ||
| return | ||
| } | ||
| for (const name of queueNames) { | ||
| await deleteQueue(this.sqsClient, name) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the test file mentioned in the comment
echo "=== Content of testAdmin.ts (lines 52-62) ==="
sed -n '52,62p' packages/sqs/test/utils/testAdmin.ts
echo -e "\n=== Full file context (lines 40-70) ==="
sed -n '40,70p' packages/sqs/test/utils/testAdmin.ts
echo -e "\n=== Check package.json for fauxqs dependency ==="
cat package.json | grep -A 2 -B 2 'fauxqs' || echo "Not found in root package.json"
echo -e "\n=== Check packages/sqs/package.json ==="
cat packages/sqs/package.json | grep -A 2 -B 2 'fauxqs' || echo "Not found in packages/sqs/package.json"Repository: kibertoad/message-queue-toolkit
Length of output: 1436
🏁 Script executed:
#!/bin/bash
# Look for fauxqs in node_modules
echo "=== Looking for fauxqs type definitions ==="
find node_modules/fauxqs -name "*.d.ts" -o -name "*.ts" 2>/dev/null | head -20
echo -e "\n=== Check if fauxqs exists and look for index.d.ts ==="
ls -la node_modules/fauxqs/ 2>/dev/null | head -20
echo -e "\n=== Search for emptyBucket in testAdmin.ts ==="
grep -n "emptyBucket" packages/sqs/test/utils/testAdmin.ts
echo -e "\n=== Full testAdmin.ts file to see all server method calls ==="
cat packages/sqs/test/utils/testAdmin.tsRepository: kibertoad/message-queue-toolkit
Length of output: 2578
🌐 Web query:
fauxqs npm library FauxqsServer createQueue deleteQueue createBucket emptyBucket async Promise
💡 Result:
I couldn’t find any npm package or GitHub project named fauxqs nor any API docs mentioning FauxqsServer, createQueue/deleteQueue, or createBucket/emptyBucket after searching those exact identifiers. If you paste a link to the repo/npm page (or the exact package name), I can map the methods and their Promise/async behavior precisely.
If your goal is “a fake/local AWS SQS server” for tests, a common package is fake-sqs, which exposes an SQSServer you await to start (it uses async/Promises for server startup via bootstrap()). [1]
If you’re implementing real SQS queue creation/deletion semantics (or mocking them), note AWS SQS requires waiting at least 60 seconds after deleting a queue before recreating one with the same name. [2]
Sources: [1] [2]
🏁 Script executed:
#!/bin/bash
# Check if fauxqs is a workspace package
echo "=== Looking for fauxqs in workspace ==="
find . -name "package.json" -type f -exec grep -l '"name".*fauxqs' {} \;
echo -e "\n=== Search for fauxqs directory ==="
find . -type d -name "fauxqs" 2>/dev/null | head -10
echo -e "\n=== Check node_modules .package.json for fauxqs ==="
cat node_modules/fauxqs/package.json 2>/dev/null | head -30
echo -e "\n=== Look for index.js or index.d.ts in fauxqs ==="
ls -la node_modules/fauxqs/ 2>/dev/null
echo -e "\n=== Try to find type definitions for FauxqsServer ==="
rg "FauxqsServer" --type ts --type tsx -A 5 -B 5 | head -100Repository: kibertoad/message-queue-toolkit
Length of output: 355
🏁 Script executed:
#!/bin/bash
# Search for how server methods are called
echo "=== Search for server.deleteQueue calls ==="
rg "server\.deleteQueue|server\.createQueue|server\.createBucket|server\.emptyBucket" -n
echo -e "\n=== Look for FauxqsServer type imports and definitions ==="
rg "FauxqsServer|from.*fauxqs" -n
echo -e "\n=== Search test files for await patterns with server methods ==="
rg "await.*server\.|server\.[a-zA-Z]+.*await" -n
echo -e "\n=== Check if createQueue returns a value (line 47) ==="
sed -n '45,50p' packages/sqs/test/utils/testAdmin.tsRepository: kibertoad/message-queue-toolkit
Length of output: 3635
🏁 Script executed:
#!/bin/bash
# Check fauxqsInstance files
echo "=== Content of packages/sqs/test/utils/fauxqsInstance.ts ==="
cat packages/sqs/test/utils/fauxqsInstance.ts
echo -e "\n=== How createQueue result is used in tests ==="
rg "createQueue" packages/sqs/test -A 2 -B 2 --type ts | head -50
echo -e "\n=== Check SNS version too ==="
cat packages/sns/test/utils/fauxqsInstance.tsRepository: kibertoad/message-queue-toolkit
Length of output: 5636
Add await or return when calling server methods to handle async operations.
The deleteQueue, createBucket, and emptyBucket server method calls are not awaited. While the async methods themselves will not throw, the underlying operations will execute fire-and-forget. Add await before each call or return before the loop/call to ensure test cleanup doesn't execute before operations complete:
deleteQueues: Addawaitbefore eachthis.server.deleteQueue()call orawait Promise.all()for all deletionscreateBucket: Addawaitbeforethis.server.createBucket()emptyBuckets: Addawaitfor eachthis.server.emptyBucket()call
Note: createQueue already returns the promise, so it's handled correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sqs/test/utils/testAdmin.ts` around lines 52 - 62, The server-side
calls in deleteQueues, createBucket, and emptyBuckets are invoked
fire-and-forget; update deleteQueues to await each this.server.deleteQueue(...)
(or collect and await Promise.all over queueNames), update createBucket to await
this.server.createBucket(...), and update emptyBuckets to await each
this.server.emptyBucket(...) (or await Promise.all) so the async server
operations complete before the test cleanup proceeds; refer to the deleteQueues,
createBucket, and emptyBuckets functions and their this.server.deleteQueue(),
this.server.createBucket(), this.server.emptyBucket() calls to apply the
changes.
| async createBucket(name: string) { | ||
| if (this.server) { | ||
| this.server.createBucket(name) | ||
| return | ||
| } | ||
| return await assertBucket(this.s3!, name) | ||
| } |
There was a problem hiding this comment.
Non-null assertion on this.s3! will throw at runtime if s3 was not provided.
When server is undefined and s3 was also not supplied (it's optional in the constructor), createBucket and emptyBuckets will crash with an opaque error. Consider adding an early guard:
🛡️ Suggested defensive check
async createBucket(name: string) {
if (this.server) {
this.server.createBucket(name)
return
}
+ if (!this.s3) {
+ throw new Error('S3 client is required when not using FauxqsServer')
+ }
- return await assertBucket(this.s3!, name)
+ return await assertBucket(this.s3, name)
}The same applies to emptyBuckets on Line 79.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async createBucket(name: string) { | |
| if (this.server) { | |
| this.server.createBucket(name) | |
| return | |
| } | |
| return await assertBucket(this.s3!, name) | |
| } | |
| async createBucket(name: string) { | |
| if (this.server) { | |
| this.server.createBucket(name) | |
| return | |
| } | |
| if (!this.s3) { | |
| throw new Error('S3 client is required when not using FauxqsServer') | |
| } | |
| return await assertBucket(this.s3, name) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sqs/test/utils/testAdmin.ts` around lines 64 - 70, createBucket (and
likewise emptyBuckets) currently uses a non-null assertion on this.s3 which will
throw if neither server nor s3 were provided; add an early guard in createBucket
(and emptyBuckets) that checks if this.server is falsy and this.s3 is falsy, and
in that case throw or return a clear, descriptive error (e.g., "S3 client not
available: provide server or s3") instead of using this.s3! and calling
assertBucket; reference the methods createBucket and emptyBuckets and the helper
assertBucket when adding the guard.
| } else { | ||
| const { createLocalhostHandler } = await import('fauxqs') | ||
| s3Config = { | ||
| endpoint: 'http://s3.localhost:4566', | ||
| region: 'eu-west-1', | ||
| credentials: { | ||
| accessKeyId: 'access', | ||
| secretAccessKey: 'secret', | ||
| }, | ||
| requestHandler: createLocalhostHandler(), | ||
| } |
There was a problem hiding this comment.
Top-level conditional await import('fauxqs') is valid ESM but couples module init to the dev dependency.
The dynamic import runs at module evaluation time (not lazily on first use). If fauxqs is ever missing from node_modules (e.g., a partial install), this will throw at import time rather than at the point of use, which makes the error message less actionable. This is a minor concern given the file lives exclusively in test utilities — just worth being aware of.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sqs/test/utils/testAwsConfig.ts` around lines 26 - 36, The top-level
await import('fauxqs') that assigns createLocalhostHandler into s3Config runs at
module evaluation and can fail if the dev dependency is absent; change this to a
lazy dynamic import by moving the import into a function that constructs/returns
the s3Config at call-time (e.g., add an async function like buildLocalS3Config
or getS3Config that does const { createLocalhostHandler } = await
import('fauxqs') and then returns the s3Config object with requestHandler:
createLocalhostHandler()); update callers to use that function so
createLocalhostHandler is only imported when needed and not during module
initialization, keeping the symbol names createLocalhostHandler and s3Config for
easy location.
| "test:coverage": { | ||
| "dependsOn": ["^test:coverage"] | ||
| }, | ||
| "test:coverage": {}, |
There was a problem hiding this comment.
Parallel test:coverage execution creates a port-4566 conflict for the fauxqs backend.
With "test:coverage": {}, TurboRepo will run test:coverage in all packages simultaneously (no dependsOn dependency ordering). Both packages/sqs and packages/sns call startFauxqs({ port: 4566, host: 'localstack' }) in their respective vitest.setup.ts/globalSetup files. When a developer runs turbo run test:coverage without QUEUE_BACKEND=localstack, both processes race to bind localstack:4566; the second one fails and its entire test suite crashes.
Two options to fix:
Option A — assign different ports per package (e.g. SQS on 4566, SNS on 4567) and update the matching client endpoints.
Option B — add a dependsOn guard so packages run sequentially when fauxqs is active:
🔧 Option B – sequential ordering
- "test:coverage": {},
+ "test:coverage": {
+ "dependsOn": ["^test:coverage"]
+ },Note: the fauxqs server is started in per-package vitest setup hooks; it lives only for the duration of that package's vitest process, so Option A (distinct ports per package) requires no shared state and is the cleaner long-term approach.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "test:coverage": {}, | |
| "test:coverage": { | |
| "dependsOn": ["^test:coverage"] | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@turbo.json` at line 12, Parallel test execution causes both packages to call
startFauxqs({ port: 4566, host: 'localstack' }) and race to bind the same port;
fix by either (A) assigning per-package ports and updating client endpoints
(e.g., change startFauxqs call in packages/sqs and packages/sns to different
port numbers and update any client initialization that relies on
QUEUE_BACKEND/localstack endpoint) or (B) make turbo run "test:coverage"
sequential when fauxqs is used by adding a dependsOn guard in turbo.json so
packages run one after another when QUEUE_BACKEND=localstack; locate and update
the startFauxqs calls in your vitest.setup.ts/globalSetup and the turbo.json
"test:coverage" entry accordingly.
| ["packages/sqs"]="@message-queue-toolkit/sqs" | ||
| ) | ||
|
|
||
| AWS_PACKAGE_PATHS=("packages/sqs" "packages/sns") |
There was a problem hiding this comment.
Should we also add here s3-payload-store
There was a problem hiding this comment.
will do it in a separate PR, currently it only uses fauxqs, but probably should use dual setup as well
2 participants
Summary by CodeRabbit
New Features
Chores