Skip to content

feat(mocks): migrate to T.Mock() extension syntax - #5472

Merged
thomhurst merged 4 commits into
mainfrom
feat/mock-extension-syntax-migration
Apr 9, 2026
Merged

feat(mocks): migrate to T.Mock() extension syntax#5472
thomhurst merged 4 commits into
mainfrom
feat/mock-extension-syntax-migration

Conversation

@thomhurst

Copy link
Copy Markdown
Owner

Summary

  • Migrate all tests, benchmarks, and docs from Mock.Of<T>() to T.Mock() C# 14 static extension syntax, using the more specific return type for interfaces (e.g., IGreeterMock instead of Mock<IGreeter>)
  • Extend source generator to discover classes (not just interfaces) from T.Mock() invocations and generate strongly-typed constructor-arg overloads via BuildForClass()
  • Fix 2nd-pass incremental guard to detect generated extensions for both interfaces and classes (checks ContainingType name instead of return type namespace)
  • Add snapshot test for class with constructor parameters covering the new overload generation
  • Update documentation to reflect T.Mock() as the primary API across index, setup, advanced, and argument-matchers pages

Mock.Of<T>() calls are intentionally preserved for multi-interface mocks, delegate mocks, wrap mocks, and the IDefaultValueProvider overload which have no T.Mock() equivalent.

Test plan

  • 37/37 snapshot tests pass (including new Class_With_Constructor_Parameters_Extension_Discovery)
  • TUnit.Mocks.Tests builds with 0 errors on net10.0
  • TUnit.Mocks.Benchmarks builds with 0 errors
  • CI passes across all target frameworks

…iscovery
- Migrate all tests, benchmarks, and docs from Mock.Of<T>() to T.Mock()
- Extend source generator to discover classes (not just interfaces) from T.Mock() invocations
- Generate strongly-typed constructor-arg overloads for class mocks (BuildForClass)
- Fix 2nd-pass guard to work for both interface and class extensions
- Add snapshot test for class with constructor parameters
- Update documentation to reflect T.Mock() as the primary API
@codacy-production

codacy-productionBot commented Apr 9, 2026

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues5 minor

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

Results:
5 new issues

CategoryResults
CodeStyle5 minor

View in Codacy

🟢 Metrics7 complexity

MetricResults
Complexity7

View in Codacy

TIP This summary will be updated as you push new changes. Give us feedback

@claudeclaudeBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review: feat(mocks): migrate to T.Mock() extension syntax

This is a clean, well-scoped migration. The source generator changes are well-structured and the 37-snapshot test pass gives good confidence. A few observations below.


Positive: Incremental guard fix is solid

The change from:

resolved.ReturnType.ContainingNamespace?.ToDisplayString()=="TUnit.Mocks.Generated"

to:

resolved.ContainingType?.Nameis{}containingName&&containingName.EndsWith("_MockStaticExtension")

is a meaningful improvement. The old check would miss class extensions (which live in TUnit.Mocks, not TUnit.Mocks.Generated), causing the source generator to process them twice. The new check catches both interface and class extensions uniformly.


Positive: Clean BuildCore extraction

The lambda-based callback pattern keeps the namespace/class scaffolding DRY while allowing Build() (interfaces) and BuildForClass() (classes) to differ only in their body generation. Good abstraction.


Medium: Constructor dispatch collision for same-param-count overloads

The generated factory dispatches on constructorArgs.Length:

if(constructorArgs.Length==1){impl=newFooMockImpl(engine,(string)constructorArgs[0]);}

If a class has two constructors with the same parameter count but different types:

publicMyService(string name){}publicMyService(int timeout){}

…the factory would generate two conflicting else if (constructorArgs.Length == 1) branches (or silently pick the first one). The extension methods themselves are compile-time safe because they generate distinct Mock(string name) and Mock(int timeout) overloads, but routing through Mock.Of<T>(behavior, args) with object[] loses the type information.

This isn't new — it's a limitation of the existing factory infrastructure — but this PR surfaces class mocking as a first-class advertised feature, so now it's more likely users will hit this with class types. Worth either:

  • Documenting the limitation (same-count, different-type constructors generate an ambiguous factory), or
  • Generating type-discriminated dispatch (e.g., (constructorArgs.Length == 1 && constructorArgs[0] is string))

