Fix chained mock setup behavior by thomhurst · Pull Request #5973 · thomhurst/TUnit

thomhurst · 2026-05-20T13:47:47Z

Summary

combine chained mock behaviors so callbacks and returns execute in the same invocation step
keep explicit Then() and ReturnsSequentially(...) as sequential behavior boundaries
preserve typed dispatch and custom side-effect behavior semantics in composites
apply state transitions after successful behavior execution

Tests

dotnet test .\TUnit.Mocks.Tests\TUnit.Mocks.Tests.csproj

Closes #5972

codacy-production · 2026-05-20T13:50:12Z

Not up to standards ⛔

🔴 Issues 2 medium · 8 minor

Alerts:
⚠ 10 issues (≤ 0 issues of at least minor severity)

Results:
10 new issues

Category Results

BestPractice 2 medium

CodeStyle 8 minor

View in Codacy

🟢 Metrics 78 complexity

Metric Results

Complexity 78

View in Codacy

_{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.}

claude

Code Review

This PR correctly fixes the root bug in #5972: chained .Callback().Returns() calls were treated as sequential invocation steps rather than a single composited invocation. The CompositeBehavior + ISideEffectBehavior design is a clean way to solve this.

Positive Aspects

ISideEffectBehavior is a solid abstraction — the "returns null but doesn't override the return value" distinction is the right invariant to encode.
Excellent test coverage: regression tests, throw-guard on state transitions, typed dispatch, async variants, and state machine integration.
State transition semantics improved: moving the _currentState update from before behavior execution to ApplyMatchedSetup (after execution) is the right semantic for state machines. If a behavior throws, state correctly stays put — as confirmed by TransitionsTo_Does_Not_Advance_State_When_Behavior_Throws.
ISetupChain<TReturn> now inheriting IMethodSetup<TReturn> reduces interface duplication cleanly.

Issues Worth Addressing

1. CompositeBehavior typed dispatch is a maintenance hazard (MockEngine.Typed.cs)

The new dispatch order in every ExecuteBehavior overload is:

b is IArgumentFreeBehavior af ? af.Execute() :
b is CompositeBehavior cb ? cb.Execute(a1, a2) :       // ← new special case
b is ITypedBehavior<T1, T2> tb ? tb.Execute(a1, a2) :
b.Execute(store.ToArray())

CompositeBehavior has internal typed Execute<T1, T2>(...) methods and can't implement ITypedBehavior<T1, T2> (exponential arity combinations). The result is two separate typed-dispatch mechanisms: one via interface, one via is CompositeBehavior special-casing.

The maintenance concern: every future arity added to MockEngine.Typed.cs must also be added to CompositeBehavior, but there's nothing enforcing that at compile time. A missed arity in CompositeBehavior falls through to the untyped Execute(object?[]), which silently degrades rather than failing.

One architectural improvement to consider: introduce an ITypedDispatch or IArityDispatch interface that CompositeBehavior and the typed engine code can use, rather than coupling to the concrete type. Alternatively, a source-generator invariant check ("CompositeBehavior must have same arities as MockEngine.Typed") would catch the drift.

2. ITypedBehavior<T> going from internal to public is a permanent API commitment

The original rationale ("typed dispatch is tightly coupled to the source generator") is explicitly removed. Making these public with [EditorBrowsable(EditorBrowsableState.Never)] allows external code to implement them (as demonstrated by FallbackThrowingTypedSideEffectBehavior in the test), but it means any future arity change is now a binary breaking change.

This is a reasonable trade-off if the intent is documented extensibility, but the doc comment currently just says "Custom behaviors can implement the matching arity to avoid the object?[] fallback." — that's a fairly casual statement for a permanent public API surface. Worth adding an explicit stability caveat, or considering [Experimental] attribute.

3. AddBehavior redundant condition

if (_nextBehaviorStartsNewStep || (_singleBehavior is null && _behaviors is null))

Since _nextBehaviorStartsNewStep initializes to true and is only set to false after successfully adding a behavior, the second clause (_singleBehavior is null && _behaviors is null) can only be true when _nextBehaviorStartsNewStep is also true. This is dead code — consider removing it to avoid misleading a future reader about invariants.

