feat(testdoubles): sequential responses and overload-safe argument matching - #115
Conversation
Match and UseGeneratedTestDoubles live in the Compono namespace, not Compono.TestDoubles (the package name). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Snapshot skills/compono/references/testdoubles.md at 644a5ad (pre-fix) and rerun evals 28-31 against it, per skill-creator's paired-run requirement for skill-improvement benchmarks. old_skill passes 1/4 (eval 29 only) vs with_skill's 4/4, confirming the fix corrects real regressions rather than restating already-passing behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codex flagged that with_skill and old_skill runs weren't launched together, so they don't satisfy skill-creator's paired-run requirement. Rerun all 8 combinations (evals 28-31 x with_skill/ old_skill) in a single batch against the current skill and the 644a5ad pre-fix snapshot. Also verified and refuted the accompanying claim that only testdoubles.md was snapshotted: the snapshot always captured the full skills/compono directory, including the pre-fix SKILL.md (5139507 also added a guardrail section there). Net effect: old_skill now fails all 4 evals (0/4, down from 1/4) since eval 29's prior old_skill pass was a run-to-run fluke, not a stable result - stronger, cleaner evidence the fix corrects real regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior wording said the earlier baseline "snapshotted only testdoubles.md" then immediately said it "captured the full directory" - contradictory. Clarify that only the benchmark's description of the earlier baseline was wrong, not the snapshot itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tching Implements ADR-0054 (sequential/call-count-based responses via ReturnConfigBuilder<T>.ReturnsSequence(...)/SequenceOutcome.Throw(...)) and ADR-0044 Amendment 21 (overload-safe argument matching via a new <Member>Matching(Match<T1>, ...) member name sharing ADR-0050's entries/call-log/lock state with the unchanged discriminator surface), per PLAN-0054. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # skills/compono/references/testdoubles.md
#114 (dotnet-minor-patch Dependabot group) bumped Directory.Packages.props' TUnit.Core/TUnit floor from 1.64.13 to 1.65.38, but package-validation.yaml's inspect-packed-nupkgs.sh still asserted the old floor as Compono.TUnit's "intended tested range" (ADR-0031 Amendment 1) - a deliberate, human-gated check that a dependency floor bump has actually been validated, not meant to silently track whatever's currently pinned. The Dependabot PR's own CI run already proved 1.65.38 builds and tests clean; this just acknowledges that in the check itself. Confirmed locally: packed and ran inspect-packed-nupkgs.sh against all 8 publishable packages, all green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both were Accepted (capability) / Proposed (API) pending implementation. PLAN-0054 has now implemented and validated both (unit/generator-execution/ snapshot coverage, real Native AOT publish-and-run, and a real consumer dogfood pass) - flip to Accepted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ea32d0c6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- docs/reference/api: regenerate from XML docs (ADR-0032 drift check) - SequenceOutcome<T>/SequenceOutcome/ThrownOutcome, ReturnConfig<T>'s new HasConfiguredSequence/NextSequenceOutcome(), and ReturnConfigBuilder<T>.ReturnsSequence(...) were new public API this session added without ever regenerating this directory - would have failed docs.yml's drift check on push. - docs/packages/compono-testdoubles.md: was still stating "no sequential/ call-count-based responses" and "no argument matching on an overloaded member" in four places (Call verification, Multiple response configurations per member, and the closing "What it deliberately doesn't do" summary) - added "Overload-safe argument matching" and "Sequential/call-count-based responses" sections mirroring the skill's own testdoubles.md, and corrected every stale negative claim. Verified: `uv run mkdocs build --clean --strict` succeeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82bccd15cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Five confirmed issues across two review passes: - TestDoubleAnalyzer.cs: the <Member>Matching alias-collision check compared nullable-aware display-string text instead of real C# signature identity, missing a collision between e.g. "string" and "string?" (real signature identity never considers nullable annotations). It also only considered non-overloaded real members as possible collision sources, missing that an ordinary overloaded real member's own discriminator extension can collide too. Rewrote both checks to compare real ITypeSymbols via SymbolEqualityComparer.Default (nullability-insensitive) across every real member sharing the candidate name, not just one eligibility shape. - ReturnConfigBuilder.cs: ReturnsSequence(...)'s XML doc still described the rejected dual-implicit-conversion design (a bare Exception implicitly converting) instead of the shipped SequenceOutcome.Throw(...) API. Also stored the caller's own params array by reference instead of snapshotting it - a caller mutating the array after configuring would silently change an already-configured response and violate NextSequenceOutcome()'s lock-free-safety premise that the sequence is immutable once configured. - docs/packages/compono-testdoubles.md: the "Why this doesn't apply to an overloaded member" paragraph flatly contradicted the newly-added "Overload-safe argument matching" section - clarified that the compiler spike's finding is scoped to the same-call-site shape, not a blanket "no matching on overloaded members" claim. Also, per this round of review: added a coding-standards.md/AGENTS.md rule (mirrored, since AGENTS.md is a condensed pointer at coding-standards.md) capturing the general lesson - a generator's collision-prediction logic must compare real ITypeSymbols via SymbolEqualityComparer.Default, and must consider every real member sharing a candidate name, not just the ones reachable through the code path being written. Regenerated docs/reference/api for the XML doc change (ADR-0032 drift check). New regression coverage: two generator-execution snapshot tests proving both collision-detection fixes fire the hash-suffixed fallback, one unit test proving the array-snapshot fix. Validated: full solution sweep (823/823 on net10.0), Compono.Generators.Tests on net10.0 and net11.0 (276/276 each, zero unexpected snapshot diffs - every prior fixture's generated output is unchanged), `uv run mkdocs build --clean --strict` succeeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review focus specifically on commit 7a7cd17, which fixes the 5 issues from your two previous review rounds. Please verify:
No need to re-review anything before 7a7cd17 unless this commit's changes interact with it in a way that matters. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a7cd17daf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two confirmed issues, both in the same collision-detection code the previous round rewrote: - The real-signature pre-pass unconditionally excluded every closed-instantiation-eligible (ADR-0049) candidate, on the assumption only "ordinary" real members needed considering. A non-overloaded closed-instantiation-eligible member with real matched parameters emits a Match<T>-wrapped generic Configure<T>() extension exactly like the new alias does, so it can collide with an overloaded generic member's own alias (e.g. Foo<T>(int)/Foo(string) -> FooMatching<T>(Match<int>)) just as easily as any other real member. Removed the exclusion - the existing isMatchingEligible branch already computes the correct signature for both the non-overloaded (Match<T>-wrapped) and overloaded (real, unwrapped) closed-instantiation shapes once given the chance to run. - The stored/compared signature only tracked parameter types, not generic arity - also part of real C# signature identity (PLAN-0054's own "Naming/collision policy" Finding 4 already established this). A real non-generic FooMatching(int) and a generic alias FooMatching<T>(Match<int>) share parameter types but not arity, so they don't actually collide - the prior comparison treated them as if they did, forcing the whole alias family to an unnecessary hash-suffixed name. Signatures are now compared as (arity, parameter types) tuples. New regression coverage: one generator-execution snapshot test proving the closed-instantiation collision now triggers the fallback, one proving the arity-only mismatch no longer does (natural name kept, real overload set still coexists cleanly). Validated: full solution sweep (825/825 on net10.0), Compono.Generators.Tests on net10.0 and net11.0 (278/278 each, zero unexpected snapshot diffs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review focus specifically on commit 2753543, which fixes the two round-3 findings (closed-instantiation-eligible members excluded from collision detection, and generic arity missing from the signature comparison). Please verify:
No need to re-review anything before 2753543 unless this commit's changes interact with it in a way that matters. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27535434d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A candidate's own TypeParameters.Length is not always its generated extension's actual emitted arity - TestDoubleMemberInfo.ExtensionIsGeneric (the template's own governing rule) says a solo (non-overloaded, non-closed-instantiation) generic member's extension is always emitted non-generic, mirroring ADR-0044 Requirement 2's "one backing slot covers every closed instantiation" rule onto a matching-eligible member's own extension too. The round-3 fix used raw TypeParameters.Length as arity unconditionally, so a solo generic real member sharing a candidate name (e.g. FooMatching<T>(int), whose own extension actually emits as the non-generic FooMatching(Match<int>)) was recorded at the wrong arity and a genuine collision went undetected. Computed effective arity now mirrors ExtensionIsGeneric exactly: generic arity only counts when the candidate is itself overloaded or closed-instantiation-eligible, zero otherwise - regardless of the method's own real TypeParameters.Length. New regression coverage: a generator-execution snapshot test (OverloadMatchingAliasCollidesWithSoloGenericRealMemberEmittingNonGenericExtension_FallsBackToHashSuffixedName) reproducing Codex's exact scenario - Foo(int)/Foo<T>(string) plus a solo real generic FooMatching<T>(int) - confirming the fallback now fires. Validated: full solution sweep (826/826 on net10.0), Compono.Generators.Tests on net10.0 and net11.0 (279/279 each, zero unexpected snapshot diffs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review focus specifically on commit 71d9fc8, which fixes the round-4 finding (a solo generic real member's effective emitted arity being computed incorrectly - it was using raw TypeParameters.Length instead of mirroring TestDoubleMemberInfo.ExtensionIsGeneric's rule that a solo generic member's extension is always emitted non-generic). Please verify:
At this point we've been through 4 rounds of review on this same collision-detection logic ( |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
#76) * feat: migrate to Compono (from AutoFixture) and Compono.TestDoubles (from NSubstitute) Adopts Compono for composition (replacing AutoFixture) and Compono.TestDoubles for IAmazonDynamoDB test doubles (replacing NSubstitute), using the new overload-safe argument matching (DeleteItemAsyncMatching, ADR-0044 Amendment 21) and sequential/ call-count-based responses (ReturnsSequence, ADR-0054) capabilities - the two gaps that previously required NSubstitute for AcquireLockHandleAsync_DisposeHandle_ShouldCallReleaseLock and the three retry-loop tests. NSubstitute is now fully removed from the dependency graph. Validated via scripts/dogfood-validate.sh against local Compono packages across multiple rounds of PR review on the Compono side (LayeredCraft/compono#115) before this repo's own Directory.Packages.props is bumped to the real published release in the next commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: bump Compono/Compono.XunitV3/Compono.TestDoubles to 0.9.0-preview.88 The real published release containing overload-safe argument matching (ADR-0044 Amendment 21) and sequential/call-count-based responses (ADR-0054), the two capabilities the previous commit's migration depends on. Verified against the real package (not a local/dogfood build): dotnet restore + full dotnet test across all 4 TFMs (net8.0/net9.0/ net10.0/net11.0) - 180/180 passing, matching what scripts/dogfood-validate.sh already predicted in LayeredCraft/compono#115. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: widen timer-test delay/threshold margin to fix CI flake Codex review (PR #76): Compono.TestDoubles has no invocation-aware callback, so DelayedPutItemResponseAsync()/DelayedDeleteItemResponseAsync() are eagerly invoked (and their own Task.Delay starts counting) at Configure() time, one statement before the SUT actually awaits them - not when the SUT invokes the double. Any scheduling/composition overhead between that Configure() call and the SUT's own internal stopwatch starting eats directly into the delay budget, which the prior ~5ms delay against a ">4" threshold had essentially no margin to absorb - the exact CI failure just observed (2.21ms measured, not a lock-acquisition correctness bug). Compono.NSubstitute's invocation-aware Returns(callInfo => ...) would eliminate the race entirely, but reintroducing it here would partially undo the very NSubstitute-removal this PR is about. Widened the delay/threshold margin instead (100ms delay, >20ms threshold) so realistic CI scheduling overhead can't push the measured duration below threshold. Verified with 3 consecutive full local test runs (180/180 each). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: use TestContext.Current.CancellationToken for the outer SUT call DynamoDbDistributedLockTests.cs was passing CancellationToken.None as the token driving each sut.AcquireLockAsync/ReleaseLockAsync/AcquireLockHandleAsync call - inconsistent with Retry/RetryIntegrationTests.cs and Retry/ExponentialBackoffRetryPolicyTests.cs, which already correctly wire TestContext.Current.CancellationToken so the test framework's own cancellation (test timeout, --fail-fast, etc.) can actually reach the async call under test. CancellationToken.None never observes that. The discriminator-only Configure().PutItemAsync(new PutItemRequest(), CancellationToken.None)/DeleteItemAsync(...) calls are unaffected and correctly unchanged - real arguments there are discarded, used only for compile-time C# overload resolution on a non-matching-eligible overloaded member, not compared against what the SUT actually passes at runtime. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
ReturnConfigBuilder<T>.ReturnsSequence(...)/SequenceOutcome.Throw(...), wired through all dispatch shapes the generated double template covers.<Member>Matching(Match<T1>, ...)member name sharing the same entries/call-log/lock state as its unchanged discriminator-only surface (ADR-0050).Accepted(capability, API, and implementation) — ADR status flipped accordingly.package-validation'sTUnit.Coretested-range check, which fell out of sync after chore: Bump the dotnet-minor-patch group with 9 updates #114's Dependabot bump (1.64.13 → 1.65.38) and failed that PR'spackage-validationjob.main(fix(skill): correct TestDoubles matching guidance #113 skill-guidance fix, chore: Bump the dotnet-minor-patch group with 9 updates #114 Dependabot bump).Validation
Compono.Generators.Tests274/274 on net10.0 and net11.0).Compono.TestDoubles.AotSmokeTest) covering both phases' scenarios.dynamodb-distributed-lockviascripts/dogfood-validate.shwith fresh local packages — 180/180 passing (45/45 × net8.0/9.0/10.0/11.0), zero NSubstitute in the dependency graph after converting its 4 remaining NSubstitute-backed test sites to the new APIs.package-validation'sinspect-packed-nupkgs.shre-run locally against all 8 publishable packages — all green.Test plan
dotnet testfull solution sweep (net10.0)Compono.Generators.Testson net10.0 and net11.0Compono.TestDoubles.AotSmokeTest)dynamodb-distributed-lockdogfood pass viascripts/dogfood-validate.shinspect-packed-nupkgs.shre-run locally after the TUnit.Core fix