Skip to content

Comments

feat(mocks): unified Mock<T> API — remove Setup/Verify/Raise surfaces#4991

Merged
thomhurst merged 17 commits intomainfrom
feat/unified-mock-api
Feb 24, 2026
Merged

feat(mocks): unified Mock<T> API — remove Setup/Verify/Raise surfaces#4991
thomhurst merged 17 commits intomainfrom
feat/unified-mock-api

Conversation

@thomhurst
Copy link
Owner

Summary

  • Remove .Setup., .Verify., and .Raise. intermediate properties from Mock<T>. Methods are now called directly on the mock via source-generated extension methods, with chain methods (.Returns() vs .WasCalled()) disambiguating between setup and verification.
  • Add unified return types (MockMethodCall<T>, VoidMockMethodCall, PropertyMockCall<T>, PropertySetterMockCall<T>) that expose both setup and verification surfaces on a single object.
  • Replace MockSetupBuilder + MockVerifyBuilder with a single MockMembersBuilder that generates extension methods on Mock<T> and unified sealed classes per qualifying method.
  • Void methods and property setters eagerly register their setups in the constructor, since they're commonly used without chaining (e.g., mock.Log(Arg.Any<string>()) to allow calls in strict mode).

Before

mock.Setup.Greet(Arg.Any<string>()).Returns("Hello");
mock.Verify.Greet(Arg.Any<string>()).WasCalled();
mock.Raise.OnMessage("hi");

After

mock.Greet(Arg.Any<string>()).Returns("Hello");
mock.Greet(Arg.Any<string>()).WasCalled();
mock.RaiseOnMessage("hi");

Test plan

  • All 475 runtime tests pass (TUnit.Mocks.Tests on net10.0)
  • All 10 source generator snapshot tests pass (TUnit.Mocks.SourceGenerator.Tests)
  • Snapshot .verified.txt files updated to reflect new generated output
  • Verify CI passes across all target frameworks (net8.0, net9.0, net10.0)

…ed types

These runtime types serve as unified return types that support both setup
and verification on a single object. They use lazy setup registration --
the MethodSetup is only created and registered in the engine when a setup
method (Returns, Callback, etc.) is called, not when the mock method
itself is called.
…ll<TProperty> unified types

Merges PropertySetupAccessor<TProperty> and PropertyVerifyAccessor<TProperty> into
a single PropertyMockCall<TProperty> that exposes both setup and verify surfaces.
Adds PropertySetterMockCall<TProperty> for setter-specific setup and verification
with argument matchers. Old types remain for now and will be deleted in a later task.
Remove the IMockSetup<T>, IMockVerify<T>, IMockRaise<T> marker interfaces
and the Setup/Verify/Raise properties from Mock<T>. Replace the two
constructors with a single (T, MockEngine<T>) constructor. Update InState
to accept Action<Mock<T>> instead of Action<IMockSetup<T>>.

Delete PropertySetupAccessor and PropertyVerifyAccessor, which are
replaced by the new PropertyMockCall unified type.

Source-generated extension methods will target Mock<T> directly instead
of going through marker interface indirection.
…d MockMembersBuilder

The new MockMembersBuilder generates a single _MockMembers.g.cs file per
mocked type, replacing the separate _MockSetup.g.cs and _MockVerify.g.cs
files. Key changes:

- Extension methods target Mock<T> directly instead of IMockSetup<T>
- No more holder classes (_MockSetup, _MockVerify) — engine accessed via mock.Engine
- Typed wrappers are now sealed classes (not structs) with lazy setup registration
- Each wrapper implements ICallVerification for unified setup + verify
- Non-qualifying methods return MockMethodCall<T> or VoidMockMethodCall base types
- Member name conflict detection appends '_' for Mock<T> built-in names
- Property extensions use PropertyMockCall<T> targeting Mock<T>
…k<T> constructor

- MockRaiseBuilder: Remove _MockRaise holder class and IMockRaise<T> marker
  interface. Extension methods now target Mock<T> directly with Raise prefix
  (e.g., RaiseOnMessage). Event raising goes through IRaisable interface.