4. CompositeBehavior.Combine is O(n²) in allocation for long chains

if (first is CompositeBehavior composite)
{
    var combined = new IBehavior[composite._behaviors.Length + 1];
    composite._behaviors.CopyTo(combined, 0);
    combined[^1] = second;
    return new CompositeBehavior(combined);
}

Each AppendToCurrentBehaviorStep call copies the entire array. For n chained behaviors: 1 + 2 + 3 + ... + n allocations. This is setup-time-only so not a hot-path concern in practice, but a List<IBehavior> backing store would avoid the copies entirely and simplify the code. Minor — not blocking.

Summary

The fix is architecturally sound and the test coverage is thorough. The main concern is the coupling between CompositeBehavior's internal typed methods and MockEngine.Typed.cs's dispatch logic — if you add arity 9+ in the future, it's easy to update one and forget the other. The other points are relatively minor. Good work overall.

claude

Code Review

This is a well-structured fix for #5972. The + approach correctly models the distinction between "behaviors that contribute a return value" vs "behaviors that run as side effects," and the test coverage is thorough.

One previous review (mine) contained a couple of inaccuracies worth correcting:

ITypedBehavior<T> did NOT go public — the interfaces are still internal; adding [EditorBrowsable] to an internal declaration is a no-op and was misleading to flag.
The "redundant condition" flagged in AddBehavior does not exist in this diff.

Critical: Implicit behavioral breaking change for chained returns

The most significant concern is undocumented:

Before this PR, every AddBehavior call added a new sequential invocation step:

mock.Foo().Returns(1).Returns(2);
// Call 1 → 1, Call 2 → 2

After this PR, behaviors without an explicit Then() are composed into one step:

mock.Foo().Returns(1).Returns(2);
// Both calls → 2 (last non-side-effect wins)

Users who relied on chained .Returns() for sequential values without ReturnsSequentially or Then() will silently see different results — no compile error, no runtime error, just wrong values. This needs to be documented prominently in the changelog/migration guide (at minimum) or ideally caught with a warning if it's detectable.

ReturnsSequentially was correctly updated to use Then() between values, so that pattern is safe. But free chaining is not.

Moderate: Arity coupling now lacks documentation

The arity-coupling comment block was removed from MockEngine.Typed.cs with this diff. That comment was the only place telling maintainers that adding arity T9 requires updating both ICompositeBehavior/CompositeBehavior AND MockEngine.Typed.cs. Now CompositeBehavior.cs has the ICompositeBehavior interface definition and 8-arity typed Execute methods, but there's nothing coupling them to the engine's dispatch other than convention.

If a future arity is added to MockEngine.Typed.cs but ICompositeBehavior is missed, the composite silently falls through to b.Execute(store.ToArray()) (untyped, boxing) — no build error, subtle perf regression. Consider adding the coupling comment to CompositeBehavior.cs where the interface is declared.

Minor: Redundant volatile operations inside lock

AddBehaviorStep is always called inside lock (BehaviorLock), but uses Volatile.Write/Volatile.Read:

