fix(mocks): handle base classes with explicit interface impls (#5673)#5674
fix(mocks): handle base classes with explicit interface impls (#5673)#5674
Conversation
…rchy DiscoverMembers walked AllInterfaces for class partial mocks and re-emitted every interface member as `public override`, which fails when the base class implements the member explicitly (or non-virtually). Concretely, mocking Microsoft.EntityFrameworkCore.ChangeTracking.EntityEntry produced "CS0115: 'EntityEntryMockImpl.Instance': no suitable method found to override" because EntityEntry implements IInfrastructure<InternalEntityEntry>.Instance explicitly. Skip interface members for which the class already provides an implementation (FindImplementationForInterfaceMember is non-null) — the inherited impl already satisfies the interface, and the mock only needs to override what the class walk already collected. Also extends the KitchenSink suites with patterns that would catch similar regressions (explicit interface impls on abstract/concrete/inherited classes, derived interfaces with `new`-redeclared members) plus previously uncovered shapes: default interface methods, params arrays, tuple returns, IAsyncEnumerable, delegate returns, deeply-nested generics, nullable reference type arguments. Every new pattern has both a Returns()-driven test and verification via WasCalled. Fixes #5673
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Summary
This is a clean, targeted fix for a real compiler error (CS0115) that emerged when mocking types with explicit interface implementations (e.g., EF Core's EntityEntry). The approach is correct and the test coverage is excellent.
The Fix (MemberDiscovery.cs)
if (typeSymbol.TypeKind == TypeKind.Class
&& typeSymbol.FindImplementationForInterfaceMember(member) is not null)
{
continue;
}This is the right fix. FindImplementationForInterfaceMember is exactly the Roslyn API designed to answer "is this interface slot already satisfied by the class hierarchy?" Using it as a guard is semantically correct:
- Virtual/public implementations are already captured by
ProcessClassMembersearlier in the same method. - Explicit implementations are intentionally unreachable as
overridetargets. - Non-virtual concrete implementations are equally off-limits.
The ordering matters here — ProcessClassMembers runs before the interface loop, so any virtual member that satisfies an interface slot was already collected. This guard correctly prevents re-emitting it as a redundant (and broken) public override.
One worth considering: Does ProcessClassMembers walk the full inheritance chain (not just typeSymbol's own declared members), so that inherited virtual base-class implementations are collected before the interface loop skips them? If ProcessClassMembers is shallow (only typeSymbol.GetMembers(), not typeSymbol.BaseType.GetMembers() recursively), there's a latent risk for the pattern:
interface IFoo { void Foo(); }
class Base : IFoo { public virtual void Foo() { } }
class Derived : Base { } // mocking DerivedFindImplementationForInterfaceMember would find Base.Foo (non-null → skip in interface loop), but if ProcessClassMembers doesn't recurse into base classes, Foo is never collected at all. The runtime tests for KitchenSinkInheritanceTests cover part of this shape, so if those pass the hierarchy walk must be correct — but it's worth a comment or a dedicated test that verifies an inherited virtual that satisfies an interface can still be mocked on the derived type.
Comment Verbosity
The 5-line comment at the fix site is on the wordy side for inline code. The key invariant is:
For class mocks, virtual/abstract members were collected in the class walk. The interface loop should skip any slot already satisfied — explicit impls and non-virtuals can't be overridden anyway.
Consider trimming to the invariant + issue number (the PR/commit is the right place for the full story). That said, this is minor and shouldn't block merge.
Snapshot Tests
The CLAUDE.md rule requires committing .verified.txt snapshot files when source generator output changes. This fix changes which members get emitted for any class type with explicit interface implementations, which is a generator output change. No snapshot file updates appear in this PR.
If TUnit.Mocks.SourceGenerator.Tests has snapshot coverage for class mocks, those snapshots may need updating. The PR reports 45/45 passing there, which suggests either (a) the fix doesn't change the output for currently-snapshotted types, or (b) the snapshot tests don't yet cover the affected shape. Either way, it's worth adding a snapshot test for the HasExplicitInstance pattern introduced in KitchenSinkConcreteTests.cs so the generator output for this fix is locked in.
EF Core Dependency
Adding Microsoft.EntityFrameworkCore (net10.0-only) as a test dependency is a reasonable trade for a real-world regression guard. The local analog in KitchenSinkConcreteTests.cs (IHasHiddenInstance/HasExplicitInstance) already covers the general shape — the EF test is valuable specifically as a canary for future EF Core API changes. Scoping it to net10.0 is the right call since EF 10 is net10-only.
Test Coverage
The breadth of the new kitchen-sink tests is genuinely impressive: params, tuples, Func<,> returns, IAsyncEnumerable<T>, deeply-nested generics, nullable references, new-redeclared interface members, and multiple hierarchy shapes. These go well beyond what's strictly needed for the fix and meaningfully raise the regression bar for the mock generator.
Verdict
Approve with minor suggestions. The core fix is correct, necessary, and well-tested. The two items worth following up on are:
- Verify (or add a test for) the inherited-virtual-satisfies-interface shape to confirm
ProcessClassMembersrecurses through base types. - Consider adding a snapshot test for the explicit-interface-impl generator output to lock in the fix at the generator level.
Neither is a blocker.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 2 minor |
| CodeStyle | 1 minor |
🟢 Metrics 28 complexity
Metric Results Complexity 28
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…erage ShapeLeaf inherits from ShapeBase, which satisfies IInheritedShape via virtual members. Mocking ShapeLeaf must collect those inherited virtuals (proving ProcessClassMembers recurses through BaseType) AND the interface-loop guard must skip the duplicate interface-member emission (proving FindImplementationForInterfaceMember picks up the inherited impl). Inspection of the generated MockImpl confirms exactly one override per inherited member — no duplicates from the interface walk.
There was a problem hiding this comment.
Follow-up Review
Previous Review Issues — Status
Issue 1 (ProcessClassMembers recursion for inherited virtuals): ✅ Addressed
KitchenSinkInheritanceTests.cs now has a dedicated ShapeLeaf/ShapeBase hierarchy with the explicit test Leaf_Inherits_Base_Virtual_That_Satisfies_Interface. This confirms both that ProcessClassMembers recurses into base types AND that the interface-loop guard correctly skips the inherited slot without dropping the member from the mock. The test structure (unconfigured → calls base, configured → mock intercepts, cast-through-interface, event-raise, verification counts) is thorough and directly answers the concern from the prior review.
Issue 2 (Snapshot tests):
No .verified.txt files appear in this diff. CLAUDE.md requires committing snapshot files when source generator output changes. The fix to MemberDiscovery.cs changes what members are emitted for any class type with interface implementations, which is a generator output change.
The PR reports 45/45 in TUnit.Mocks.SourceGenerator.Tests, which means either (a) existing snapshots don't cover a class-with-interface-impl shape, or (b) the snapshot test runner regenerated without a diff. Either way, a generator-level snapshot for the HasExplicitInstance or ScopedLeaf shape would lock in exactly which members are (and aren't) emitted for this case. Without it, a future regression in the generator could silently re-emit the overriding member and only be caught at runtime.
Minor New Issues
Orphaned comment in KitchenSinkInheritanceTests.cs:
The first comment header has no test under it — the second header immediately follows. Either the first comment is a stale artifact or the Derived_Class_Inherits_Base_Explicit_Interface_Impl test (which actually is the #5673 shape) should follow the first header. This is cosmetic but slightly misleading.
Unused import in KitchenSinkInterfaceTests.cs:
None of the new code in this file requires System.Runtime.CompilerServices — the async IAsyncEnumerable local function doesn't need it, EnumeratorCancellation isn't used, and there are no [MethodImpl] or [CallerMemberName] usages. Looks like a stale import from an earlier iteration.
Verdict
The core fix is correct and unchanged from the prior review. The most significant remaining item is the missing snapshot coverage at the generator level (Issue 2). The new inheritance tests are a genuine addition that directly validate the fix's key invariant. The orphaned comment and unused import are trivial cleanup items.
Approve or merge at maintainer discretion — neither of the remaining items is a blocker for correctness.
There was a problem hiding this comment.
Follow-up Review
Previous Review Issues — Status
Issue 1 (ProcessClassMembers recursion for inherited virtuals): Addressed
KitchenSinkInheritanceTests.cs now has a dedicated ShapeLeaf/ShapeBase hierarchy with the explicit test Leaf_Inherits_Base_Virtual_That_Satisfies_Interface. This confirms both that ProcessClassMembers recurses into base types AND that the interface-loop guard correctly skips the inherited slot without dropping the member from the mock. The test structure (unconfigured → calls base, configured → mock intercepts, cast-through-interface, event-raise, verification counts) is thorough and directly answers the concern from the prior review.
Issue 2 (Snapshot tests): Still open
No .verified.txt files appear in this diff. CLAUDE.md requires committing snapshot files when source generator output changes. The fix to MemberDiscovery.cs changes what members are emitted for any class type with interface implementations, which is a generator output change.
The PR reports 45/45 in TUnit.Mocks.SourceGenerator.Tests, which means either (a) existing snapshots don't cover a class-with-interface-impl shape, or (b) the snapshot test runner regenerated without a diff. Either way, a generator-level snapshot for the HasExplicitInstance or ScopedLeaf shape would lock in exactly which members are (and aren't) emitted for this case. Without it, a future regression in the generator could silently re-emit the overriding member and only be caught at runtime.
Minor New Issues
Orphaned comment in KitchenSinkInheritanceTests.cs:
There are two consecutive section header comments with no test between them:
// -- Hierarchy where L1 explicitly implements interface (#5673 shape) --
// -- Base-virtual-satisfies-interface: leaf inherits, mock still overrides --
[Test]
public async Task Leaf_Inherits_Base_Virtual_That_Satisfies_Interface()
The first comment header has no test under it — the second header immediately follows. Either the first comment is a stale artifact or Derived_Class_Inherits_Base_Explicit_Interface_Impl (which actually covers the #5673 shape) should appear under the first header. Cosmetic only, but slightly misleading.
Unused import in KitchenSinkInterfaceTests.cs:
using System.Runtime.CompilerServices; was added but none of the new code requires it — the async IAsyncEnumerable local function doesn't need it, [EnumeratorCancellation] isn't used, and there are no [MethodImpl] or [CallerMemberName] usages. Looks like a stale import from an earlier iteration.
Verdict
The core fix is correct and unchanged from the prior review. The most significant remaining item is the missing snapshot coverage at the generator level (Issue 2 above). The new inheritance tests are a genuine addition that directly validate the fix's key invariant. The orphaned comment and unused import are trivial cleanup items.
Approve or merge at maintainer discretion — neither of the remaining items is a blocker for correctness.
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
…return fix) Adds KitchenSinkEdgeCasesTests exercising 15 previously-uncovered shapes: T1 DIM on interface with a class target that does not override T2 Abstract class satisfies interface via abstract method T3 Class re-implements interface on top of base's implicit impl T4 Four-level inheritance with interface at root T5 Write-only property T6 `abstract override` member T7 Two interfaces with same-name members and different return types T9 Nullable value-type parameter and return T10 IDisposable + IAsyncDisposable abstract class T11 Class with an explicit event + custom add/remove accessors T12 Inner class inheriting a closed generic T13 Derived interface with `new`-redeclared property T16 IAsyncEnumerable with CancellationToken parameter T19 Obsolete member T20 `ref readonly` return Every test pairs `Returns(...)` with `WasCalled`/verification so regressions surface whether they break call interception or call tracking. Generator fix for T20: ProcessClassMembers now classifies methods whose return is `ref` or `ref readonly` as non-mockable — the mock engine cannot route a ref return through its boxed-object dispatch, so the inherited impl flows through unchanged (test asserts this). Five shapes (T8, T14, T15, T17, T18) surfaced genuine generator gaps that are out of scope for the #5673 fix and are called out as SKIPPED in the file with tracking notes (self-referential IEquatable name collision, interface with indexer, static-abstract interface on a class, `required` members, and member names matching C# keywords).
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
There was a problem hiding this comment.
Follow-up Review (3rd pass)
Previous Issues — Status Check
Issue 1 (ProcessClassMembers recursion for inherited virtuals): ✅ Addressed — confirmed in prior review
Issue 2 (Snapshot tests):
No .verified.txt files appear in the diff (9 files changed, all additions). The fix to MemberDiscovery.cs changes generator output for any class type with explicit interface implementations — that is a source generator output change. CLAUDE.md explicitly requires committing snapshot files when generator output changes.
If TUnit.Mocks.SourceGenerator.Tests already covers class-with-interface-impl shapes, those snapshots need to reflect the fix. If they don't, a new snapshot test for the HasExplicitInstance or ScopedLeaf shape would lock in exactly which members are (and aren't) emitted — without it, a future generator regression could silently re-emit the problematic public override and only surface at runtime compile time.
Orphaned comment in KitchenSinkInheritanceTests.cs:
Two consecutive section headers with no test between them:
// ── Hierarchy where L1 explicitly implements interface (#5673 shape) ──
// ── Base-virtual-satisfies-interface: leaf inherits, mock still overrides ──
[Test]
public async Task Leaf_Inherits_Base_Virtual_That_Satisfies_Interface()The first header either needs a test or should be removed — it's misleading as written.
Unused import in KitchenSinkInterfaceTests.cs:
using System.Runtime.CompilerServices; was added but nothing in the file uses it. Easy one-liner cleanup.
Verdict
The core fix remains correct, necessary, and well-tested. The snapshot coverage gap is the only substantive open item — the orphaned comment and unused import are trivial. All three have been flagged in both previous reviews without change.
Ready to approve once the snapshot concern is resolved (or explicitly documented why no snapshot update is needed for this generator change).
Default interface methods require runtime support not present in .NET Framework (net472) — declaration produces CS8701. Wrap both the DIM in IKitchenSink (DefaultGreet) and the T1 IHasDim/ClassNoDimOverride pair, along with their matching tests, in #if NET8_0_OR_GREATER so the multi-targeted test project builds on net472.
There was a problem hiding this comment.
Follow-up Review (4th pass)
New Commit: 75aea8f5
test(mocks): guard DIM patterns with NET8_0_OR_GREATER — wraps IHasDim, ClassNoDimOverride, IKitchenSink.DefaultGreet, and their tests in #if NET8_0_OR_GREATER to fix the multi-targeted build on net472. This is correct and necessary.
Previous Issues — Status
Issue 1 (ProcessClassMembers hierarchy recursion): ✅ Resolved — confirmed in 2nd/3rd pass reviews.
Issue 2 (Snapshot tests):
No .verified.txt files in the diff. MemberDiscovery.cs changes which members are emitted for class types with explicit interface implementations — that is a generator output change covered by CLAUDE.md's snapshot rule. The project reports 45/45 in TUnit.Mocks.SourceGenerator.Tests, but without a snapshot for the HasExplicitInstance or ScopedLeaf shape, a future regression could silently re-emit the public override and only be caught at runtime. Worth at least confirming why no snapshot update was needed (e.g., if the snapshot test suite doesn't cover this shape yet, a new snapshot would lock in the fix).
Orphaned comment in KitchenSinkInheritanceTests.cs:
The first header comment sits directly above the second with no test between them. Either remove the first header or reorder the tests so Derived_Class_Inherits_Base_Explicit_Interface_Impl (which is the actual #5673 shape) follows it.
Unused import in KitchenSinkInterfaceTests.cs:
using System.Runtime.CompilerServices; added in the original diff; nothing in the file uses it.
Verdict
The DIM guard in the latest commit is correct. The core fix remains sound and well-tested. The snapshot gap is the only substantive open item — the orphaned comment and unused import are trivial one-liner cleanups. All three have now been flagged across four reviews; happy to approve once addressed (or once the snapshot behaviour is explained).
There was a problem hiding this comment.
Follow-up Review (4th pass)
New Commit: 75aea8f5
test(mocks): guard DIM patterns with NET8_0_OR_GREATER — wraps IHasDim, ClassNoDimOverride, IKitchenSink.DefaultGreet, and their tests in #if NET8_0_OR_GREATER to fix the multi-targeted build on net472. This is correct and necessary.
Previous Issues — Status
Issue 1 (ProcessClassMembers hierarchy recursion): ✅ Resolved — confirmed in 2nd/3rd pass reviews.
Issue 2 (Snapshot tests):
No .verified.txt files in the diff. MemberDiscovery.cs changes which members are emitted for class types with explicit interface implementations — that is a generator output change covered by CLAUDE.md's snapshot rule. The project reports 45/45 in TUnit.Mocks.SourceGenerator.Tests, but without a snapshot for the HasExplicitInstance or ScopedLeaf shape, a future regression could silently re-emit the public override and only be caught at runtime. Worth at least confirming why no snapshot update was needed (e.g., if the snapshot test suite doesn't cover this shape yet, a new snapshot would lock in the fix).
Orphaned comment in KitchenSinkInheritanceTests.cs:
Two consecutive section header comments with no test between them — the first header has no matching test directly below it. Either remove the first header or reorder so Derived_Class_Inherits_Base_Explicit_Interface_Impl (the actual #5673 shape) follows it.
Unused import in KitchenSinkInterfaceTests.cs:
using System.Runtime.CompilerServices; was added in the original diff; nothing in the file uses it after the DIM guard wrapping.
Verdict
The DIM guard in the latest commit is correct. The core fix remains sound and well-tested. The snapshot gap is the only substantive open item — the orphaned comment and unused import are trivial one-liner cleanups. All three have been flagged across four reviews; happy to approve once addressed (or once the snapshot behaviour is explained).
1 participant
Summary
Microsoft.EntityFrameworkCore.ChangeTracking.EntityEntry(and similar types with an explicit interface impl) failed to compile withCS0115: 'EntityEntryMockImpl.Instance': no suitable method found to override.AllInterfacesfor class partial mocks and re-emitted every interface member aspublic override, which fails when the base class's impl is explicit (or non-virtual) — there's nothing to override.EntityEntryimplementsIInfrastructure<InternalEntityEntry>.Instanceexplicitly.typeSymbol.FindImplementationForInterfaceMember(member)is non-null. The inherited impl already satisfies the interface; the mock only overrides what the class walk collected (virtual/abstract/override).Test coverage
Issue5673Testscompiles against EF Core 10 (ref is conditional onnet10.0since EF 10 is net10-only) and exercisesEntityEntry.Mock(...)at runtime.new-redeclared memberparamsarrays, tuple returns, delegate (Func<,>) returnsIAsyncEnumerable<T>returnsDictionary<TKey, List<TValue>>)IsNull<T>()Returns()-driven test ANDWasCalled/WasNeverCalledverification so regressions surface whether they break value interception or call tracking.Test plan
dotnet runinTUnit.Mocks.Tests(net10.0) — 936/936 passdotnet runinTUnit.Mocks.SourceGenerator.Tests(net10.0) — 45/45 passdotnet runinTUnit.Mocks.Analyzers.Tests(net10.0) — 30/30 passIssue5673Tests.csreproduces the original CS0115