- MockFactoryBuilder: Remove creation of setup/verify/raise intermediary
  objects from all three factory methods. Use simplified Mock<T>(impl, engine)
  constructor.
- MockDelegateFactoryBuilder: Same simplification - remove setup/verify
  creation, use Mock<T>(del, engine) constructor.
Remove .Setup., .Verify., and .Raise. accessor patterns from all 43
test files in TUnit.Mocks.Tests/ to match the new unified API where
mock members are accessed directly on Mock<T>. Also update InState
lambda parameters from 'setup' to 'm'.
Void methods and property setters are commonly used without chaining
(e.g., mock.Log(Arg.Any<string>()) to "allow" the call in strict mode).
Eagerly call EnsureSetup() in constructors so the setup is registered
even when no chain method is called. Also fix PropertySetterMockCall
to properly delegate to VoidMethodSetupBuilder and update all snapshot
tests to reflect the new generated output.
Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Unified Mock API — Remove Setup/Verify/Raise Surfaces

This is a well-motivated refactor. The before/after in the description shows a real ergonomic improvement, and the architecture is cleaner overall. A few issues worth discussing:


Issue 1 (Design): Thread-Safety Race in EnsureSetup() for Non-Void Methods

MockMethodCall.cs and all generated *_MockCall classes for non-void methods:

private MethodSetupBuilder<TReturn> EnsureSetup()
{
    if (_builder is null)   // Thread A and Thread B can both see null here
    {
        var setup = new MethodSetup(_memberId, _matchers, _memberName);
        _engine.AddSetup(setup);       // Called twice — double-registration
        _builder = new MethodSetupBuilder<TReturn>(setup);
    }
    return _builder;
}

This check-then-act pattern is a data race. Under concurrent setup calls, two threads can both pass the null check, both call _engine.AddSetup(), and both assign _builder. The result is a double-registered MethodSetup for the same mock member, with one orphaned in the engine.

A safer approach would be Lazy<T> initialization or Interlocked.CompareExchange. Since TUnit has ThreadSafetyTests.cs, this may already be tested — but in that case, the test should be validating the engine-side behavior as well.

Why this matters: Even if tests are sequential, the VoidMockMethodCall is the standard pattern for "allow in strict mode" where the same mock object might be shared across parallel test fixtures via [ClassDataSource].


Issue 2 (Design): Setup Registration Asymmetry Between Void and Non-Void

VoidMockMethodCall calls EnsureSetup() eagerly in the constructor, while MockMethodCall<TReturn> defers. The comment explains the intent, but it creates a subtle inconsistency:

// This DOES register a setup (void — eager):
mock.Log(Arg.Any<string>());

// This does NOT register a setup (non-void — lazy):
mock.GetValue(Arg.Any<string>());

A user who writes mock.GetValue(Arg.Any<string>()) without chaining .Returns() gets a no-op MockMethodCall<string> that's immediately garbage collected without side effects. That's actually the correct behavior in loose mode — but in strict mode it could produce a confusing "unexpected call" failure that doesn't match the mental model of "I called mock.GetValue to allow it."

Consider at minimum adding an XML doc note on MockMethodCall<TReturn> that explains this distinction. Or, if the intent is specifically to match void-style usage in strict mode, providing a .Allow() method that eagerly registers a permissive setup (similar to how VoidMockMethodCall works) would be more explicit than relying on the void/non-void asymmetry.


Issue 3 (Design): WasCalled() Creates a New Verification Object on Every Invocation

public void WasCalled() => _engine.CreateVerification(_memberId, _memberName, _matchers).WasCalled();
public void WasCalled(Times times) => _engine.CreateVerification(_memberId, _memberName, _matchers).WasCalled(times);
// ... repeated for every overload

