Conversation
jumboduck
commented
Apr 22, 2026
jumboduck
commented
- chore: rename smoke test targets to contract test targets
- docs: add ADR for fakes and contract tests pattern
Rename `test_smoke_aws` → `test_contract_aws` and `test_smoke_github` → `test_contract_github` to use consistent "contract test" terminology throughout the Makefile and TODO.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @jumboduck's task in 1m 47s —— View job PR Review: Contract Testing ADR
Static AnalysisAll checks pass — ReviewOverall: This is a well-written ADR that clearly documents the fakes + contract tests pattern. The Makefile rename from ADR quality: The document is well-structured and follows the existing ADR format established by the other two ADRs in Two minor suggestions posted as inline comments:
What's done well:
No bugs, security issues, or functional concerns — this is a docs + build target rename PR and it's clean. |
- Add 'who can run the tests' as a third problem in Context (Jon's comment) - Clarify that the shared contract function runs against both fake and real adapter — this is the mechanism that guarantees they behave identically (Tooky's comment) - Expand Consequences to reflect the contributor access improvement Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| test_contract_aws: ensure_gotestsum ## Run AWS contract tests against real AWS (requires AWS creds) | ||
| @echo "Running AWS contract tests against real AWS..." | ||
| @echo "Requires AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY to be set" | ||
| @$(GOTESTSUM) -- -v -p=1 -run "LambdaContract_RealAWS|AWSTestSuite/TestGetLambdaPackageData|AWSTestSuite/TestGetEcsTasksData|AWSTestSuite/TestGetS3Data" ./internal/aws/ |
There was a problem hiding this comment.
Nit: The AWSTestSuite/TestGetLambdaPackageData|AWSTestSuite/TestGetEcsTasksData|AWSTestSuite/TestGetS3Data tests included here aren't contract tests in the same sense as LambdaContract_RealAWS — they're integration tests that happen to hit real AWS. This is fine as a pragmatic aggregation target, but the target name test_contract_aws and help text "Run AWS contract tests" may be misleading once ECS and S3 get their own proper contract test suites (per TODO.md). Worth keeping in mind for the future, no action needed now.
|
|
||
| 2. **Write a shared contract test function** that asserts the key behavioural properties of the interface — what fields are present, what error behaviour to expect, and how edge cases are handled. | ||
|
|
||
| 3. **Run the contract tests against both implementations**: the shared contract test function is invoked twice — once against the fake (in the regular test suite, no credentials required) and once against the real service (env-gated, called from `make test_contract`). Running the same suite against both is what guarantees they behave identically. |
There was a problem hiding this comment.
Steps 3 and 4 read a bit out of order — step 3 talks about running contract tests against both implementations including the fake, but the fake isn't introduced until step 4. Consider swapping the order (build the fake first, then describe how the contract tests run against both) so a reader encountering this for the first time doesn't have to read ahead.
| 3. **Run the contract tests against both implementations**: the shared contract test function is invoked twice — once against the fake (in the regular test suite, no credentials required) and once against the real service (env-gated, called from `make test_contract`). Running the same suite against both is what guarantees they behave identically. | |
| 3. **Build an in-memory fake** that satisfies the same interface and passes the same contract tests. The fake is the only implementation used in the main integration test suite. | |
| 4. **Run the contract tests against both implementations**: the shared contract test function is invoked twice — once against the fake (in the regular test suite, no credentials required) and once against the real service (env-gated, called from `make test_contract`). Running the same suite against both is what guarantees they behave identically. |
| @@ -100,7 +100,7 @@ | |||
| - [ ] Fake-backed unit tests for orchestration (concurrent cluster/service/task fetching, error propagation) | |||
| - [ ] `NewECSClientFunc` factory + inject fake into `snapshotECS_test.go` command tests | |||
| - [ ] Trim existing ECS integration tests to smoke tests | |||
There was a problem hiding this comment.
The TODO items at lines 102 and 112 still say "Trim existing … integration tests to smoke tests". Now that the Makefile targets have been renamed from test_smoke_* to test_contract_*, should these also say "contract tests" for consistency?
4 participants
