feat(testdoubles): add CallVerifier.AtLeast/AtMost, ReceivedCalls() and ClearCalls() - #134
Conversation
…nd ClearCalls() Adds AtLeast(int)/AtMost(int) count verification to core CallVerifier, forwarded through Compono.Logging's LogVerificationBuilder, and reachable from Compono.TestDoubles/Compono.Http with no package-side changes (ADR-0044 Amendment 22). Adds Compono.TestDoubles generated ReceivedCalls() (snapshot-based retrospective call inspection for ADR-0048-eligible members) and whole-double ClearCalls() (observation reset preserving configured behavior) per ADR-0060. Includes generator/runtime changes, tests across all affected packages (unit, generated-source snapshots, AOT smoke, concurrency), docs, skill and eval updates, and a completed baseline-vs-updated eval comparison and dogfood validation run, all tracked in PLAN-0063. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NiVv392P3m46azTD1TpU3s
![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: 593f54e4df
ℹ️ 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".
| // each member's own existing synchronization primitive is used independently, per member. | ||
| internal static class {{ safe_identifier }}_ClearCallsExtension | ||
| { | ||
| public static void ClearCalls(this {{ interface_fully_qualified_name }} self) |
There was a problem hiding this comment.
When a target interface already declares an applicable zero-argument ClearCalls or ReceivedCalls member, C# instance-member lookup wins over these extensions. For example, repository.ClearCalls() will invoke the interface member implemented by the generated double and silently leave its call history intact. The analyzer's bridge-collision check still reserves only Configure and Verify, so extend that check to reject these collisions with a diagnostic before emitting the double.
AGENTS.md reference: AGENTS.md:L183-L185
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48816c8: widened TestDoubleAnalyzer's CMP0023 reserved-name collision check to also cover ClearCalls/ReceivedCalls, not just Configure/Verify. Added ClearCallsNamedMember_ReportsCollisionDiagnostic/ReceivedCallsNamedMember_ReportsCollisionDiagnostic generator-fixture regression tests.
| if (member.IsEligibleForMatching) | ||
| reservedNames.Add(member.ReceivedCallClassName); |
There was a problem hiding this comment.
For a valid interface containing an eligible Foo(int value) plus a configurable sibling named Foo_ReceivedCall, the generated double declares both the record type __Foo_ReceivedCall and the sibling's backing field with that same identifier, producing a consumer compilation error. Adding the record name to this callback-only reservation set can rename colliding callback declarations, but it neither renames the record nor feeds the earlier derivedAuxiliaryNameOwners pass that handles sibling-generated declarations; include this new derived name in that collision analysis or otherwise disambiguate it.
AGENTS.md reference: AGENTS.md:L183-L185
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48816c8: the derived {FieldName}_ReceivedCall record name now feeds into the existing derivedAuxiliaryNameOwners pre-pass alongside _calls/_lock/_Entry/_entries, so a real sibling-name collision demotes the eligible member out of matching eligibility (falls back to its plain configuration surface) instead of emitting a duplicate declaration. Added ReceivedCallRecordNameCollidesWithSiblingMember_FallsBackWithoutRejectingEligibleMember covering the exact scenario you described.
| // internal {{ member.field_name }}_calls tuple above uses for its own, unrelated, internal-only | ||
| // matching purpose). Not emitted for an overload-matching-eligible member - ReceivedCalls() is | ||
| // scoped to exactly ADR-0048's non-overloaded eligible-member set for 1.1. | ||
| internal readonly record struct {{ member.received_call_class_name }}({{ for p in member.parameters }}{{ p.fully_qualified_type_name }} {{ p.escaped_name }}{{ if !for.last }}, {{ end }}{{ end }}); |
There was a problem hiding this comment.
A legal eligible method such as Foo(int __Foo_ReceivedCall) makes this render record struct __Foo_ReceivedCall(int __Foo_ReceivedCall), whose synthesized positional property has the same name as its enclosing type and fails with CS0542. Other record-generated member names can similarly collide because parameter names are copied verbatim without validation. Make the exposed property names collision-safe or report an unsupported-shape diagnostic rather than emitting uncompilable consumer code.
AGENTS.md reference: AGENTS.md:L183-L185
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48816c8: a parameter whose name equals the record's own type name is now suffixed _Value in the record declaration only (positional construction elsewhere is order-based, not name-based, so nothing else needed changing). Added ReceivedCallRecordParameterNameCollidesWithRecordType_ConsumerCompiles covering it.
…kage-validation CI, regen API docs
Three real generator-correctness bugs found by automated PR review, each with a
new generator-fixture regression test:
- ClearCalls()/ReceivedCalls() are always-emitted, always-zero-argument bridge
extensions exactly like Configure()/Verify(), but the CMP0023 reserved-name
collision check only covered the latter two - an interface declaring its own
zero-argument ClearCalls/ReceivedCalls member silently shadowed the generated
bridge with no diagnostic. Widened the reserved-name set in TestDoubleAnalyzer.
- An eligible member's generated {FieldName}_ReceivedCall record-class name could
collide with an unrelated sibling member's own natural field name, producing a
real CS0102 duplicate-declaration compile error. Fixed by feeding this derived
name into the existing derivedAuxiliaryNameOwners collision pre-pass, which
demotes the colliding member out of matching eligibility instead.
- A parameter literally named the same as its own member's generated record type
produced a positional record property sharing the enclosing type's name
(CS0542). Fixed in TestDouble.scriban by suffixing that one parameter's
declared name inside the record declaration only.
Also fixes two unrelated CI blockers surfaced on this PR:
- .github/workflows/package-validation.yaml never set -p:Version on its
validation-only pack, defaulting to 1.0.0.0 - this started failing ApiCompat's
CP0003 the moment nuget.org's published baseline crossed 1.0.0
(1.1.0-preview.103, published by PR #133's merge to main). Every PR's
package-validation would fail this way regardless of content; pinned the
packed Version to the resolved baseline.
- docs/reference/api/ had drifted from PLAN-0063's new public members
(CallVerifier.AtLeast/AtMost, LogVerificationBuilder.AtLeast/AtMost,
ReturnConfig<T>.ClearObservedCalls) - regenerated via
.github/scripts/generate-api-reference.sh.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NiVv392P3m46azTD1TpU3s
|
@codex review commit 48816c8 specifically - it addresses your three prior findings plus two unrelated CI fixes. Please focus on:
Not asking for a full re-review of the whole diff - just these four areas. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48816c8bf6
ℹ️ 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".
| // property name; suffixed with "_Value" in that one case (Codex review, PR #134). Two real | ||
| // parameters can never already share a name (the compiler already guarantees that for the real | ||
| // member this record mirrors), so at most one parameter in this list ever needs the suffix. | ||
| internal readonly record struct {{ member.received_call_class_name }}({{ for p in member.parameters }}{{ p.fully_qualified_type_name }} {{ if p.escaped_name == member.received_call_class_name }}{{ p.escaped_name }}_Value{{ else }}{{ p.escaped_name }}{{ end }}{{ if !for.last }}, {{ end }}{{ end }}); |
There was a problem hiding this comment.
Fresh evidence is the legal multi-parameter shape Foo(int __Foo_ReceivedCall, int __Foo_ReceivedCall_Value): this conditional renames the first parameter to __Foo_ReceivedCall_Value, duplicating the second parameter in the generated positional record and making the consumer compilation fail. The new fixture only covers the single-parameter case, so choose a name absent from every real parameter or report a diagnostic.
AGENTS.md reference: AGENTS.md:L183-L185
Useful? React with 👍 / 👎.
…ion-free, not just naive Codex review round 2 (PR #134) caught a real gap in the previous fix: the scriban template's unconditional "_Value" suffix for a parameter colliding with its own record's type name could itself collide with a second real parameter already literally named that (`Foo(int __Foo_ReceivedCall, int __Foo_ReceivedCall_Value)`), producing a duplicate positional-record-property compile error. Moved the rename computation into a new TestDoubleMemberInfo.ReceivedCallRecordParametersText C# property, which checks the candidate against every real parameter's own name (not just the colliding one) and increments a numeric suffix until free (__Foo_ReceivedCall_Value2, _Value3, ...). Wired through TestDoubleEmitter.cs's anonymous projection model, which the first attempt had missed - Scriban's default reflection binding only sees properties on the wrapper anonymous object passed to Render(), not arbitrary computed properties on the underlying TestDoubleMemberInfo record. Added ReceivedCallRecordParameterNameCollidesWithBothRecordTypeAndItsNaiveRenameCandidate_ConsumerCompiles covering exactly the two-parameter collision Codex identified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NiVv392P3m46azTD1TpU3s
|
@codex review commit 9f70037 specifically - it addresses your round-2 finding on the record-parameter rename. What changed: the collision-avoidance logic moved out of the scriban template and into a new C# property, The new algorithm: seed a Please specifically verify:
Not asking for a full re-review - just confirm this closes the gap airtight. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
1 participant
📋 Summary
Implements PLAN-0063:
CallVerifiergainsAtLeast(int)/AtMost(int)(ADR-0044 Amendment 22), andCompono.TestDoublesgains generatedReceivedCalls()(retrospective, snapshot-based call inspection for the ADR-0048-eligible member set) andClearCalls()(whole-double observation reset preserving configured behavior) (ADR-0060).📝 Changes
Compono):CallVerifier.AtLeast/AtMost, matchingExactly's existing structure and exception message format.Compono.Logging:LogVerificationBuilder.AtLeast/AtMostforwarders through the existingToCallVerifier().Compono.TestDoublesgenerator/runtime: new generatedreadonly record structper ADR-0048-eligible member,ReceivedCalls()bridge (snapshot-based,IReadOnlyList<T>), andClearCalls()(clears call counts/histories, preserves configuredReturns/Throws/sequences). Two real compile errors found and fixed against generator fixtures during implementation (matched-parameters closed-instantiation shape's missing top-levelConfigfield; a type parameter literally namedConfigshadowing the field).Compono.Http/Compono.TestDoubles:AtLeast/AtMostreachable with zero package-side code changes.CallVerifier, package reachability tests (Compono.Http.Tests,Compono.Logging.Tests),ReceivedCalls()/ClearCalls()coverage (snapshot isolation, reference-retention semantics, sequence non-rewind, concurrency race test), generated-source snapshot tests, and an AOT smoke test extension.docs/packages/compono-testdoubles.md,compono-http.md,compono-logging.mdupdated with new sections/bullets.skills/compono/SKILL.mdand references updated to remove stale "not supported" claims;evals.jsongrew from 46 → 52 evals, with a recorded baseline-vs-updated comparison (Task 12 of the plan) showing no regressions.🧪 Validation
dotnet build -c Release— 0 errors.dotnet test -c Release— 3567/3567 passed, 0 failed (all target frameworks: net8.0/net9.0/net10.0/net11.0).scripts/dogfood-validate.shrun against the trivia-platform consumer repo with a freshly packed local version — 783/783 consumer tests passed, no packaging regression.ClearCalls()concurrency race (200-iterationBarrier-synchronized test), reference-retention vs. value-copy semantics for captured arguments,ClearCalls()not rewinding a configuredReturnsSequence, negative/boundary counts forAtLeast/AtMost.💬 Notes for Reviewers
Full task-by-task execution detail (including the two real compile-error fixes found via generator fixtures, and the mandatory skill eval baseline comparison) is recorded in PLAN-0063's Tasks/Notes sections.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NiVv392P3m46azTD1TpU3s