Each WasCalled() call instantiates a fresh CallVerificationBuilder that's used once and discarded. This is a one-shot pattern and is functionally fine, but it means the matchers array is passed through the allocation on each call. Since the matchers are already stored on the MockMethodCall, consider caching the verification builder on first creation (similar to EnsureSetup()) rather than recreating it each time. This would also reduce object graph depth during verification failures.


Issue 4 (Maintenance): MockMemberNames Hardcoded List in MockMembersBuilder

private static readonly HashSet<string> MockMemberNames = new(System.StringComparer.Ordinal)
{
    "Object", "Engine", "Behavior", "Invocations", "DefaultValueProvider",
    "SetupAllProperties", "Reset", "VerifyAll", "VerifyNoOtherCalls",
    "GetAutoMock", "GetDiagnostics", "SetState", "InState",
    "GetHashCode", "GetType", "ToString", "Equals"
};

This is fragile — it must be manually kept in sync with Mock<T>'s public API. If a new method is added to Mock<T>, it won't be added here, silently generating a collision. A better approach would be to derive this list from the Mock<T> type at generator initialization time using symbol analysis (the generator already has access to the compilation, so typeof(Mock<T>) members can be enumerated at source-gen time). This would make it impossible for the list to go stale.


Issue 5 (Minor): PropertyMockCall<T>.Getter Creates a New Setup on Every Access

public IPropertySetup<TProperty> Getter
{
    get
    {
        var methodSetup = new MethodSetup(_getterMemberId, matchers, $"{_propertyName} (get)");
        _engine.AddSetup(methodSetup);   // New registration each access
        return new PropertySetupBuilder<TProperty>(methodSetup);
    }
}

This was the same behavior in PropertySetupAccessor<T>, so it's not a regression. But since PropertyMockCall<T> now also owns verification, the multiple-access concern is more visible. Worth a comment explaining the deliberate re-registration behavior (or raising it as a separate follow-up).


What's Good

  • The API simplification is genuinely better. mock.Greet(Arg.Any<string>()).Returns("Hello") is more natural than mock.Setup.Greet(Arg.Any<string>()).Returns("Hello").
  • Removing the IMockSetup<T> / IMockVerify<T> marker interfaces removes an entire layer of indirection including the unsafe downcasts ((ISomethingMockSetup)setup) that existed in every generated extension method.
  • MockOfT.cs is notably simpler — removing Setup, Verify, and Raise properties reduces the public surface of Mock<T>.
  • MockMembersBuilder.cs replacing two separate builders reduces generated file count (one _MockMembers.g.cs instead of _MockSetup.g.cs + _MockVerify.g.cs).
  • The GetSafeMemberName escape hatch for name collisions is a necessary correctness fix.
  • Snapshot tests are updated and committed — good hygiene per project rules.

Remove all references to the old .Setup., .Verify., and .Raise.
intermediate properties. Update all code examples to use the new
unified API where methods are called directly on Mock<T> and the
chain method disambiguates between setup and verification. Also
update XML doc comments in PropertyMockCall and MockAssertionExtensions
to reference current type names.
Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: feat(mocks): unified Mock API — remove Setup/Verify/Raise surfaces

Summary

This is a solid API simplification. Removing the .Setup., .Verify., and .Raise. intermediate properties reduces ceremony significantly and the fluent chaining approach is much more ergonomic. The overall design direction is correct. That said, there are several issues worth addressing.


Issues Found

1. PropertyMockCall<T>.Getter registers a new setup on every access — potential bug

In PropertyMockCall.cs, the Getter property creates and registers a brand-new MethodSetup on every single access:

public IPropertySetup<TProperty> Getter
{
    get
    {
        var methodSetup = new MethodSetup(_getterMemberId, matchers, $"{_propertyName} (get)");
        _engine.AddSetup(methodSetup);  // New setup registered every time!
        return new PropertySetupBuilder<TProperty>(methodSetup);
    }
}

This means:

mock.Name.Returns("Alice");  // Accesses Getter internally → registers setup #1
mock.Name.Returns("Bob");    // Accesses Getter again → registers setup #2 (!!)

