chore(aws-tests): swap Moto for Floci as the AWS mock for integration tests#4148
chore(aws-tests): swap Moto for Floci as the AWS mock for integration tests#4148slang25 wants to merge 6 commits into
Conversation
Floci handles SQS tag verification and SQS/SNS fair-queue scenarios that Moto 5.1.22 didn't, so the corresponding Skip.If guards come off (+1 V3, +8 V4 tests now running against aws-mock-ci). Cold start is also ~3.9x quicker (~2.7s -> ~0.7s). Also fixes a latent test bug in the V3 + V4 Reactor ByUrl tests where the queue name was passed as channelName while declaring FindQueueBy.Url — Moto tolerated the mismatch, Floci (and real AWS) don't. Test helper uses the short-vhost bucket-address template when AWS_SERVICE_URL is set, to work around floci-io/floci#977 (200 OK on HEAD against the region-qualified vhost form regardless of bucket existence). Real-AWS runs still use the default full vhost template. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
slang25
requested review from
DevJonny,
holytshirt,
iancooper and
preardon
as code owners
github-actions
Bot
added
the
Maintenance
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Review by Claude (Opus 4.7) - solid, well-scoped chore PR. Net effect: more tests running in CI, faster cold start, and one production-relevant bug surfaced. Full review follows in subsequent comments due to length constraints. |
|
What is good: 8 previously-skipped V4 fair-queue parameterisations + 1 V3 SQS tag test now run in aws-mock-ci. Skip.If/SkippableTheory fully removed from both AWS test projects. Genuine bug fix in When_infrastructure_exists_can_verify_by_url.cs (Standard Reactor, V3+V4): was declaring findQueueBy QueueFindBy.Url but passing the queue name as channelName. The Standard FIFO Reactor variant already passed the URL (Fifo/Reactor/When_infastructure_exists_can_verify_by_url.cs line 69), so this brings Standard Reactor in line. Floci vhost workaround in CredentialsChain.cs is properly documented with a link to floci-io/floci#977. Cold-start improvement (~2s) is cumulative across CI runs. |
|
Nit on the PR description: it claims the Reactor fix just makes things consistent with the Async sibling, but the Standard async sibling (Standard/Proactor/When_infrastructure_exists_can_verify_by_url_async.cs lines 58-65) does NOT set findQueueBy QueueFindBy.Url on its validation subscription at all (only on the producer). So they are still not symmetric after this PR. Not a blocker, just noting the PR text overstates the consistency angle - the real motivation is that real AWS, like Floci, rejects a name where a URL is expected. |
|
Concern 1 (biggest): floci/floci:latest is unpinned in both docker-compose-aws.yaml (line 3) and .github/workflows/ci.yml (line 454). The image being replaced was pinned to motoserver/moto:5.1.22, which is the right pattern. Floci is explicitly described as 'under active development', which makes :latest extra risky - a future Floci push could break CI on master without any code change in this repo. Recommend pinning to a specific semver tag (or, even better, an image digest) and bumping deliberately via Dependabot or a manual PR. The same hazard that motivated the original 5.1.22 Moto pin now applies in reverse. |
|
Concern 2: Health-check drift between docker-compose-aws.yaml and the CI workflow. The compose file uses CMD-SHELL with a fallback (try /_localstack/health, otherwise /), while ci.yml uses only /_localstack/health. Pick one. If the fallback is genuinely needed for local docker compose (e.g., older Floci builds people might pull), then CI is silently relying on the new endpoint always existing - which combined with :latest is two coupled hazards. If the fallback is not needed, drop it from compose so they match. Either way, having them diverge invites the local-passes-CI-fails or vice-versa class of bug. |
|
Concern 3: Orphan dependency. Xunit.SkippableFact is still referenced in tests/Paramore.Brighter.AWS.Tests/Paramore.Brighter.AWS.Tests.csproj (line 21) and tests/Paramore.Brighter.AWS.V4.Tests/Paramore.Brighter.AWS.V4.Tests.csproj (line 20), but there are zero remaining Skippable or Skip.If usages in either project after this PR. Either remove the PackageReference (and the Directory.Packages.props line 156 PackageVersion if nothing else needs it) in this PR, or open a follow-up. Worth doing here so the dead reference does not linger. |
|
Concern 4 (minor): The Floci workaround in CredentialsChain.GetBucketAddressTemplate() unconditionally switches to short-vhost form whenever AWS_SERVICE_URL is set. The comment correctly links to floci-io/floci#977, but if a future emulator (or a Moto comeback) requires the full region-qualified form, this override quietly becomes wrong. Consider either gating the override behind an explicit env signal (something like USE_SHORT_VHOST=1) or adding a clear TODO with revert instructions tied to the upstream issue. Easy to forget once #977 is resolved. |
|
Non-issues / scope notes: No src/ changes - confirmed test-only, no production code touched. No security or regression surface. Whitespace-only edit in When_creating_luggagestore_missing_ACL (V3 + V4) is a harmless drive-by. When_posting_a_message_resources_are_tagged_async going from SkippableFact to Fact is correct now that Floci handles SQS tag verification, but only exists for the V3 Proactor - matches the PR's stated counts (V3 +1, V4 +8) but worth a quick check that V4 does not have a similar un-skip opportunity hiding elsewhere. Test coverage: counts in the PR description (V3 136 to 137, V4 130 to 138) align with the diff (9 newly-running parameterisations total). |
|
Summary: LGTM in intent and almost all the mechanics. The single must-do before this lands is pinning the Floci image - :latest on a 'under active development' upstream is the same hazard that motivated the original Moto pin. Ideally also drop the now-unused Xunit.SkippableFact reference in the same PR (or as an immediate follow-up). Reconcile the docker-compose vs CI health-check definitions while you are at it. The CredentialsChain workaround is fine to merge as-is given the linked upstream issue, but worth a TODO to make the revert obvious later. Nice cleanup of the test-skip backlog. |
…ound - Pin floci/floci to 1.5.19 in docker-compose-aws.yaml and ci.yml rather than tracking :latest (Floci is under active development). - Drop the short-vhost bucket-address template workaround: 1.5.19 ships floci-io/floci#978 which fixes the region-qualified vhost HEAD bug (floci-io/floci#977), so the helper can return the default {bucket}.s3.{region}.{host} form again. - Drop the now-redundant /_localstack/health || / fallback from the compose healthcheck so it matches the CI workflow. - Remove the orphan Xunit.SkippableFact PackageReference from both AWS test projects and from Directory.Packages.props — the last SkippableFact/Skip.If usages came out with this PR's first commit. Verified locally against 1.5.19: V3 137/137 and V4 138/138 passing, no skips. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ReviewNice, focused PR — test-infra only, scoped tightly, version pinned, no production code touched. I verified the cleanup is complete (no A couple of things worth addressing1. Documentation still mentions Moto (likely out of scope, but easy follow-ups):
Not blocking, but the CONTRIBUTING.md row is the one most likely to confuse new contributors who clone the repo and grep for ""Moto"". 2. Healthcheck form changed in 3. Minor / nit: Both Things I checked and am happy with
Security / supply chainFloci is a smaller, newer project than Moto, so the pin matters more. The PR is doing the right thing by pinning explicitly rather than chasing PerformanceThe ~2.7s → ~0.7s cold-start improvement is a real CI win across the AWS jobs. Not a primary motivation but a nice bonus. Approve from my side once the CONTRIBUTING.md row is at least updated; the sample READMEs and EventBridge comments can be a separate follow-up. 🤖 Generated with Claude Code |
…mments
Addresses follow-up review notes on the Floci swap:
- CONTRIBUTING.md compose-file table row: "Moto (AWS)" -> "Floci (AWS)".
- samples/Scheduler/QuartzTaskQueue/README.md: walk-through and resource
link now point at Floci instead of Moto.
- samples/Scheduler/AwsTaskQueue/{GreetingsPumper,GreetingsReceiverConsole}/Program.cs:
remove the stale "EventBridge Scheduler is not implemented by Moto"
comment and have the samples honour AWS_SERVICE_URL when set. Verified
against floci/floci:1.5.19 that scheduler.CreateSchedule actually fires
and delivers to an SQS target on time, so the samples can now be run
end-to-end against the local emulator.
- Revert an unintentional trailing-whitespace edit in
When_creating_luggagestore_missing_ACL (V3 + V4) that snuck in via an
earlier reverted change.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Code ReviewFocused, low-risk swap — read through the full diff and spot-checked the sibling tests around the bonus fix. Looks good to merge. A few observations below; none are blockers. What works well
Observations / follow-up candidates (non-blocking)
Tests
Security / performance
SummaryLGTM. The bonus fix is the most consequential code change and it's both correct and minimal. The two scheduler-sample lines and one async-sibling-test concern are worth a quick triage but neither belongs in this PR. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Code ReviewReviewed the swap from What's working well
Concerns1. (Medium) Sample programs may break against
But these samples use Either:
As-is, a user with 2. (Low) Healthcheck shell-form vs exec-form change.
3. (Low / pre-existing, not introduced here) The async sibling at TestsNet +9 active tests (no losses, no new skips). The bug fix is exercised by the previously-passing-but-incorrectly-configured Security / performance
SummaryApprove once point 1 (sample behavior change) is either substantiated or reverted. Points 2 and 3 are nits/follow-ups. |
Floci treats a 12-digit AWS_ACCESS_KEY_ID as the account ID for per-account resource isolation. Pinned Floci 1.5.19 (via earlier commit in this PR) also includes floci-io/floci#983 which makes STS GetCallerIdentity honour the same convention, so ARN construction via the convention validator now matches the SNS/SQS namespace. CredentialsChain in both AWS test projects now hands out a random 12-digit access key per test-runner process (cached in a static field). This means V3 and V4 can share a single Floci container without name collisions, and we can later run them as parallel `dotnet test` invocations in aws-mock-ci. Verified locally against floci/floci:1.5.19 with V3 and V4 running concurrently as separate dotnet test processes: - V3 137/137 passed - V4 138/138 passed - Wall clock 43s vs. ~67s sequential Note: parallel `dotnet test` invocations on the same solution will race on shared project output (e.g. Paramore.Brighter.deps.json), so a future CI parallelisation will need to pre-build the test projects and then run with --no-build --no-restore. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Code ReviewOverall this is a clean, well-scoped infrastructure swap. The PR description does an excellent job documenting the rationale, the bonus fix, and the upstream Floci issues. Production code is untouched, test coverage strictly improves (8 fewer skips on V4, 1 fewer on V3), and the dependency surface shrinks ( A few observations: Potential concern: Scheduler samples will likely break under
|
Follow-up to the Floci swap. The two Scheduler/AwsTaskQueue samples
honored AWS_SERVICE_URL after the earlier commit but still wouldn't
start against Floci end-to-end because:
1. Both samples gated all service registration on
CredentialProfileStoreChain.TryGetAWSCredentials("default"), so a
developer with no AWS profile (which is most people running against
a local emulator) silently got no services at all. Now: when
AWS_SERVICE_URL is set, hand out a random 12-digit BasicAWSCredentials
(Floci uses 12 digits as the account ID for per-account namespace
isolation); otherwise fall back to the default profile as before.
2. AwsSchedulerFactory in the Pumper assumed the IAM role
"brighter-scheduler" already existed. The AWSScheduler test fixtures
already set MakeRole = OnMissingRole.Create for the same reason;
the sample should too, otherwise the first run anywhere (including
real AWS in a fresh account) blows up before scheduling anything.
Verified locally against floci/floci:1.5.19 with
AWS_SERVICE_URL=http://localhost:4566:
- Pumper schedules 200 messages over 2 minutes without errors.
- Floci logs confirm CreateRole, CreateScheduleAsync, and SNS topic
setup all succeeded.
- (Receiver has a pre-existing FIFO topic/queue Type mismatch — out
of scope for this PR.)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
1 participant
Description
Swaps
motoserver/moto:5.1.22forfloci/floci:1.5.19indocker-compose-aws.yamland theaws-mock-ciworkflow. Same port (4566), sameAWS_SERVICE_URLcontract, no production code touched.Why
A few
Skip.If(AWS_SERVICE_URL is set, ...)guards in the AWS tests existed because Moto either didn't implement a thing or had it gated behind a newer version than we pin. Floci handles them, so the guards come off:Counts: V3
aws-mock-ci136 → 137 passing, V4aws-mock-ci130 → 138 passing. No new skips.Cold start is noticeably quicker too —
docker runto health-endpoint-responding goes from ~2.7s on Moto to ~0.7s on Floci on my machine. Worth a few seconds per CI run.Floci is also under active development and explicitly aims to be a drop-in LocalStack-Community replacement — Why Floci covers the project's goals.
Bonus fix exposed by stricter behaviour
When_infastructure_exists_can_verify_by_url(V3 + V4 Standard Reactor) was passing the queue name aschannelNamewhile declaringFindQueueBy.Url. Moto tolerated the mismatch; Floci (and real AWS) don't. Now uses the queue URL as the channel name, consistent with the Standard FIFO Reactor sibling.Floci dependencies (now landed upstream)
Two issues were filed and resolved against Floci while this PR was in review; both fixes ship in the pinned
1.5.19:HEADon{bucket}.s3.{region}.localhost:4566returned 200 for any host. Fixed by #978. No test-side workaround required; the bucket-address template is unchanged from before this PR.GetCallerIdentityignored the 12-digit access-key account-ID convention. Fixed by #983. Opens up a future option of per-process random account IDs for concurrent V3/V4 runs against a single Floci container; left as a follow-up.Housekeeping
Xunit.SkippableFactis no longer used by either AWS test project (allSkip.Ifguards came out with the swap), so thePackageReferenceand itsPackageVersioninDirectory.Packages.propsare removed.curl -sf http://localhost:4566/_localstack/health— the redundant|| curl /fallback in compose is gone.Related Issues
Upstream: floci-io/floci#977, floci-io/floci#981 — both closed.
Type of Change
Checklist
Additional Notes
Replaces #4147 (closed; that one was branched from the fork, which meant the gated CI jobs were being skipped). Verified locally against
floci/floci:1.5.19withAWS_SERVICE_URL=http://localhost:4566: V3 137/137 and V4 138/138 passing, no skips.