private void AddBehaviorStep(IBehavior behavior)
{
    if (_singleBehavior is null && _behaviors is null)
    {
        Volatile.Write(ref _singleBehavior, behavior);   // ← redundant inside lock
        return;
    }
    ...
    behaviors = [Volatile.Read(ref _singleBehavior)!];  // ← redundant inside lock

The lock provides the necessary memory barriers. The volatile calls are left over from the previous lock-free fast path. They don't cause bugs but will confuse future readers into thinking a lock-free path still exists here.

Positive highlights

Moving _currentState = setup.TransitionTarget to ApplyMatchedSetup (post-execution) with proper lock (Lock) is the right fix — the previous approach would advance state even on a thrown exception, which was semantically wrong. TransitionsTo_Does_Not_Advance_State_When_Behavior_Throws is a good regression test for this.
ISideEffectBehavior as a public marker interface is clean extensibility — the BehaviorCompositionRegressionTests custom behavior validates this works correctly.
ISetupChain<TReturn> extending IMethodSetup<TReturn> is a clean simplification; no breaking change since TransitionsTo on IMethodSetup<TReturn> already returns ISetupChain<TReturn>.
The 243-line MixedBehaviorRegressionTests is comprehensive and documents the expected semantics across async, out-params, events, and state machines.

The breaking behavioral change for chained returns is the main thing to address — everything else is polish.

claude

Code Review

This is a well-structured fix for #5972. The CompositeBehavior + ISideEffectBehavior approach correctly models the distinction between "behaviors that contribute a return value" vs "behaviors that run as side effects," and the test coverage is thorough.

One previous review (mine) contained a couple of inaccuracies worth correcting:

ITypedBehavior<T> did NOT go public — the interfaces are still internal; adding [EditorBrowsable] to an internal declaration is a no-op.
The "redundant condition" flagged in AddBehavior does not exist in this diff.

Critical: Implicit behavioral breaking change for chained returns

The most significant concern is undocumented.

Before this PR, every AddBehavior call added a new sequential invocation step:

mock.Foo().Returns(1).Returns(2);
// Call 1 → 1, Call 2 → 2

After this PR, behaviors without an explicit Then() are composed into one step:

mock.Foo().Returns(1).Returns(2);
// Both calls → 2  (last non-side-effect wins)

Users who relied on chained .Returns() for sequential values without ReturnsSequentially or Then() will silently see different results — no compile error, no runtime error, just wrong values. ReturnsSequentially was correctly updated to use Then() between values so that pattern is safe, but free chaining is not. This needs a changelog/migration note at minimum.

Moderate: Arity coupling comment was removed

The arity-coupling comment block was removed from MockEngine.Typed.cs. That comment was the only place telling maintainers that adding arity T9 requires updating both ICompositeBehavior/CompositeBehavior AND MockEngine.Typed.cs. If a future arity is added to MockEngine.Typed.cs but ICompositeBehavior is missed, the composite silently falls through to b.Execute(store.ToArray()) (untyped boxing) — no build error, subtle perf regression. Consider adding the coupling comment to CompositeBehavior.cs where the interface is declared.

Minor: Redundant volatile operations inside lock

AddBehaviorStep is always called inside lock (BehaviorLock), but uses Volatile.Write/Volatile.Read. The lock provides the necessary memory barriers. These volatile calls are left over from the previous lock-free fast path and will confuse future readers into thinking a lock-free path still exists.

Positive highlights

Moving _currentState = setup.TransitionTarget to ApplyMatchedSetup (post-execution) with proper lock(Lock) is the right fix — the previous approach advanced state even when behavior threw. TransitionsTo_Does_Not_Advance_State_When_Behavior_Throws is a good new regression test for this.
ISideEffectBehavior as a public marker interface is clean extensibility — the custom behavior in BehaviorCompositionRegressionTests validates it works correctly.
ISetupChain<TReturn> extending IMethodSetup<TReturn> is a clean simplification; no breaking change since IMethodSetup<TReturn>.TransitionsTo already returns ISetupChain<TReturn>.
MixedBehaviorRegressionTests is comprehensive and documents expected semantics across async, out-params, events, and state machines.

The breaking behavioral change for chained returns is the main thing to address before merging — everything else is polish.

claude

Code Review — Follow-up (third pass)

All three issues raised in the previous two reviews have been addressed:

Previously flagged, now resolved:

Breaking behavioral change for chained returns — Addressed by the doc update in docs/docs/writing-tests/mocking/setup.md, which now clearly explains that behaviors chained without .Then() execute as one step and that the last non-side-effect return wins. Users relying on sequential chained .Returns() are directed to ReturnsSequentially or explicit .Then() separators.
Arity coupling comment removed — Addressed by the comment block at the top of CompositeBehavior.cs naming all the files that must be kept in sync when adding a new arity. This is the right place to put it since the interface definition lives there.
Redundant volatile operations inside lock — Addressed. AddBehaviorStep (always called under BehaviorLock) now uses plain field access. The GetNextBehavior lock-free reader still uses Volatile.Read correctly, and the field comment accurately reflects this split. No confusion for future readers.

Still present, still acceptable:

CompositeBehavior.Combine copies the backing array on every append (O(n²) allocations for long chains). This was flagged in the first review as minor/non-blocking, and it remains so — mock setup chains are short in practice and this is setup-time-only, not the invocation hot path.

Overall: The fix is correct, the design is sound, and all substantive review feedback has been incorporated. Ready to merge.

claude

Code Review

Summary

Good fix for #5972. The core problem was well-diagnosed: chained behaviors () were being added as separate sequential invocation steps, so on invocation 2 would never be reached when the user expected them to run together on invocation 1. The + approach is clean and the semantics are correct.

What's working well

State transition timing is now correct. Moving from inside the setup-matching loop to after in means a throwing behavior correctly leaves the state unchanged. I verified by reading the call site at line ~252 — behavior executes first, runs only on success.
** marker** cleanly separates "I ran a side effect" from "I produced a return value", avoiding the callback-overwrites-return bug without needing special-casing in individual behavior types.
** now extends ** — removes the impedance mismatch where callers had to juggle two interface types for the same chain.
OTel drain test rewrite — replacing the wall-clock with a controlled upstream + gate is the right fix for the macOS flake. The old approach tested elapsed time; the new one tests the actual invariant.
Test coverage is thorough: explicit cases for throw-before-callback, last-return-wins, sequential separation via , typed dispatch, and state transition on throw.

Concern: has O(N²) allocations for a single step

Every call to copies the existing array:

For a step with N behaviors, this performs 1 + 2 + ... + (N-1) = O(N²) copies during setup. For the typical 2–3 behavior chain this is fine, but it's an unexpected footgun. A builder that is sealed to an array on first Execute would be O(N) total. Since is called under , a simple held directly on the step slot would work without the array-copy dance.

Minor: default semantics are implicit

The flag starts , gets set after the first , and gets reset to by . The rule "behaviors chain until " is correct, but the state machine is not obvious from the field name alone. Something like a nullable "current step index" or an explicit "step under construction" reference would surface the grouping intent more directly. Low priority.

Minor: Lock-free path removed without comment

The previous had a careful fast path to avoid allocating for the single-behavior case, with a correctness comment explaining the CAS-then-double-check idiom. That's now replaced with unconditionally. Setup is not a hot path so this is fine, but the removal is silent — a one-line comment noting it was intentionally simplified would help future readers who wonder why the lock-free path disappeared.

Nit: arity comment lives in a new place but still needs updating

The arity coupling comment was removed from and a new one added at the top of . Good move. Just verify the new list is complete — it mentions , , , both setup builders, and . Looks complete.

Overall this is a well-structured fix with solid test coverage. The O(N²) allocation concern is the only thing worth revisiting if the mocks library sees heavy use, but for typical mock chains it won't matter in practice.

claude

Code Review

Summary

Good fix for #5972. The core problem was well-diagnosed: chained behaviors were being added as separate sequential invocation steps, so Returns on invocation 2 would never be reached when the user expected them to run together on invocation 1. The CompositeBehavior + ISideEffectBehavior approach is clean and the semantics are correct.

What's working well

State transition timing is now correct. Moving _currentState = setup.TransitionTarget from inside the setup-matching loop to after behavior.Execute() in MockEngine.cs means a throwing behavior correctly leaves the state unchanged. Verified by reading the call site around line 252 -- behavior executes first, ApplyMatchedSetup runs only on success.
ISideEffectBehavior marker cleanly separates "ran a side effect" from "produced a return value", avoiding the callback-overwrites-return bug without special-casing in individual behavior types.
ISetupChain<TReturn> now extends IMethodSetup<TReturn> -- removes the impedance mismatch where callers had to juggle two interface types for the same chain.
OTel drain test rewrite -- replacing the wall-clock Task.Delay(200ms) with a controlled upstream HttpListener + TaskCompletionSource gate is the right fix for the macOS flake. The old approach tested elapsed time; the new one tests the actual invariant.
Test coverage is thorough: explicit cases for throw-before-callback, last-return-wins, sequential separation via Then(), typed dispatch, and state transition on throw.

Concern: `CompositeBehavior.Combine` has O(N^2) allocations for a single step

Every call to AppendToCurrentBehaviorStep copies the existing _behaviors array:

var combined = new IBehavior[composite._behaviors.Length + 1];
composite._behaviors.CopyTo(combined, 0);
combined[^1] = second;
return new CompositeBehavior(combined);

For a step with N behaviors this performs 1 + 2 + ... + (N-1) = O(N^2) copies during setup. For a typical 2-3 behavior chain this is fine, but it is an unexpected footgun. A List<IBehavior> builder that seals to an array on first Execute would be O(N) total. Since AppendToCurrentBehaviorStep is already called under BehaviorLock, a simple list held directly on the step slot would work without the array-copy dance.

Minor: Lock-free path removed without comment

The previous AddBehavior had a careful Interlocked.CompareExchange fast path with an explicit correctness comment about the CAS-then-double-check idiom. That is now replaced with unconditional lock (BehaviorLock). Setup is not a hot path so this is fine, but a one-line note that it was intentionally simplified would help future readers who might wonder why the lock-free path disappeared.

Minor: `_nextBehaviorStartsNewStep` semantics are implicit

The flag starts true, becomes false after the first AddBehavior, and resets to true via Then(). The "behaviors chain unless separated by Then()" rule is correct, but it's not surfaced by the field name alone. Something like a nullable "step under construction" slot would make the grouping intent more direct. Low priority.

Overall this is a well-structured fix with solid test coverage. The O(N^2) allocation concern is the only thing worth revisiting if the mocks library sees heavy use, but for typical mock chains it will not matter in practice.

claude

Code Review — Final Pass

All previously raised concerns across the review history have been addressed. Here is a final verification:

Previously flagged, confirmed resolved:

O(N²) allocations in CompositeBehavior — Resolved. CompositeBehavior now uses a List<IBehavior> internally with _snapshot caching and calls composite.Add(second) (O(1) append) when combining with an existing composite. O(N) total for a chain of N behaviors.
Breaking behavioral change for chained .Returns() — Resolved. docs/docs/writing-tests/mocking/setup.md now clearly documents that chained behaviors without .Then() execute as a single invocation step and the last return wins, with explicit guidance to use ReturnsSequentially or .Then() when sequential steps are needed.
Arity coupling comment removed — Resolved. The comment block at the top of CompositeBehavior.cs names all files that must be updated when adding a new arity, which is the right place since the interface lives there.
Redundant volatile operations inside lock — Resolved. AddBehaviorStep (always called under BehaviorLock) uses plain field access. Volatile.Read in GetNextBehavior is still correctly used for the lock-free read path. The field comment accurately describes this split.
Lock-free path removed without comment — Resolved. The lock (BehaviorLock) in AddBehavior now has an inline comment explaining the intentional removal of the CAS fast path.
_hasOpenBehaviorStep semantics — Resolved. The field has a <summary> doc comment ("True while chained behaviors are being composed into the current invocation step") and a clear name. The state machine is obvious.

Design correctness verified:

State transition timing is correct: _currentState is set in ApplyMatchedSetup, which is called after behavior execution. If the behavior throws, the exception bypasses ApplyMatchedSetup and state stays unchanged. TransitionsTo_Does_Not_Advance_State_When_Behavior_Throws directly verifies this.
CompositeBehavior.Execute throw semantics are correct: if a ThrowBehavior is in the composite, behaviors before it (e.g. a callback) run, then the exception propagates out of Execute, skipping any behaviors after it and skipping ApplyMatchedSetup. The Throw_Before_Callback_Does_Not_Run_Later_Callback_In_Same_Behavior test confirms this.
ICompositeBehavior vs ISideEffectBehavior visibility split is correct: ISideEffectBehavior is public to allow user-defined behaviors (as demonstrated by CustomSideEffectBehavior in the tests), while ICompositeBehavior/CompositeBehavior remain internal.
ReturnsSequentially correctly inserts Then() between each value, ensuring each maps to a distinct invocation step.

The fix is correct, the design is sound, and all substantive feedback has been addressed. Ready to merge.

fix(mocks): preserve chained setup behavior

e886f00

Closes #5972

thomhurst had a problem deploying to Pull Requests May 20, 2026 13:47 — with GitHub Actions Failure

thomhurst temporarily deployed to Pull Requests May 20, 2026 13:47 — with GitHub Actions Inactive