It also means VerifyAll() tracks two separate setups and both must be invoked. The lazy EnsureSetup() pattern used in MockMethodCall<T> and VoidMockMethodCall is the right approach here too. The PropertyMockCall<T> should lazily cache its getter setup builder rather than registering fresh on each access.

2. Void verification has a hidden setup side effect — breaks VerifyAll() semantics

VoidMockMethodCall eagerly registers a setup in its constructor. This means calling a method for the purpose of verification also registers a setup:

// Old (verify only, no setup registered):
mock.Verify.Log(Arg.Any<string>()).WasCalled();

// New (verification + inadvertent setup registration):
mock.Log(Arg.Any<string>()).WasCalled();

After migrating to the new API, code that previously called VerifyAll() after doing verification-only calls will now see those verification calls counted as uninvoked setups and fail. The PR description mentions the void eager-registration is for strict mode compatibility (mock.Log(Arg.Any<string>()) as an "allow" statement), but the side effect on VerifyAll() is a behavioral regression. Consider either:

  • Not eagerly registering until a setup method (.Callback(), .Throws()) is explicitly called
  • Tracking setups vs. verifications separately in the engine

3. Allocation regression: readonly structsealed class

The previous *_TypedSetup types were readonly struct, meaning zero heap allocations for the setup-chain wrapper. All the new unified types (MockMethodCall<T>, VoidMockMethodCall, and every generated *_MockCall) are sealed class — one heap allocation per mock method invocation.

In tight loops or assertion-heavy tests, this creates measurable GC pressure. The CLAUDE.md "Performance First" principle flags this. The original structs were allocation-free for the common case; the new design trades that away for API simplicity. This may be acceptable, but it should be a deliberate decision rather than an implicit one.

If allocation matters here, consider a different split: keep the generated per-method wrapper types as readonly struct for the common path (setup+chain), and only allocate a verify object on-demand when .WasCalled() is called.

4. Member name mismatch between setup and verification in PropertyMockCall<T>

Setup registers with name $"{_propertyName} (get)" (e.g., "Name (get)"), but verification uses $"get_{_propertyName}" (e.g., "get_Name"). If the engine uses member names in error messages or to correlate setup/call records, these will be inconsistent:

// PropertyMockCall.cs — setup:
var methodSetup = new MethodSetup(_getterMemberId, matchers, $"{_propertyName} (get)");

// PropertyMockCall.cs — verify:
_engine.CreateVerification(_getterMemberId, $"get_{_propertyName}", ...).WasCalled(times);

These names should be unified. Since matching is probably done by _getterMemberId, this may only affect error messages, but even that inconsistency is confusing when debugging test failures.

5. Non-void standalone calls are silently a no-op

The asymmetry between void and non-void is non-obvious:

mock.Log("x");                 // void → setup is registered (strict mode compatible)
mock.GetUser(Arg.Any<int>()); // non-void → nothing happens (no setup, no verify)

There is no compiler warning or runtime diagnostic when mock.GetUser(Arg.Any<int>()) appears alone. This could silently mask developer mistakes (e.g., forgetting to chain .WasCalled() or .Returns()). A Roslyn analyzer that warns on unconsumed MockMethodCall<T> return values (similar to [MustUseReturnValue]) would significantly improve discoverability. It would be worth either adding that analyzer or documenting the asymmetry prominently.


Minor Notes

  • The Raises method on ISetupChain<TReturn> takes a string event name ("ItemAdded"), which is stringly typed. The old typed raise extensions (mock.Raise.ItemAdded(e)) were compile-time safe. The new mock.RaisesItemAdded(e) generated overloads restore type safety for the Raises shortcut on the setup chain, which is good.

  • The removal of IMockSetup<T> and IMockVerify<T> as separate extension targets simplifies the source generator considerably. The MockMembersBuilder replacing two separate builders is cleaner.

  • MockOfT.cs constructor simplification (removing setup/verify/raise constructor parameters) is clean and correct.

  • The snapshot .verified.txt files are appropriately updated and confirm the generator output is working correctly.


