[iancooper]

…pt can find orphaned resources

FIFO test fixtures were creating SNS topics without the Environment=Test tag, causing orphaned FIFO topics to accumulate and hit the SNS topic quota. Also updates cleanup script with missing test prefixes and corrects the target region to eu-west-1.

Description

Related Issues

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the Contributing Guide
• I have checked the documentation for relevant guidance
• I have added/updated XML documentation for any public API changes
• I have added/updated tests as appropriate
• My changes follow the existing code style and conventions

Additional Notes

[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 for PR 4122

Thanks for tackling this — orphaned FIFO topics hitting the SNS quota is a real operational pain. The fix is well-scoped and the pattern is consistent across the ~40 test fixtures touched.

What works well

• Real bug, narrow fix. FIFO SnsAttributes were the only ones missing Environment=Test; standard SNS fixtures and SQS attributes already carry the tag, so this PR brings FIFO into line with what the rest of the suite already does. Confirmed by spot-checking Standard/Proactor and Standard/Reactor fixtures in both Paramore.Brighter.AWS.Tests and Paramore.Brighter.AWS.V4.Tests.
• Constructor over object initializer is the right call here — SnsAttributes exposes a constructor (string?, string?, SqsType, bool, List<Tag>?), and switching from the property initializer to named parameters is cleaner once tags are involved.
• V3 / V4 parity is maintained: every change in Paramore.Brighter.AWS.Tests has its mirror in Paramore.Brighter.AWS.V4.Tests. Easy to miss; nice that it did not.

Concerns / follow-ups

1. Cleanup script prefixes are masking missing tags elsewhere. The new prefixes added to clean_failed_tests_aws_assets.sh (Consumer-DLQ-Fifo, DLQ-Reader, Invalid-Reader) point at SQS-only test fixtures that are also untagged today:

   • Consumer-DLQ-Fifo → tests/.../Sqs/Fifo/Reactor/When_rejecting_fifo_message_with_delivery_error_should_send_to_dlq.cs (and Proactor) — SqsAttributes is constructed with type: SqsType.Fifo, rawMessageDelivery: true and no tags.
   • DLQ-Reader → ~12 files under Sqs/Standard/{Reactor,Proactor} constructing SqsAttributes with no tags.
   • Invalid-Reader → 2 files in Sqs/Standard/{Reactor,Proactor} constructing SqsAttributes with no tags.

   The naming-convention pass is a sensible safety net, but the root fix is the same one this PR applies to FIFO SNS: pass tags: new Dictionary<string, string> { { "Environment", "Test" } } to those SqsAttributes calls. Otherwise the comment above TEST_PREFIXES ("particularly FIFO tests were not tagged") will keep accruing prefixes whenever a new untagged fixture lands. Worth a follow-up PR to tag those at source.

2. using placement is inconsistent. The added using Amazon.SimpleNotificationService.Model; lands in three different positions across the files:

   • Sometimes second, between System.Collections.Generic and System.Linq (e.g. When_a_message_consumer_reads_multiple_messages_async.cs).
   • Sometimes between System.Collections.Generic and System.Net.* (e.g. When_requeueing_redrives_to_the_dlq_async.cs).
   • Sometimes appended at the end of the using block (e.g. When_infastructure_exists_can_assume_async.cs, When_topic_missing_verify_throws.cs).

   Not a behavioural issue, but it breaks the "System namespaces first, then alphabetical" convention used elsewhere in the suite. A dotnet format pass on the touched files would normalise this.

3. API asymmetry surfaces in the diff (pre-existing, not introduced here). SqsAttributes takes Dictionary<string, string>? for tags while SnsAttributes takes List<Tag>?. The Dictionary form is materially nicer at call-sites — tags: new Dictionary<string, string> { { "Environment", "Test" } } vs tags: [new Tag { Key = "Environment", Value = "Test" }] plus a separate using Amazon.SimpleNotificationService.Model. Worth considering as a future ADR: align SnsAttributes.tags to the same dictionary shape and convert internally. Out of scope for this fix.

4. Region change worth double-checking. aws-cleanup.yml flips AWS_REGION from us-west-2 to eu-west-1. Confirm that the test workflows that actually create these resources are also running in eu-west-1, otherwise the cleanup will run against the wrong region and the orphans will keep accumulating in us-west-2. (If us-west-2 was historically used at all, a one-shot manual sweep there may be needed.)

5. Minor — repeated literal. The [new Tag { Key = "Environment", Value = "Test" }] literal is repeated ~40 times. A TestTags.Environment static helper in a Helpers/AwsTestExtensions class would make it a single source of truth and make the intent clearer at the call-site. Minor; happy to leave for a tidy-first follow-up.

Test coverage

No new tests, which is correct for this PR — these are test fixture changes, and the property under test (the tag is set) is essentially asserted by the cleanup script existence. There is no behaviour for the C# test suite to cover.

Severity summary

• Should fix as follow-up: Tag the SQS fixtures behind the new cleanup-script prefixes (item 1) so we stop chasing prefix lists.
• Nice to fix in this PR: Normalise using placement (item 2) — dotnet format should do it.
• Informational: Region change verification (item 4), API asymmetry follow-up (item 3), helper constant (item 5).

Overall, a clean, mechanical, low-risk fix for an annoying ops problem.