chore: replace LocalStack with Moto for AWS test infrastructure

[holytshirt]

Closes #4068

Summary

Replace LocalStack with Moto as the default AWS mock for local and CI runs, after LocalStack Community Edition dropped its free tier in February 2026. Moto is open-source, covers the AWS services Brighter exercises (SQS, SNS, S3, STS), and runs as a single Docker image.

Migration

• Rename docker-compose-localstack.yaml → docker-compose-aws.yaml (single motoserver/moto service on :4566).
• Rename env var LOCALSTACK_SERVICE_URL → AWS_SERVICE_URL across test helpers, CI, samples, docs, and commented-out override blocks.
• Rename the CI job localstack-ci → aws-mock-ci.
• Pin the Moto image to motoserver/moto:5.1.22 in both compose and CI so an unannounced upstream release can't silently break CI.
• Add a curl -sf http://localhost:4566/moto-api/ health check to the Moto service in both compose and CI so test steps wait for Moto to accept connections.
• Drop the LocalStack-specific S3 endpoint rewrite (http://s3.{authority}) from GatewayFactory.
• Drop [Trait("Fragile", "CI")] (and the inverted [Trait("CI", "Fragile")]) from AWS test classes — these were workarounds for LocalStack flake. After this PR no AWS test carries that trait, so the redundant --filter "Fragile!=CI" is removed from the aws-mock-ci step.
• Ensure every AWS test class carries [Trait("Category", "AWS")].
• Replace the hardcoded http://localhost:4566/ URL in samples/TaskQueue/AWSTaskQueue/GreetingsSender/Program.cs with Environment.GetEnvironmentVariable("AWS_SERVICE_URL"), matching every other AWS sample.
• Tidy two stale double-commented localhost:4566 / AWS_SERVICE_URL blocks in the AWS scheduler samples (the live code intentionally hardcodes real AWS because EventBridge Scheduler isn't implemented by Moto — a one-line comment explains the pin).

Latent test bugs found and fixed

While re-investigating the previously-skipped redrive tests, three pump/mapper/handler mismatches surfaced — these were the real cause of the "task scheduler issues" / "DLQ flaky" skip reasons, not LocalStack:

• Sync mapper on async pump (4 SNS Proactor + 2 SQS Fifo Proactor redrive tests, v3 + v4): registered SimpleMessageMapperFactory + sync MyDeferredCommandMessageMapper on a Proactor. The mapper never dispatched, so the deferred message was never redriven. Switched to SimpleMessageMapperFactoryAsync + MyDeferredCommandMessageMapperAsync + RegisterAsync.
• Async handler on sync pump (2 SQS Fifo Reactor redrive tests, v3 + v4): the inverse mismatch. Switched to QuickHandlerFactory + MyDeferredCommandHandler + Register.
• Mixed Standard/Fifo queue+topic (2 SNS Fifo Proactor redrive tests, v3 + v4): topicAttributes set Type = SqsType.Fifo but queueAttributes did not. Recently-added SqsSubscription validation on master correctly rejects this. Added type: SqsType.Fifo to the queue attributes.

All eight previously-skipped tests now run green against Moto.

Approach to running against real AWS

No trait filter required — if AWS_SERVICE_URL is unset and a local AWS profile is configured, the helpers use real AWS credentials.

Two scoped runtime skips when running against Moto (skipped via Skip.If(AWS_SERVICE_URL != null, …), so they still run against real AWS):

1. SkippableFact — one SQS-only producer-tag verification test that fails against Moto due to a .NET SDK / Moto ListQueueTags interop issue.
2. SkippableTheory — eight V4 fairQueue: True theory cases. SQS fair-queue support landed in Moto >= 5.1.23; the latest stable is 5.1.22. Each guard carries a // TODO: remove once Moto pin in #4096 is bumped to 5.1.23+ comment.

Out of scope: AWSScheduler tests still target real AWS — EventBridge Scheduler is not implemented by Moto.

Drive-by polish

These are all pre-existing issues that surfaced because this PR touches the same files; cheap to fix in passing.

• Rename CredentialsChain.GetBucketAddressTemple → GetBucketAddressTemplate in v3 + v4 (typo, 5 callers each).
• Rename When_throwing_defer_action_respect_redrive_async → When_throwing_defer_action_respect_redrive in the v3 + v4 SQS Fifo Reactor tests — the methods are now sync (void), so the _async suffix was misleading.
• Delete PR-DESCRIPTION.md from the repo root (a tooling artifact that was already on master from Fix CI acceptance test reliability: Add monitoring, increase timeouts and health check retries, add Kafka readiness verification #3994; this PR's body covers what it documented).

Test plan

Against pinned motoserver/moto:5.1.22, full suites with no filters:

• AWS v3: 136 passed, 1 skipped, 0 failed (skip = SQS producer tag test)
• AWS v4: 130 passed, 8 skipped, 0 failed (skips = fairQueue: True theory cases)
• CI aws-mock-ci job passes against Moto in GitHub Actions
• CI aws-ci job (real AWS) — currently wedged on Amazon.SimpleNotificationService.Model.TopicLimitExceededException. This is a CI account state issue, not a regression from this PR — master is failing the same way (e.g. run 24963243390). The CI account has accumulated SNS FIFO topics from prior runs and needs a one-off cleanup before the real-AWS smoke run can be confirmed.

Follow-up

• Bump Moto image to 5.1.23+ when released to enable SQS fair-queue tests #4096 — bump Moto pin to 5.1.23+ when it cuts a stable release, so the eight V4 fair-queue cases come off the SkippableTheory guard.
• AWS CI account: clean up leftover SNS FIFO topics so aws-ci can run again (separate to this PR).

🤖 Generated with Claude Code

[claude]

Code Review: Replace LocalStack with Moto for AWS test infrastructure

Overview

Clean, well-scoped migration driven by LocalStack Community Edition dropping its free tier in February 2026. The change is mechanically straightforward — env var rename, docker image swap, S3 URL workaround removal — and the PR description is admirably thorough about what was re-verified, un-skipped, and left out of scope.

---

Positive observations

• Correct motivation and timing. Switching to Moto now rather than reacting to a broken CI is the right call.
• Fragile=CI trait removal is justified. Those traits were workarounds for LocalStack flakiness, not inherent test instability. Removing them cleans up the filter logic.
• S3 URL special-casing removed (GatewayFactory.cs, CreateFactory/CreateS3Connection). The old http://s3.{authority} subdomain rewrite was a LocalStack-specific quirk; deleting it simplifies the code correctly.
• SkippableFact usage is well-targeted. The single SQS tag verification test that fails against Moto skips cleanly when AWS_SERVICE_URL is set, and still runs against real AWS. The skip condition (!string.IsNullOrWhiteSpace(AWS_SERVICE_URL)) is correct.
• [Trait("Category", "AWS")] gap-filling across V4 Transformer and SQS test classes improves consistency and ensures those tests are caught by the CI filter.
• Documentation and sample updates are complete — CONTRIBUTING.md, README, both sample Program.cs files, and the QuartzTaskQueue README are all updated.

---

Issues and suggestions

1. Pin the Moto image tag (medium priority)

Both docker-compose-aws.yaml and the CI job use motoserver/moto:latest. An unannounced Moto release could break CI without any code change.

# docker-compose-aws.yaml and ci.yml
image: motoserver/moto:5.1.4   # pin to a known-good version

Add a comment referencing the release date so it's easy to update intentionally.

2. No health check for the Moto service in CI (medium priority)

The PR description says health checks were added for other services, but the new moto service block in ci.yml has no options: health check:

services:
  moto:
    image: motoserver/moto:latest
    ports:
      - 4566:4566
    env:
      MOTO_PORT: "4566"
    # Missing:
    options: >-
      --health-cmd "curl -sf http://localhost:4566/moto-api/reset || exit 1"
      --health-interval 5s
      --health-timeout 3s
      --health-retries 10

Without this, a test step could start before Moto is accepting connections, causing flaky failures.

3. docker-compose-aws.yaml also missing a health check (low priority)

For consistency with other compose files in the repo and to give developers a reliable docker compose up --wait experience, add the same health check locally.

4. Un-skipped async redrive test — validate the skip reason was accurate (low priority, for discussion)

In When_throwing_defer_action_respect_redrive_async.cs (V4), the [Fact(Skip = "DLQ is fragile on async tests")] is removed and the test now runs unconditionally. The PR says the test passes against Moto (v4: 134 passed, 4 skipped, 0 failed). That's great — just confirm the original skip reason ("DLQ is fragile on async") was entirely due to LocalStack timing and not a latent race in the test body, so it doesn't resurface under load.

5. samples/GreetingsSender hardcoded URL remains (low priority, out of scope)

samples/TaskQueue/AWSTaskQueue/GreetingsSender/Program.cs line 56 still has a hardcoded "http://localhost:4566/" instead of reading AWS_SERVICE_URL. This predates this PR and is a pre-existing issue, but the env-var rename here is a good opportunity to fix it. Not a blocker.

---

Summary

This is a solid infrastructure maintenance PR. The two medium-priority items (pinning the image tag and adding a health check to the Moto service in CI) are the main things worth addressing before merging to avoid future flakiness. Everything else is minor or informational.

🤖 Generated with Claude Code

[iancooper]

Nice fix

[iancooper]

Not sure if this merged in the global inbox fixes, but failing tests seem to relate to those.

[holytshirt]

I'll pull in the latest changes

[claude]

Code Review — chore: replace LocalStack with Moto for AWS test infrastructure

Overall this is a well-executed migration. The motivation is clear (LocalStack CE dropped its free tier), the rename from LOCALSTACK_SERVICE_URL → AWS_SERVICE_URL is consistent across CI, test helpers, samples, and docs, and the removal of the LocalStack-specific S3 subdomain rewrite is correct. The SkippableFact approach for the one test that is genuinely incompatible with Moto is a clean, targeted solution.

A few issues worth addressing before merging:

---

Medium — Moto image not pinned

Both docker-compose-aws.yaml and the CI workflow use motoserver/moto:latest. This is the same fragility vector that motivated moving away from LocalStack — an unannounced breaking release will silently break CI.

Suggestion: Pin to a specific digest or semver tag, e.g. motoserver/moto:5.1.0, and add a comment noting when it was last bumped. The version used during your test run (v3/v4 results above) would be the natural starting point.

---

Medium — No health check on the Moto service in CI

The services.moto block in ci.yml has no options: entry with a health check, so test jobs can start before Moto is ready to accept connections. LocalStack had the same gap, but it's worth fixing now.

Suggestion: Add a health check to the service block:

services:
  moto:
    image: motoserver/moto:5.1.0   # pin the tag too
    ports:
      - 4566:4566
    env:
      MOTO_PORT: "4566"
    options: >-
      --health-cmd "curl -sf http://localhost:4566/moto-api/state || exit 1"
      --health-interval 5s
      --health-timeout 3s
      --health-retries 10
      --health-start-period 10s

/moto-api/state is a Moto-specific endpoint that returns 200 once the server is ready.

---

Low — GreetingsSender/Program.cs still has a hardcoded URL

Every other sample correctly reads AWS_SERVICE_URL from the environment, but GreetingsSender still has:

var serviceURL = "http://localhost:4566/"; // Environment.GetEnvironmentVariable("AWS_SERVICE_URL")

This is inconsistent with the rest of the migration and will silently break if someone points the environment at a non-default address. The fix is a one-liner — uncomment the GetEnvironmentVariable call (and drop the literal).

---

Low — docker-compose-aws.yaml also lacks a health check

Local developer experience will benefit from a health check here too, so docker compose up --wait works reliably:

healthcheck:
  test: ["CMD", "curl", "-sf", "http://localhost:4566/moto-api/state"]
  interval: 5s
  timeout: 3s
  retries: 10
  start_period: 10s

---

Nit — Skip logic direction worth a comment

Skip.If(!string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable("AWS_SERVICE_URL")),
    "SQS tag verification fails against Moto; runs against real AWS.");

The logic is correct (skip when AWS_SERVICE_URL is set → Moto context; run when it is absent → real AWS), but the double-negative is easy to misread. A short inline comment (// skip when running against Moto) would help future readers.

---

Summary

Issue	Severity	Status
Moto image not pinned (latest)	Medium	Open
No Moto health check in CI	Medium	Open
GreetingsSender hardcoded URL	Low	Open
docker-compose-aws.yaml no health check	Low	Open
Skip logic comment	Nit	Open

None of these are blockers, but the two Medium items (pinning + health check) are worth fixing before the draft is marked ready — they're exactly the class of issue that causes intermittent CI failures and is hard to diagnose later.

[claude]

Code Review: Replace LocalStack with Moto for AWS test infrastructure

Overall assessment: This is a well-motivated and cleanly executed infrastructure migration. The rationale (LocalStack Community Edition dropping its free tier in February 2026) is sound, and Moto is a solid open-source alternative for the AWS services Brighter uses. The diff is large in file count but mechanically consistent. Items below range from genuine bugs/gaps to minor polish suggestions.

---

1. Potential Bug — SNS Tagged Resources Test May Also Need [SkippableFact]

File: tests/Paramore.Brighter.AWS.Tests/MessagingGateway/Sns/Standard/Proactor/When_posting_a_message_resources_are_tagged_async.cs

The PR correctly adds [SkippableFact] + Skip.If(AWS_SERVICE_URL != null, …) to the SQS-only tagged-resources variant. However, the SNS variant also tests SQS queue tags (it verifies both SNS topic tags and SQS queue tags) and still uses a plain [Fact]. If the underlying Moto .NET SDK interop issue applies to ListQueueTags in general — not just the SQS-only test — this test will silently fail against Moto. Please confirm whether this SNS-variant test passes against Moto; if not, apply the same [SkippableFact] treatment.

---

2. Minor — Misleading Method Name: GetDLQCountAsync Is Synchronous

File: tests/Paramore.Brighter.AWS.Tests/MessagingGateway/Sqs/Fifo/Reactor/When_throwing_defer_action_respect_redrive.cs

The method public int GetDLQCountAsync(string queueName) is synchronous internally (uses .GetAwaiter().GetResult()) but carries the Async suffix. This was pre-existing but this PR un-skips the test and makes it a live test again, making the misleading name more visible. Consider renaming to GetDLQCount.

---

3. Stale Commented-Out Code in AWSScheduler GatewayFactory Files

Files:

• tests/Paramore.Brighter.AWSScheduler.Tests/Helpers/GatewayFactory.cs
• tests/Paramore.Brighter.AWSScheduler.V4.Tests/Helpers/GatewayFactory.cs

Both files contain a dead commented-out block still referencing LOCALSTACK_SERVICE_URL. The PR migrates all active references to AWS_SERVICE_URL, but these two scheduler test helpers are left with the old variable name in comments. Not functional, but inconsistent — should be updated to AWS_SERVICE_URL or removed.

---

4. Moto Pinned to a Version Known to Be Missing a Feature

Files: docker-compose-aws.yaml, .github/workflows/ci.yml

The image is pinned to motoserver/moto:5.1.22. The skip messages in V4 tests explicitly state:

"SQS fair queues require Moto >= 5.1.23; pinned image is 5.1.22. Runs against real AWS."

This means 8 test cases (4 [SkippableTheory] across Reactor/Proactor × 2 scenarios) are permanently skipped in CI with no tracking item. Please either:

• Bump to motoserver/moto:5.1.23+ now if that version is available and tested, or
• Open a GitHub issue tracking the version bump as follow-up work

---

5. Health Check Portability — python vs python3

Files: .github/workflows/ci.yml, docker-compose-aws.yaml

The health check uses:

python -c 'import urllib.request; ...'

Many modern Python Docker images only provide python3, not python. If the moto image doesn't symlink python → python3, the health check will fail silently. The safer alternative:

python3 -c 'import urllib.request; urllib.request.urlopen("http://localhost:4566/moto-api/").read()'

Or even simpler, use curl -f http://localhost:4566/moto-api/ which avoids the Python dependency entirely.

---

6. Nit — --filter "Fragile!=CI" in aws-mock-ci Is Now a No-op

File: .github/workflows/ci.yml

The PR correctly removes [Trait("Fragile", "CI")] from all AWS test classes. After this PR, the filter Fragile!=CI matches everything (no AWS tests have that trait anymore), so the filter is now effectively a no-op. Fine as-is, but you may want to drop it for the aws-mock-ci step for clarity, or document why it's retained as a generic guard.

---

7. PR-DESCRIPTION.md Should Not Be Committed to the Repo

File: PR-DESCRIPTION.md

This file appears to be a tooling artifact and is tracked in the repository. Committing PR metadata to the main branch of a library is unusual and will accumulate over time. This should be either removed or added to .gitignore.

---

Positive Notes

• S3 endpoint simplification is correct. Removing the LocalStack-specific S3 URL construction (s3.{uri.Authority}) in favor of a single Moto endpoint is clean and appropriate.
• Proactor async mapper fix is a genuine bug fix. Switching from SimpleMessageMapperFactory + sync mapper to SimpleMessageMapperFactoryAsync + async mapper for Proactor pumps correctly addresses the "task scheduler issues" that caused those tests to be skipped.
• version: '3' correctly omitted from the new docker-compose-aws.yaml (it's deprecated in recent Docker Compose versions).

---

Reviewed by Claude Code

[claude]

Code Review — PR #4069: Replace LocalStack with Moto

Overall: This is a well-motivated and well-executed infrastructure migration. The switch from LocalStack (dropped free tier) to Moto is clearly necessary, and the approach is sound. A few issues worth addressing before merge.

---

What this does well

• Health checks added everywhere. Both docker-compose-aws.yaml and the CI service definition include proper curl-based health checks. The old LocalStack compose had no health check at all — this is a net improvement.
• SkippableFact/SkippableTheory pattern is the right call. Rather than permanently skipping Moto-incompatible tests ([Fact(Skip=...)]), the conditional skip keeps them running against real AWS when AWS_SERVICE_URL is unset. This maintains test value without hiding failures.
• Bug fixes bundled in. Several Proactor tests were incorrectly wired to sync mappers/handlers (MyDeferredCommandMessageMapper instead of MyDeferredCommandMessageMapperAsync). Reactor tests had the opposite problem. These are real correctness fixes, not just trait removal.
• SqsType.Fifo added to FIFO redrive test constructors — previously missing, which would have caused the DLQ assertions to silently target a standard queue.
• [Trait("Category", "AWS")] consistently applied — closing the gap on test classes that were missing the trait (e.g., S3LuggageUploadTests, SqsSubscriptionDlqRoutingKeyTests).
• Simplified S3 URL handling — the special-cased http://s3.{authority} rewrite for AmazonS3Config is gone; Moto handles a single service URL for all services cleanly.

---

Issues

1. Moto pinned to 5.1.22 but fair-queue tests need 5.1.23

In multiple V4 test files (e.g. When_a_message_consumer_reads_multiple_messages_async.cs, When_posting_a_message_via_the_messaging_gateway.cs), the skip reason reads:

"SQS fair queues require Moto >= 5.1.23; pinned image is 5.1.22. Runs against real AWS."

If 5.1.23 is available and doesn't break other tests, bumping the pinned image from 5.1.22 → 5.1.23 would enable those SkippableTheory cases against Moto without needing a real AWS account. Since you've already verified 5.1.22 passes 132/134 tests, the incremental risk of a patch bump seems low. Worth resolving before merge to avoid shipping CI tests that are known-skipped due to a version lag you could fix.

2. Missing Xunit.SkippableFact package reference in Paramore.Brighter.AWS.V4.Tests.csproj

The diff adds the package reference only to Paramore.Brighter.AWS.Tests.csproj, but SkippableTheory and Skip.If(...) are also used in several V4 test files. The test plan reports V4 as green, so either V4 already had this package before this PR (in which case no action needed) or this was caught locally but the .csproj change is missing from the diff.

Please confirm Paramore.Brighter.AWS.V4.Tests.csproj already references Xunit.SkippableFact. If not, add the <PackageReference Include="Xunit.SkippableFact" /> entry there too.

3. PR-DESCRIPTION.md is a tracked file in the repository

diff --git a/PR-DESCRIPTION.md b/PR-DESCRIPTION.md

This file is being maintained as a committed file. The PR description on GitHub is the right home for this content. Committed PR-DESCRIPTION.md files tend to rot (they don't update when the branch diverges) and add noise to git log. Consider removing it or moving it to .github/ if it's truly needed for tooling.

---

Minor observations

• GetDLQCountAsync → GetDLQCount rename in the Reactor tests is correct (the method body uses .GetAwaiter().GetResult() — it was never async). Good cleanup.
• The [Trait("CI", "Fragile")] (reversed arguments) on AwsValidateInfrastructureByArnTests was never actually filtering the test since the filter was "Fragile!=CI". Removing it is right.
• The --filter "Fragile!=CI" removed from the CI dotnet test commands: this is correct since the fragile trait is no longer being applied to any test in this project.

---

Summary

The migration itself is clean and the testing strategy (SkippableFact for Moto gaps, full run otherwise) is solid. The two actionable items before merging:

1. Bump Moto to 5.1.23 to un-skip the fair-queue tests (or explicitly document why staying on 5.1.22 is intentional).
2. Confirm Xunit.SkippableFact is referenced in the V4 test project.

The bug fixes to Proactor/Reactor handler wiring and the SqsType.Fifo addition are good catches worth highlighting in the commit message.

🤖 Reviewed with Claude Code

[claude]

Code Review: Replace LocalStack with Moto for AWS test infrastructure

Overall this is a well-executed migration with a clear motivation (LocalStack dropped its free tier). The PR description is thorough and the approach is sound. A few items worth discussing before merge.

---

✅ What's done well

Infrastructure migration

• Moto image pinned to motoserver/moto:5.1.22 — avoids the silent-breakage risk that the old localstack:latest tag had.
• Health check using /moto-api/ is the correct Moto-specific endpoint and has a sensible retry policy.
• LOCALSTACK_SERVICE_URL → AWS_SERVICE_URL is a better name (tool-agnostic, easier to document).
• Removing the LocalStack-specific S3 virtual-host URL rewrite (http://s3.{authority}) is correct — Moto uses a flat endpoint model and doesn't require it.
• Dropping --filter "Fragile!=CI" is the right call once the underlying flakiness is resolved.

Latent bug fixes
The three categories of pump/mapper/handler mismatches are genuine bugs independently of the LocalStack→Moto switch — catching them here is a bonus. The fixes are correct:

• Sync mapper on async pump → SimpleMessageMapperFactoryAsync + RegisterAsync ✓
• Async handler on sync pump → QuickHandlerFactory + Register ✓
• Mixed Standard/Fifo queue+topic → type: SqsType.Fifo on queueAttributes ✓

Moto-skip strategy
Skip.If(AWS_SERVICE_URL != null, ...) correctly gates on the mock environment, so the same tests still exercise real AWS when AWS_SERVICE_URL is unset. The fairQueue skip only suppresses fairQueue=true theory cases, not the false path — good precision.

---

⚠️ Issues

1. PR-DESCRIPTION.md is committed to the repository

The diff includes PR-DESCRIPTION.md as a versioned file being modified (not a PR template). This file belongs in the PR body, not in main. It should be deleted from the branch before merge. If it's part of the .claude/AI workflow infrastructure, it should be in .gitignore or a dedicated non-shipped directory.

2. Commented-out dead code in scheduler samples

samples/Scheduler/AwsTaskQueue/GreetingsPumper/Program.cs and GreetingsReceiverConsole/Program.cs still contain:

// var serviceURL = "http://localhost:4566/"; // Environment.GetEnvironmentVariable("AWS_SERVICE_URL");

The comment-within-comment pattern is confusing. Either uncomment and wire up AWS_SERVICE_URL properly (as was done in GreetingsSender), or delete the dead block entirely. The inconsistency between the three sample programs in the same directory is jarring.

3. motoserver/moto image may not ship curl

The GitHub Actions health check option is:

--health-cmd "curl -sf http://localhost:4566/moto-api/"

This runs inside the service container. Moto's Python-based image may not include curl. If CI fails at service startup, this is why. A safer alternative is python -c "import urllib.request; urllib.request.urlopen('http://localhost:4566/moto-api/')" or switching to a wget-based check. (If tests are already passing in CI this is a non-issue, but worth verifying.)

4. Misleading test method name (minor)

In Sqs/Fifo/Reactor/When_throwing_defer_action_respect_redrive.cs, the public test method is still named:

public void When_throwing_defer_action_respect_redrive_async()

The _async suffix on a synchronous void test is left over from the pre-fix version. Worth cleaning up since this is a pre-existing naming issue that the refactor surfaced.

5. GetBucketAddressTemple() typo (pre-existing, minor)

"Temple" → "Template" in CredentialsChain. Not introduced here but touched in this diff, so easy to fix in passing.

---

💬 Questions / suggestions

• Smoke test against real AWS — the test plan has this unchecked. Is there a plan to confirm this before the PR graduates from DRAFT, or is CI green sufficient for merging?
• Xunit.SkippableFact version — 1.5.23 is the latest stable as of this writing. Is it pinned to a specific test project, or added globally to Directory.Packages.props? If it's globally available, ensure no test project accidentally picks it up without the CI skip logic.
• Tracking #4096 — the fairQueue pin bump is well scoped. Consider adding a TODO comment near the SkippableTheory guards referencing #4096 so future readers know the skip is intentional and tracked, rather than forgotten.

---

Summary

The core migration is correct and the bug fixes are valuable. The main blocker before merge is removing PR-DESCRIPTION.md from the tracked files. The scheduler sample dead-code cleanup and the curl availability question are worth addressing while the PR is open.

[claude]

Code Review: chore: replace LocalStack with Moto for AWS test infrastructure

Overview

This PR migrates AWS test infrastructure from LocalStack (which dropped its free tier in February 2026) to Moto, an open-source AWS mock. The changes span CI configuration, Docker Compose, test helpers, samples, and documentation — and along the way surface and fix several latent bugs in the test suite that were previously hidden under blanket Skip annotations.

---

Strengths

Infrastructure migration is clean and well-considered:

• Pinning motoserver/moto:5.1.22 in both docker-compose-aws.yaml and the CI job prevents surprise upstream breakage, and the explicit --health-cmd ensures tests don't start before Moto is ready. Both are good practices absent from the old LocalStack setup.
• Removing the LocalStack-specific S3 URL rewrite (http://s3.{authority}) in GatewayFactory.cs is a welcome simplification — Moto's uniform endpoint design doesn't require path-style workarounds, so the conditional AmazonS3Config branch was pure debt.
• The AWS_SERVICE_URL rename is a sensible deprovincialisation: the name was always generic ("a mock endpoint"), not LocalStack-specific.

Genuine bug fixes found during the migration:
The three categories of latent test bugs — sync mapper on async pump, async handler on sync pump, and Standard/Fifo type mismatch in SqsAttributes — are real correctness bugs, not flakiness. Unskipping eight previously-skipped DLQ redrive tests after fixing the underlying mismatches is the right outcome. The diagnosis and fix are accurate.

SkippableFact / SkippableTheory usage is appropriate:
The two scoped skips (SQS producer tag interop and V4 fair-queue tests) carry clear reasoning and are gated on the presence of AWS_SERVICE_URL, so they still exercise real AWS. The TODO comments tied to issue #4096 make the removal condition explicit rather than leaving future maintainers to guess.

Typo fix (GetBucketAddressTemple → GetBucketAddressTemplate) touches 10 callsites across v3 and v4 consistently — good hygiene.

---

Issues and Suggestions

1. Two tests may not need [Trait("Category", "AWS")]

SqsMessageConsumerFactoryDlqTests (When_creating_sqs_consumer_with_dlq_subscription_should_pass_routing_keys.cs) and SqsSubscriptionDlqRoutingKeyTests (When_creating_sqs_subscription_with_dlq_routing_keys_should_expose_properties.cs) appear to be unit tests verifying subscription/factory configuration in memory — they don't send or receive messages. Adding [Trait("Category", "AWS")] will pull them into the aws-mock-ci run and require Moto to be running for no reason.

Similarly, When_creating_sns_attributes_with_tags.cs (When_creating_sns_attributes_with_tags) reads like a pure value-object test.

Suggest verifying whether these three test classes make any network calls. If not, the [Trait("Category", "AWS")] additions should be reverted. Including them costs nothing when Moto is already up, but it couples the test categorisation to infrastructure it doesn't actually need.

2. GetBucketAddressTemplate returns a virtual-hosted style URL that may be sensitive to Moto configuration

The returned template http://{BucketName}.s3.{BucketRegion}.{uri.Authority} relies on virtual-hosted style routing. The old LocalStack workaround (http://s3.{authority}) was different (and arguably also wrong). Since the S3 tests pass against Moto 5.1.22, this is working in practice — but it would be useful to add a one-line comment explaining why this format is used, so the next person touching it understands it's Moto-specific path addressing rather than standard AWS virtual hosting.

3. CI: aws-ci (real AWS) is broken independently of this PR

The PR description notes the real-AWS job is wedged on TopicLimitExceededException from accumulated SNS FIFO topics. While this is called out as a pre-existing issue, it means the full real-AWS smoke run can't be confirmed as part of merging. Consider whether a prerequisite cleanup task should gate this merge, or whether the existing aws-mock-ci coverage is sufficient for now (the latter is defensible given the account state is unrelated to the change).

4. The docker-compose-aws.yaml health check uses localhost inside the container health-check command

test: ["CMD", "curl", "-sf", "http://localhost:4566/moto-api/"]

This is correct because Docker executes health check commands inside the container, so localhost resolves to the container itself. No change needed — just confirming this is intentional and correct.

---

Summary

The migration is well-executed, the latent test fixes are genuine improvements, and the scoped skips are better than the previous blanket Skip = "..." annotations. The main thing worth double-checking before merge is whether the three newly-categorised tests (SqsMessageConsumerFactoryDlqTests, SqsSubscriptionDlqRoutingKeyTests, When_creating_sns_attributes_with_tags) actually require AWS infrastructure. Everything else looks good.

🤖 Generated with Claude Code

[claude]

Code Review: Replace LocalStack with Moto for AWS test infrastructure

This is a well-executed infrastructure migration. The PR description is thorough and honest about the scope, known skips, and follow-up work. Below is my assessment.

---

Overview

Replaces LocalStack with Moto as the AWS mock for local and CI runs, driven by LocalStack dropping its free tier. Also fixes eight previously-skipped tests whose root causes were mapper/handler mismatches — not LocalStack flake.

---

What's Good

Clean, complete migration

• Env var rename (LOCALSTACK_SERVICE_URL → AWS_SERVICE_URL) applied consistently across CI, helpers, samples, and docs.
• The LocalStack-specific S3 subdomain rewrite (http://s3.{authority}) is correctly dropped — Moto doesn't need it since it uses a single endpoint for all services.
• CI health check (/moto-api/) is appropriate for detecting Moto readiness (unlike a simple TCP port check).
• Pinning motoserver/moto:5.1.22 prevents silent upstream breakage — good CI hygiene.

Latent bug fixes are properly scoped and explained

• Sync mapper on async pump → switched to SimpleMessageMapperFactoryAsync + RegisterAsync. Correct.
• Async handler on sync pump → switched to QuickHandlerFactory + sync MyDeferredCommandHandler. Correct.
• Mixed Standard/Fifo queue+topic → added type: SqsType.Fifo to queue attributes. Correct.
• All eight previously-skipped tests are now [Fact] / [Fact] without a Skip reason.

Typo fix

• GetBucketAddressTemple → GetBucketAddressTemplate — appreciated.

Moto-incompatible test handling

• SkippableFact / SkippableTheory with Skip.If(AWS_SERVICE_URL != null, …) is the right pattern: tests still run against real AWS, skipped only against Moto, and each guard has a // TODO: remove once Moto pin in #4096 is bumped comment. Clean and traceable.

---

Observations / Minor Issues

1. GetBucketAddressTemplate() virtual-hosted URL format

return $"http://{{BucketName}}.s3.{{BucketRegion}}.{uri.Authority}";

This constructs a virtual-hosted-style URL (http://{bucket}.s3.{region}.localhost:4566). Moto's S3 supports virtual-hosted style, but only if DNS resolution resolves the subdomain to Moto's host — which typically doesn't work in Docker or CI without extra DNS configuration. Moto more reliably handles path-style URLs (http://localhost:4566/{bucket}). Since this method's callers use it as a bucket address template (not as the ServiceURL), it likely works correctly if no S3 endpoint override is in play. Worth validating explicitly, or adding a note in the method about this assumption.

2. GreetingsSender/Program.cs — region logic

var serviceURL = Environment.GetEnvironmentVariable("AWS_SERVICE_URL");
var region = string.IsNullOrWhiteSpace(serviceURL) ? RegionEndpoint.EUWest1 : RegionEndpoint.USEast1;

This is pre-existing behaviour, but now that the hardcoded http://localhost:4566/ is replaced with the env var read, the region selection is implicitly tied to whether you're running against a mock. Worth a comment or, better, a separate AWS_REGION env var fallback — but this is a sample so it's not a blocker.

3. docker-compose-aws.yaml missing version: key

The old docker-compose-localstack.yaml had version: '3'. The new file omits it, which is correct for modern Docker Compose (v2 schema deprecates that key). Just noting it's intentional, not an omission.

4. Task.Delay(5000).GetAwaiter().GetResult() in sync redrive test

This is pre-existing; the PR preserves it correctly for the DLQ settling wait. Not a regression, but worth flagging as tech debt — a poll-with-timeout would be more reliable and faster on fast machines.

5. Env var name: AWS_SERVICE_URL vs AWS_ENDPOINT_URL

The AWS SDK for .NET (v3.7.300+) has added support for AWS_ENDPOINT_URL as a standard env var for endpoint override. Using AWS_SERVICE_URL works fine today, but if the SDK ever picks up AWS_ENDPOINT_URL natively, the helper wiring in GatewayFactory could be simplified. Worth tracking as a follow-up, not a blocker.

6. CI timeout still 8 minutes

The aws-mock-ci job keeps the 8-minute timeout from the LocalStack job. Given that Moto typically starts faster than LocalStack and the test plan shows 136+130 tests passing, this should be adequate. Just worth monitoring on the first few CI runs.

---

Verdict

✅ Approve. The migration is thorough, the latent bug root causes are correctly diagnosed and fixed, the Moto-incompatible skips are well-guarded and documented, and the PR description gives future maintainers everything they need. The observations above are minor and none are blockers.

The one thing to validate before merging: confirm GetBucketAddressTemplate()'s virtual-hosted URL format works with Moto in CI (or at minimum ensure no tests rely on it against Moto without verifying DNS resolution).

🤖 Generated with Claude Code

[codescene-delta-analysis]

No application code in the PR — skipped Code Health checks.

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[claude]

Code Review: Replace LocalStack with Moto (#4069)

Summary: Well-executed infrastructure migration with a clean scope, good engineering practices, and a bonus set of real bug fixes. The PR description is thorough and honest about what's known to still be broken (real-AWS CI account state, fairQueue Moto version gap). Overall this is ready to merge with a few minor observations.

---

✅ Strengths

• Image pinned to motoserver/moto:5.1.22 — avoids silent breakage from upstream releases. The TODO comments pointing to Bump Moto image to 5.1.23+ when released to enable SQS fair-queue tests #4096 for bumping the pin are exactly the right pattern.
• Health check added — curl -sf http://localhost:4566/moto-api/ with retries ensures the container is accepting connections before tests run. Much better than a blind sleep.
• Bug fixes are genuine — the sync-mapper-on-async-pump and async-handler-on-sync-pump mismatches are real bugs, not just flakiness. Fixing them here and unblocking the previously-skipped tests is the right call.
• SkippableFact/SkippableTheory with clear conditions — the Skip.If(!string.IsNullOrWhiteSpace(AWS_SERVICE_URL), ...) pattern is readable and ensures the cases still run against real AWS.
• Consistent rename — LOCALSTACK_SERVICE_URL → AWS_SERVICE_URL is applied uniformly across CI, test helpers, samples, and docs. No stragglers found.
• S3 endpoint simplification — removing the http://s3.{authority} rewrite for AmazonS3Config is correct; Moto serves all services from the single endpoint.

---

🔍 Observations / Suggestions

1. Unit tests gaining [Trait("Category", "AWS")] — is that intentional?

Three test classes that appear to be pure unit tests (no real AWS infrastructure) have been tagged:

• SqsMessageConsumerFactoryDlqTests (When_creating_sqs_consumer_with_dlq_subscription_should_pass_routing_keys.cs)
• SqsSubscriptionDlqRoutingKeyTests (When_creating_sqs_subscription_with_dlq_routing_keys_should_expose_properties.cs)
• When_creating_sns_attributes_with_tags

Adding [Category, "AWS"] means these will only execute in the aws-mock-ci job and be excluded from the default build. If they're pure unit tests, they should arguably run in every build (no tag). If they do need a live endpoint, the tag is correct. Worth a deliberate choice either way.

2. GetBucketAddressTemplate still produces virtual-hosted-style URLs for Moto

return $"http://{{BucketName}}.s3.{{BucketRegion}}.{uri.Authority}";
// → http://{BucketName}.s3.{BucketRegion}.localhost:4566

Moto supports virtual-hosted-style access, so this works in practice, but it's different from the simpler path-style that cfg.ServiceURL = serviceUrl implies. If you ever see S3 presigned URL failures locally, this template is a likely culprit. No change needed now, just something to keep in mind.

3. Task.Delay(5000) in the reactor redrive test (pre-existing)

// When_throwing_defer_action_respect_redrive.cs, ~line 1088
Task.Delay(5000).GetAwaiter().GetResult();

This is a pre-existing hardcoded 5-second wait before checking the DLQ count. Now that this test runs in CI, it will contribute to the 8-minute job budget on every run. Not a blocker, but consider a polling helper with a shorter poll interval + timeout (e.g., poll every 200ms up to 10s) if the test proves flaky under load.

4. aws-ci real-AWS job status

The PR correctly notes the TopicLimitExceededException in the CI account is not a regression from this PR. A follow-up to clean the account before this merges (or right after) would be valuable so the real-AWS smoke run can confirm no regressions.

5. Minor: When_throwing_defer_action_respect_redrive.cs — GetDLQCount still named GetDLQCountAsync in V4 Reactor tests?

The v3 Reactor file renames GetDLQCountAsync → GetDLQCount. Confirm the v4 equivalent was also updated (the diff shows v4 Proactor was updated; want to be sure v4 Reactor is consistent).

---

🔒 Security / Configuration

• BasicAWSCredentials("test", "test") used when AWS_SERVICE_URL is set — correct and standard practice for mock AWS. No real credentials in source.
• No new secrets or tokens introduced.

---

📋 Test Coverage

Scenario	Before	After
AWS mock (Moto)	LocalStack (paid)	Moto OSS, pinned 5.1.22
Previously-skipped redrive tests	8 skipped	8 now pass ✅
SQS tag verification (Moto interop)	N/A	1 SkippableFact (runs on real AWS)
V4 fairQueue cases	N/A	8 SkippableTheory (needs Moto 5.1.23+, tracked in #4096)
AWSScheduler (EventBridge)	Real AWS only	Still real AWS only (EventBridge not in Moto)

136 v3 / 130 v4 passed locally against Moto — solid result.

---

Verdict

The core migration is clean and correct. The bug fixes are a genuine improvement. The outstanding items (real-AWS CI cleanup, fairQueue Moto bump) have appropriate tracking in #4096. The unit-test categorization point is the only thing worth a deliberate decision before merge.

🤖 Generated with Claude Code