Overall

The API direction is right — unified call objects that support both setup and verification are more ergonomic. The Getter-side-effect bug and the VerifyAll() behavioral regression are the highest-priority issues to address before merging.

…d calls

Address code review Issue 1: the check-then-act pattern in EnsureSetup()
for non-void MockMethodCall<TReturn> and generated non-void sealed classes
was a data race under concurrent access. Replace the nullable field with
Lazy<T> which guarantees single initialization.

Void methods (VoidMockMethodCall, generated void classes) and property
setters (PropertySetterMockCall) are unaffected — they eagerly initialize
in the constructor before the object is visible to other threads.

Also add clarifying doc comments for:
- Issue 2: void vs non-void registration asymmetry
- Issue 5: PropertyMockCall.Getter deliberate re-registration
@thomhurst
Copy link
Owner Author

Addressed in f225a2c:

Issue 1 — Thread-safety race in EnsureSetup(): Fixed. Non-void MockMethodCall<TReturn> and generated non-void sealed classes now use Lazy<T> for thread-safe single initialization. Void methods and property setters were already safe (eager init in the constructor completes before the object is visible to other threads).

Issue 2 — Void vs non-void asymmetry: Added XML doc comment on MockMethodCall<TReturn> explaining the distinction and suggesting .Returns(default!) as the explicit escape hatch for non-void methods in strict mode.

Issue 3 — Verification object per call: Skipping. These MockMethodCall instances are ephemeral per-call objects, so the verification builder is equally short-lived. Caching adds complexity for no measurable benefit in test code.