Minor: Redundant sealed-check condition

In MockTypeDiscovery.cs:

if(namedType.IsSealed&&namedType.TypeKind!=TypeKind.Interface)returnImmutableArray<MockTypeModel>.Empty;

Since interfaces can never be sealed in C#, the namedType.TypeKind != TypeKind.Interface guard is always true when IsSealed is true. This is equivalent to if (namedType.IsSealed). Not a bug, but reduces clarity slightly.


Minor: ref readonly parameters may not be handled

FormatParameter handles In_Readonly and the loop skips Ref/Out. If ParameterDirection.Ref_Readonly (C# 12) exists in the enum and is not in the filter list, the generated extension method would silently omit the ref readonly prefix, producing code that doesn't compile. Worth either adding it to the skip filter or to the prefix lookup.


Minor: Method name BuildForClass is slightly misleading

The method also applies to abstract classes, not just concrete classes. Something like BuildForClassType or BuildForPartialMock would better signal the intent. (Low priority, just naming.)


Documentation

The docs updates are accurate and well-written. The note preserved about T.Mock() requiring C# 14 / .NET 10 is important and correctly placed.

Overall this is a solid PR — the core logic is correct, test coverage is good, and the migration is consistent across ~80 files. The constructor dispatch ambiguity is the main thing worth addressing before this is used in the wild with class types.

- Simplify IsSealed guard (interfaces can never be sealed)
- Rename BuildForClass to BuildForPartialMock for clarity

@claudeclaudeBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

Overall this is a well-executed migration. The ergonomics are a clear improvement over , and the source generator changes are structurally sound. A few things worth attention below.


✅ What's Working Well

**Template method refactor in **
Extracting and passing the body-writing logic as an is a clean way to avoid duplicating the class scaffolding boilerplate. The closure capture of shortName and mockNamespace in Build is idiomatic.

Incremental guard fix is correct and well-reasoned
The old guard checked whether the resolved method's return type namespace was TUnit.Mocks.Generated:

// oldresolved.ReturnType.ContainingNamespace?.ToDisplayString()=="TUnit.Mocks.Generated"

Class-mock extensions live in the TUnit.Mocks namespace, not TUnit.Mocks.Generated, so the old check would have missed them and caused an infinite re-generation loop. The new approach:

// newcontainingName.EndsWith("_MockStaticExtension")

…correctly covers both cases. The comment in the code explaining why is appreciated.

** simplification is safe**
The old guard was technically correct but redundant — interfaces can never be sealed in C#. Removing the redundant condition is fine.


⚠️ Concern: skips

has a guard:

if(!MockWrapperTypeBuilder.CanGenerateWrapper(model))returnstring.Empty;

has no such guard. If encodes invariants that also apply to partial (class) mocks (e.g., accessibility checks), we could silently emit an extension for a type whose mock impl was never generated.

If this is intentional (i.e., the guard only gates interface wrapper generation, which is irrelevant to class mocks), it's worth a comment here explaining why the guard is omitted:

publicstaticstringBuildForPartialMock(MockTypeModelmodel){// No CanGenerateWrapper check here — that guard gates interface-specific// wrapper type generation, which doesn't apply to class (partial) mocks.returnBuildCore(model, ...);}

⚠️ Potential bug: same-arity constructor dispatch ambiguity

In , the generated overloads are strongly typed per constructor signature. C# overload resolution will correctly pick the right one:

MyService.Mock("conn",30)// resolves to (string, int) ctor
MyService.Mock("conn",30,true)// resolves to (string, int, bool) ctor

However, both calls ultimately invoke , passing args as . The factory dispatch (from the snapshot) uses argument count:

if(constructorArgs.Length==2)impl=newMyServiceMockImpl(engine,(string)constructorArgs[0],(int)constructorArgs[1]);

If a future class has two constructors with the same number of parameters but different types, e.g.:

classSvc{Svc(stringname){}Svc(intid){}}

Then would generate an at runtime (the factory would try to cast to ). The extension overload resolution works, but the underlying routing is count-based, not type-based.

This appears to be a pre-existing limitation in the factory generator rather than something introduced here, but surfaces it more prominently. It may be worth a TODO comment, or the factory dispatch could be enhanced to include a type-signature check (e.g., comparing against expected types, or generating a different dispatch key).


Minor: and parameter modifiers

privatestaticstringFormatParameter(MockParameterModelp){varprefix=p.Direction==ParameterDirection.In_Readonly?"in ":"";return$"{prefix}{p.FullyQualifiedType}{p.Name}";}

This handles → , and the call site already filters out and . If has other values (e.g., a plain distinct from ), they'd silently emit no modifier. This is probably fine given the surrounding filter, but worth verifying the enum coverage is complete.


Summary

AreaAssessment
migration across tests/benchmarks/docsClean and consistent
refactorGood abstraction
Incremental guard fixCorrect and well-explained
omission inNeeds comment or verification
Same-arity constructor dispatchPre-existing limitation, worth a TODO
coverageLow risk, worth a quick check

The core feature works well. The main action item is ensuring the omission is intentional and documented.

@claudeclaudeBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

Overall this is a well-executed migration. The T.Mock() ergonomics are a clear improvement over Mock.Of<T>(), and the source generator changes are structurally sound. A few things worth attention below.


What's Working Well

Template method refactor in MockStaticExtensionBuilder

Extracting BuildCore and passing the body-writing logic as an Action<CodeWriter, string, string> is a clean way to avoid duplicating the class scaffolding boilerplate. The closure capture of shortName and mockNamespace in Build is idiomatic.

Incremental guard fix is correct and well-reasoned

The old guard checked whether the resolved method's return type namespace was TUnit.Mocks.Generated, but class-mock extensions live in TUnit.Mocks, not TUnit.Mocks.Generated. The old check would have missed them and caused an infinite re-generation loop. The new approach — checking that the containing type name ends with _MockStaticExtension — correctly covers both cases. The code comment explaining why is appreciated.

IsSealed simplification is safe

The old guard namedType.IsSealed && namedType.TypeKind != TypeKind.Interface was technically correct but redundant — interfaces can never be sealed in C#. Removing the redundant condition is fine.


Concern: BuildForPartialMock skips CanGenerateWrapper

Build has a guard:

if(!MockWrapperTypeBuilder.CanGenerateWrapper(model))returnstring.Empty;

BuildForPartialMock has no such guard. If CanGenerateWrapper encodes invariants that also apply to partial (class) mocks — e.g., accessibility checks — we could silently emit an extension for a type whose mock impl was never generated.

If this is intentional (i.e., the guard only gates interface wrapper generation), it's worth a comment clarifying the reason:

publicstaticstringBuildForPartialMock(MockTypeModelmodel){// No CanGenerateWrapper check — that guard gates interface-specific// wrapper type generation, which doesn't apply to class (partial) mocks.returnBuildCore(model, ...);}

Potential bug: same-arity constructor dispatch ambiguity

The generated T.Mock() overloads are strongly typed per constructor signature, so C# overload resolution correctly picks the right one. However, all overloads ultimately call Mock.Of<T>(behavior, arg1, arg2, ...), which passes args as params object[]. The factory dispatch (visible in the snapshot) routes by argument count:

if(constructorArgs.Length==2)impl=newMyServiceMockImpl(engine,(string)constructorArgs[0],(int)constructorArgs[1]);

If a class has two constructors with the same parameter count but different types:

classSvc{Svc(stringname){}Svc(intid){}}

...then Svc.Mock(42) would throw InvalidCastException at runtime (the factory would attempt to cast 42 to string). The extension overloads are fine at compile time, but the factory routing is count-based, not type-aware.

This is a pre-existing limitation in the factory generator rather than something introduced here, but BuildForPartialMock makes it more visible. A TODO comment and/or runtime type checks in the dispatch would prevent this silent failure.


Minor: FormatParameter and parameter modifier coverage

privatestaticstringFormatParameter(MockParameterModelp){varprefix=p.Direction==ParameterDirection.In_Readonly?"in ":"";return$"{prefix}{p.FullyQualifiedType}{p.Name}";}

The call site filters out Ref and Out, so those are handled. But if ParameterDirection has other values (e.g., a plain In vs In_Readonly), they would silently emit no modifier. Low risk given the existing filter, but worth confirming the enum coverage is complete.


Summary

AreaAssessment
T.Mock() migration across tests/benchmarks/docsClean and consistent
BuildCore refactorGood abstraction
Incremental guard fixCorrect and well-explained
CanGenerateWrapper omission in BuildForPartialMockNeeds comment or verification
Same-arity constructor dispatchPre-existing limitation, worth a TODO
FormatParameter coverageLow risk, worth a quick check

The core feature works well. The main action item is ensuring the CanGenerateWrapper omission is intentional and documented.

@claudeclaudeBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Follow-up Review (post-feedback commits)

The second and third commits have addressed the main feedback points cleanly. Here is the current state:


✅ Addressed from previous review

**Renamed → ** — better reflects that this applies to abstract classes too, not just concrete ones.

Simplified guard — the condition was always true when was true (interfaces can never be sealed), so removing it is correct and cleaner.

** comment added in ** — the clarifying comment is in place and accurately explains why the guard is intentionally omitted for class mocks.

** enum coverage** — I checked directly:

internalenumParameterDirection{In,// → no prefix (correct)Out,// → filtered out at call siteRef,// → filtered out at call siteIn_Readonly// → "in " prefix (correct)}

The coverage is complete. The previous concern about missing values is resolved — no action needed.


Still outstanding: same-arity constructor dispatch ambiguity

This is the one remaining unaddressed item. The generated factory dispatches on argument count:

if(constructorArgs.Length==2)impl=newMyServiceMockImpl(engine,(string)constructorArgs[0],(int)constructorArgs[1]);

For a class with two constructors of the same arity but different types:

classSvc{Svc(stringname){}Svc(intid){}}

would compile fine (the C# 14 extension overloads are type-safe), but at runtime the factory would attempt on an and throw .

This is a pre-existing limitation in the factory infrastructure, not introduced by this PR. But since now advertises class mocking as first-class, users are more likely to encounter it. A minimal mitigation would be a TODO comment in the factory generator (not in this PR — it would be in the impl builder) and optionally a runtime type check:

// Instead of count-only dispatch:elseif(constructorArgs.Length==2&&constructorArgs[0]isstring&&constructorArgs[1]isint)// Or a guard:elseif(constructorArgs.Length==2){if(constructorArgs[0]is not strings||constructorArgs[1]is not intt)thrownewArgumentException("Constructor arg types do not match (string, int).");impl=newMyServiceMockImpl(engine,s,t);}

This doesn't need to block the PR — it's a known, bounded limitation — but a follow-up issue tracking it would be worthwhile.


Summary

ItemStatus
→ rename✅ Done
guard simplification✅ Done
comment✅ Done
enum coverage✅ Confirmed correct
Same-arity constructor dispatch ambiguity⚠️ Pre-existing limitation, needs a TODO or follow-up issue

The PR is in good shape. The migration is consistent across ~80 files, snapshot tests pass, and the source generator changes are structurally sound. The constructor dispatch issue is the only remaining concern and doesn't block merging — it just needs tracking.

@claudeclaudeBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Follow-up Review (post-feedback commits)

The second and third commits have addressed the main feedback points cleanly. Here is the current state.


Addressed from previous review

Renamed BuildForClass to BuildForPartialMock — better reflects that this applies to abstract classes too, not just concrete ones.

Simplified IsSealed guard — the && namedType.TypeKind != TypeKind.Interface condition was always true when IsSealed was true (interfaces can never be sealed), so removing it is correct and cleaner.

CanGenerateWrapper comment added in BuildForPartialMock — the clarifying comment is in place and accurately explains why the guard is intentionally omitted for class mocks.

FormatParameter enum coverage — I checked ParameterDirection directly:

internalenumParameterDirection{In,// no prefix (correct)Out,// filtered out at call siteRef,// filtered out at call siteIn_Readonly// "in " prefix (correct)}

The coverage is complete. The previous concern about missing values is a non-issue — no action needed.


Still outstanding: same-arity constructor dispatch ambiguity

This is the one remaining item from the previous review. The generated factory dispatches on argument count:

if(constructorArgs.Length==2)impl=newMyServiceMockImpl(engine,(string)constructorArgs[0],(int)constructorArgs[1]);

For a class with two constructors of the same arity but different types:

classSvc{Svc(stringname){}Svc(intid){}}

Svc.Mock(42) would compile fine (the C# 14 extension overloads are type-safe), but at runtime the factory would attempt (string)constructorArgs[0] on an int and throw InvalidCastException. This is a pre-existing limitation in the factory infrastructure, not introduced by this PR — but surfacing class mocking as first-class makes users more likely to encounter it.

A minimal mitigation would be a TODO comment in the factory generator, or a follow-up issue. A more robust fix would be type-discriminated dispatch:

elseif(constructorArgs.Length==2&&constructorArgs[0]isstrings&&constructorArgs[1]isintt)
impl =newMyServiceMockImpl(engine,s,t);elseif(constructorArgs.Length==2&&constructorArgs[0]isintid)
impl =newMyServiceMockImpl(engine,id);

This doesn't need to block the PR — it's a bounded, known limitation — but a follow-up tracking issue would be worthwhile.


Summary

ItemStatus
BuildForClass to BuildForPartialMock renameDone
IsSealed guard simplificationDone
CanGenerateWrapper commentDone
FormatParameter enum coverageConfirmed correct
Same-arity constructor dispatch ambiguityPre-existing limitation, needs a TODO or follow-up issue

The PR is in good shape. The migration is consistent across ~80 files, snapshot tests pass, and the source generator changes are structurally sound. The constructor dispatch issue is the only remaining concern and does not block merging.

@thomhurst
thomhurst disabled auto-merge April 9, 2026 17:42
Constructor dispatch now always checks argument types at runtime, not
just argument count. This correctly handles classes with multiple
constructors sharing the same parameter count but different types
(e.g., Svc(string) vs Svc(int)) and gives clear ArgumentException
messages on type mismatches instead of InvalidCastException.

@claudeclaudeBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Follow-up Review (post-fix commit: type-discriminated dispatch)

The fourth commit — "fix: use type-discriminated dispatch for all constructor arities" — directly addresses the one remaining concern from previous reviews. Here is a complete status update.


All previous concerns are now resolved

Constructor dispatch ambiguity — fixed

The generated factory now uses type-aware pattern matching rather than count-only routing. For example, with same-arity constructors:

if(constructorArgs.Length==1){if((constructorArgs[0]isnull or string))impl=newMyServiceMockImpl(engine,(string)constructorArgs[0]);elseif(constructorArgs[0]isint)impl=newMyServiceMockImpl(engine,(int)constructorArgs[0]);elsethrownewArgumentException($"No matching constructor found...");}

This is exactly the right fix. The is null or string pattern correctly handles nullable strings, value-type constructors are matched by concrete type, and unresolvable cases produce a clear ArgumentException rather than a silent InvalidCastException. The new Class_With_Same_Arity_Constructor_Overloads snapshot test covers this path directly.

ItemStatus
BuildForClassBuildForPartialMock renameDone
IsSealed guard simplificationDone
CanGenerateWrapper comment in BuildForPartialMockDone
FormatParameter enum coverageConfirmed correct
Same-arity constructor dispatch ambiguityFixed — type-discriminated dispatch

One edge case worth noting (not a blocker)

The is null or string branch is checked before value-type branches. This is the right default ordering, but it means if a class has both Svc(string s) and Svc(object o) constructors, a null argument will always route to string. This is almost certainly the desired behavior, but worth documenting if the mocking framework is expected to support object-typed constructor parameters at some point.


Summary

This PR is ready to merge. The migration is consistent across ~80 files, all 37 snapshot tests pass (including the two new ones covering class mocking), the source generator changes are structurally sound, and the constructor dispatch issue that warranted a follow-up has been cleanly resolved. Good work iterating on the feedback.

@thomhurst
thomhurst enabled auto-merge (squash) April 9, 2026 18:38
@thomhurst
thomhurst merged commit 9839302 into mainApr 9, 2026
14 of 15 checks passed
@thomhurst
thomhurst deleted the feat/mock-extension-syntax-migration branch April 9, 2026 18:47
This was referenced Jun 2, 2026
Sign up for freeto 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

@thomhurst