Issue 4 — Hardcoded MockMemberNames: Acknowledged as a maintenance concern. Deriving from Mock<T> symbol analysis at generator time is doable but non-trivial (the generator runs against the user's compilation, not TUnit.Mocks itself). Tracking as a follow-up.

Issue 5 — Getter re-registration: Added clarifying comment explaining this is intentional — each access allows chaining different behaviors that stack as additional setups.

All 475 runtime tests and 10 snapshot tests pass.

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Follow-up on feat(mocks): unified Mock API

This review follows the two previous Claude reviews submitted before commit f225a2c9. Good news: the thread-safety fix in that last commit (Lazy<T> for non-void setup registration) is the right approach and directly addresses the previous concern.


What Was Addressed Since Last Review

Thread safety (non-void path) — fixed correctly. MockMethodCall<T> and the generated *_MockCall types for methods with return values now use Lazy<MethodSetupBuilder<T>> for deferred, thread-safe setup creation. This is consistent and correct.


Remaining Concerns

1. PropertyMockCall.Setter has an uncontrolled multiple-registration bug (higher severity than Getter)

The Getter property registering a new setup on each access was flagged previously and has since been documented as intentional (supports sequential behavior chaining). That reasoning is defensible. However, the Setter property has the same pattern but with eager registration via VoidMockMethodCall, which makes it a genuine bug rather than a feature:

// In PropertyMockCall<T>:
public VoidMockMethodCall Setter
{
    get => new VoidMockMethodCall(_engine, _setterMemberId, $"{_propertyName} (set)",
               Array.Empty<IArgumentMatcher>());
    // ↑ Every access constructs a new VoidMockMethodCall, which eagerly calls EnsureSetup()
    // and registers a NEW MethodSetup in the engine
}

This means:

mock.Count.Setter.WasCalled(Times.Exactly(3));  // ← registers setup #1 as side effect
mock.Count.Setter.WasCalled(Times.Exactly(3));  // ← registers setup #2 as side effect

mock.VerifyAll();  // ← fails: setup #2 was never invoked

Unlike Getter, Setter eagerly registers, so accessing it once purely for verification still produces an uninvoked setup in the engine. The Getter case at least requires calling .Returns() before it registers.

The fix is the same as the Getter comment suggests for the lazy case: make Setter return a cached instance per PropertyMockCall, or defer registration until a setup chain method is explicitly called. Since PropertyMockCall<T> is a readonly struct recreated on each property access, the caching would need to live elsewhere (e.g., in the engine keyed by member ID, or via a different design for Setter).

2. Member name inconsistency between setup registration and verification in PropertyMockCall<T>

The setup side registers names with one convention and the verification side uses a different one:

// Setup (via Getter):
new MethodSetup(_getterMemberId, matchers, $"{_propertyName} (get)")  // → "Name (get)"

// Verification (via WasCalled):
_engine.CreateVerification(_getterMemberId, $"get_{_propertyName}", ...)  // → "get_Name"

// Setup (via PropertySetterMockCall):
new MethodSetup(_setterMemberId, matchers, $"{_propertyName} (set)")  // → "Name (set)"

// Verification (via PropertySetterMockCall.WasCalled):
_engine.CreateVerification(_setterMemberId, $"set_{_propertyName}", ...)  // → "set_Name"

The mock implementation itself uses "get_Name" / "set_Name" in call records (consistent with the C# convention). Error messages during setup failures will show "Name (get)" while error messages from verification failures will show "get_Name" — these two names appearing in the same diagnostic context when debugging a test failure is confusing. Unify to "get_{_propertyName}" / "set_{_propertyName}" throughout.

3. VoidMockMethodCall thread safety in EnsureSetup (minor)

The void path uses a manual null-check pattern:

private VoidMethodSetupBuilder EnsureSetup()
{
    if (_builder is null)  // ← not thread-safe (no Interlocked/volatile)
    {
        var setup = new MethodSetup(...);
        _engine.AddSetup(setup);
        _builder = new VoidMethodSetupBuilder(setup);
    }
    return _builder;
}

In practice this is harmless because the constructor calls EnsureSetup() eagerly and _builder is never null by the time any external caller invokes it. But it's inconsistent with the non-void path's Lazy<T> approach and the null-check will confuse readers who wonder if there's a valid uninitialized state. Consider replacing with Lazy<VoidMethodSetupBuilder> for consistency, or remove the null check and initialize _builder directly in the constructor.


Observations (no action required)

  • C# 14 extension blocks for properties — using extension(Mock<T> mock) blocks to generate extension properties is clean and future-proof. Worth documenting that this requires LangVersion = preview / net10.0 for anyone reading the generated code.
  • PropertyMockCall<T> stays as readonly struct — good. The struct is stack-allocated per access and the reference to the engine is stable. The allocation concern from the previous review applies mainly to MockMethodCall<T> and VoidMockMethodCall, which are now sealed class by design.
  • MockOfT.cs constructor cleanup — the removal of setup/verify/raise parameters and the simplification to Mock(T mockObject, MockEngine<T> engine) is clean and correct.

Summary

The last commit (f225a2c9) correctly resolved the non-void thread-safety concern. The highest-priority remaining issue is the Setter property registering a new setup on every access — this is behaviorally different from the intentional Getter behavior because of eager registration, and will cause VerifyAll() failures for any test that accesses Setter more than once. The member name inconsistency is a secondary concern that will create confusing error messages in failure scenarios.

…on, Lazy<T>

1. Unify property member names between setup and verification paths:
   setup now uses "get_Name"/"set_Name" (was "Name (get)"/"Name (set)")
   to match verification, which already used "get_Name"/"set_Name".

2. PropertyMockCall.Setter now creates VoidMockMethodCall with
   eagerRegister: false, avoiding unwanted setup registration when
   accessed purely for verification (.WasCalled()). Setup methods
   (.Callback(), .Throws()) still trigger registration on demand.

3. VoidMockMethodCall now uses Lazy<T> for consistency with the
   non-void MockMethodCall<TReturn> pattern, with an internal
   eagerRegister parameter that forces evaluation in the constructor
   when true (default for extension methods on Mock<T>).
@thomhurst
Copy link
Owner Author

Addressed in f5f06f4 (responding to reviews 2 and 3):

Review 2 Issue 1 (Getter re-registration): This is pre-existing behavior from PropertySetupAccessor. Each access intentionally allows stacking setups (last wins). Already documented in prior commit.

Review 2 Issue 2 (Void eager + VerifyAll): The specific edge case is mock.Log(Arg.Any<string>()).WasNeverCalled() followed by mock.VerifyAll() — the eager registration creates a setup that was never invoked. In practice, if WasCalled() passes, the setup was also invoked, so VerifyAll() agrees. The WasNeverCalled() + VerifyAll() combo is the real concern, but no existing test hits this pattern (475/475 pass). A proper fix requires the engine to distinguish setup-registrations from verification-only objects — tracking as a follow-up.

Review 2 Issue 3 (Allocation regression): Deliberate. These are test-time ephemeral objects, not engine hot paths. The readonly structsealed class change was required for Lazy<T> and mutable builder fields.

Review 2 Issue 4 / Review 3 Issue 2 (Member name inconsistency): Fixed. Setup paths now use "get_Name" / "set_Name" matching the verification paths and the engine's call records.

Review 2 Issue 5 (Non-void silent no-op): Already documented. A [MustUseReturnValue]-style Roslyn analyzer is a good follow-up idea.

Review 3 Issue 1 (Setter re-registration): Fixed. PropertyMockCall.Setter now creates VoidMockMethodCall with eagerRegister: false. Accessing .Setter for verification no longer registers a setup. Setup methods (.Callback(), .Throws()) still trigger registration on demand.

Review 3 Issue 3 (VoidMockMethodCall consistency): Fixed. Now uses Lazy<T> matching the non-void pattern, with an eagerRegister parameter that forces evaluation in the constructor when true.

All 475 runtime tests and 10 snapshot tests pass.

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Follow-up after f5f06f4

This is the fourth review pass on this PR. The previous three rounds of review (pre-f225a2c9 and pre-f5f06f4) surface-addressed thread safety and several design concerns. Here is where things stand now.


What Has Been Addressed

  • Thread safety (non-void): MockMethodCall<TReturn> and generated non-void *_MockCall classes now use Lazy<T>, which is correct.
  • Thread safety (void): VoidMockMethodCall uses Lazy<T> with eager initialization — also correct. The _lazyBuilder.Value call in the constructor ensures the setup is registered on construction before the object is visible to any other thread.
  • PropertyMockCall<T>.Getter re-registration: Documented as intentional ("last wins" stacking). Acceptable given the prior-art in PropertySetupAccessor.
  • Setter deferred registration: PropertyMockCall<T>.Setter now uses eagerRegister: false, and PropertySetterMockCall<T> handles its own eager registration. Consistent.

Remaining Issue 1: PropertySetterMockCall<T>.EnsureSetup() is inconsistent with the Lazy<T> pattern

VoidMockMethodCall and all non-void *_MockCall classes use Lazy<T> for safe lazy/eager initialization. PropertySetterMockCall<T> still uses a manual null-check:

private VoidMethodSetupBuilder EnsureSetup()
{
    if (_builder is null)
    {
        var matchers = new IArgumentMatcher[] { _matcher };
        var methodSetup = new MethodSetup(_setterMemberId, matchers, $"set_{_propertyName}");
        _engine.AddSetup(methodSetup);
        _builder = new VoidMethodSetupBuilder(methodSetup);
    }
    return _builder;
}

In practice, EnsureSetup() is called eagerly in the constructor so _builder is always non-null after construction. The null-check is unreachable in normal usage. But it's an inconsistency that could become a real race if someone subclasses or calls from a context where the object is visible before the constructor finishes. Recommend aligning with Lazy<T> for consistency:

private readonly Lazy<VoidMethodSetupBuilder> _lazyBuilder;

public PropertySetterMockCall(...) 
{
    ...
    _lazyBuilder = new Lazy<VoidMethodSetupBuilder>(() => {
        var matchers = new IArgumentMatcher[] { _matcher };
        var setup = new MethodSetup(_setterMemberId, matchers, $"set_{_propertyName}");
        _engine.AddSetup(setup);
        return new VoidMethodSetupBuilder(setup);
    });
    _ = _lazyBuilder.Value; // eager
}

Remaining Issue 2: VerifyAll + Void Eager Registration Semantic Conflict

This was raised in the previous review and thomhurst's response was truncated. The concern: when a user writes verification-only code for void methods:

mock.Log(Arg.Any<string>()).WasNeverCalled();

VoidMockMethodCall eagerly registers a MethodSetup before the user even calls .WasNeverCalled(). If the test then calls mock.VerifyAll(), the engine finds a registered setup for Log that was never invoked — and depending on how VerifyAll is implemented, this could produce a false failure ("Log was set up but never called").

The root issue is that eager void registration is a side effect of constructing the unified object, not of explicitly calling a setup method. Setup and verification now share a constructor, so it's impossible for the engine to know whether the user intends to set up or verify at construction time.

Suggested approach: Rather than eager registration at construction time, consider registering the setup only when a chain method with setup semantics is called, and verifying through CreateVerification for all WasCalled/WasNeverCalled paths. Strict mode compatibility (allowing the call without chaining) could be handled by a separate Allow() method:

mock.Log(Arg.Any<string>());           // lazy — does nothing yet
mock.Log(Arg.Any<string>()).Allow();    // explicit: "allow this call in strict mode"
mock.Log(Arg.Any<string>()).WasCalled(); // verify only — no setup registered

This is admittedly a larger API change, but it eliminates the ambiguity entirely. If that's too invasive, at minimum the documentation should warn about the VerifyAll interaction when using void methods for verification only.


Minor Issue 3: Lazy<T> Default Thread-Safety Mode

Lazy<T> without a constructor argument uses LazyThreadSafetyMode.ExecutionAndPublication, which acquires a lock on every value access until the value is initialized. Since most mock setups are single-threaded and the lock is held during _engine.AddSetup() (which may itself be thread-safe), this is fine but slightly heavier than necessary.

LazyThreadSafetyMode.PublicationOnly is a better fit here — it allows multiple threads to compute the value but only one registration "wins". However, since AddSetup has side effects (registering a MethodSetup), PublicationOnly could still cause double-registration races. The current ExecutionAndPublication is technically the safer choice. No change required — this is informational.


Minor Issue 4: Generated WasCalled Overloads — Repeated CreateVerification() Calls

In every generated *_MockCall class and in MockMethodCall<T> / VoidMockMethodCall, each WasCalled overload calls _engine.CreateVerification(...) independently:

public void WasCalled() => _engine.CreateVerification(_memberId, _memberName, _matchers).WasCalled();
public void WasCalled(Times times) => _engine.CreateVerification(_memberId, _memberName, _matchers).WasCalled(times);
public void WasCalled(Times times, string? message) => _engine.CreateVerification(_memberId, _memberName, _matchers).WasCalled(times, message);
public void WasCalled(string? message) => _engine.CreateVerification(_memberId, _memberName, _matchers).WasCalled(message);

Since only one of these is called per verification, this is not a runtime performance concern. But extracting a private ICallVerification BuildVerification() => _engine.CreateVerification(...) helper would reduce duplication and make it easier to spot if the arguments ever diverge. Low priority.


Overall Assessment

The API direction is sound — unified mock.Method().Returns() / mock.Method().WasCalled() is genuinely more ergonomic than mock.Setup.Method() / mock.Verify.Method(). The thread-safety work from previous review iterations is solid.

The two issues worth resolving before merge:

  1. PropertySetterMockCall.EnsureSetup() alignment (minor, low risk but inconsistent with the rest of the codebase)
  2. VerifyAll + void eager registration (needs confirmation that this is tested and documented if it's an accepted limitation)

The rest is polish.

Align PropertySetterMockCall<T> with VoidMockMethodCall and
MockMethodCall<TReturn> by replacing the manual null-check pattern
with Lazy<T> for thread-safe setup registration.
This was referenced Feb 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant