Conversation
…ory inheritance Flags classes inheriting directly from `Microsoft.AspNetCore.Mvc.Testing.WebApplicationFactory<T>` and offers a code fix to rewrite the base to `TUnit.AspNetCore.TestWebApplicationFactory<T>`, preserving distributed tracing, per-test logging correlation, and `TestContext.Current` resolution inside request handlers. Closes #5596
There was a problem hiding this comment.
TUnit0064 Analyzer + Code Fix — Review
This is a well-structured addition that follows the existing analyzer/codefixer patterns in the repo. The analyzer logic is sound, the diagnostic location is correct (base type, not class identifier), and the transitive-safety reasoning is clearly thought through. A few things worth checking before merge:
1. Code fix missing from versioned Roslyn packaging paths (potential gap)
In TUnit.AspNetCore.Core.csproj, the codefix DLL is only packed into analyzers/dotnet/cs:
<None Include="...TUnit.AspNetCore.Analyzers.CodeFixers.dll"
Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" />But the analyzers also ship at versioned paths: analyzers/dotnet/roslyn4.14/cs, analyzers/dotnet/roslyn4.4/cs, and analyzers/dotnet/roslyn4.7/cs. The code fix provider references the analyzer's DiagnosticId at runtime to wire up, so the fixer DLL needs to be resolvable alongside whichever versioned analyzer is loaded. Whether NuGet's fallback convention resolves dotnet/cs as a base path for all Roslyn variants is worth explicitly verifying — the existing TUnit.Analyzers.CodeFixers packaging is the right reference to confirm against.
2. AddUsingIfMissing only inspects compilation-unit-level usings
if (compilationUnit.Usings.Any(u => u.Name?.ToString() == namespaceName))
return compilationUnit;CompilationUnitSyntax.Usings only covers top-level using directives. If the user writes:
namespace MyApp
{
using TUnit.AspNetCore; // inside a namespace block
public class MyFactory : WebApplicationFactory<Program> { }
}The code fix will add a second using TUnit.AspNetCore; at the top of the file. A more robust check would also scan NamespaceDeclarationSyntax.Usings in the compilation unit. This is an edge case but worth handling cleanly, especially since this fix will be applied automatically.
3. Missing code fix test: no-duplicate-using when already present
The single code fix test (Rewrites_Base_Type_To_TestWebApplicationFactory) only covers the case where there is no pre-existing using TUnit.AspNetCore;. There is no test verifying that the fix does not insert a duplicate when the using already exists:
// fixedSource should not gain a second using
using TUnit.AspNetCore;
public class MyFactory : {|#0:Microsoft.AspNetCore.Mvc.Testing.WebApplicationFactory<Program>|} { }Given that the fix-all provider (BatchFixer) may apply this across multiple files, a duplicate-using bug here would be more disruptive than a single-file typo.
4. Minor: AddUsingIfMissing appends at the end of the using list
compilationUnit.AddUsings(newUsing) appends the new directive after existing ones. Most C# style guides and IDE formatters sort usings alphabetically — TUnit.AspNetCore would typically sort after System.* but before project-local namespaces. This is purely cosmetic (the Formatter.Annotation on the replacement node handles indentation, but not using order), but users on projects with enforced using-sort rules may see a follow-up Roslyn fix applied on top. Not a blocker, just worth being aware of.
Summary
The core analyzer logic, diagnostic placement, and guard against TestWebApplicationFactory itself are all correct. The packaging gap (#1) is the most important thing to verify before shipping — if the code fix doesn't load in the versioned Roslyn scenarios, users will see the squiggle but no quick-fix lightbulb, which is a worse UX than no fix at all. The duplicate-using edge case (#2 / #3) is the other thing worth tightening up with a test.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
🟢 Metrics 40 complexity
Metric Results Complexity 40
TIP This summary will be updated as you push new changes. Give us feedback
…de fix Also guards against ignoring pre-existing top-level usings and adds tests for both cases (top-level + namespace-scoped) so BatchFixer can't emit duplicate directives.
|
Thanks for the review. Responses: 1. Packaging: The single 2 & 3. Namespace-scoped usings + test: Real gap, fixed in 5aeb7cd. 4. Using order: Agreed — purely cosmetic and downstream formatters sort. Leaving to avoid scope creep. 23/23 tests pass on net9.0. |
There was a problem hiding this comment.
PR Review: TUnit0064 — Direct WebApplicationFactory Inheritance Analyzer + Code Fix
This is a well-executed addition that follows the existing analyzer/code-fixer patterns cleanly. The diagnostic rationale is sound (silent loss of tracing, log correlation, and TestContext.Current) and the implementation correctly avoids false positives.
What works well
Analyzer design: Using RegisterCompilationStartAction to resolve WebApplicationFactory and TestWebApplicationFactory once per compilation (rather than once per symbol) is the right approach and avoids repeated lookups.
Diagnostic location: Reporting at baseList.Types[0].GetLocation() instead of the class identifier is deliberate and correct — the squiggle appears exactly where the problematic inheritance is declared.
Transitive case: The early exit when baseType.OriginalDefinition != webApplicationFactory correctly handles chains like MyFactory : UserBase : TestWebApplicationFactory<T> without false positives.
Code fix robustness: Handling all three qualified name forms (GenericNameSyntax, QualifiedNameSyntax { Right: GenericNameSyntax }, AliasQualifiedNameSyntax) covers the realistic ways users would write the fully-qualified base type.
Using deduplication: ContainsUsingInNamespace correctly descends into BaseNamespaceDeclarationSyntax, which covers both block-scoped and file-scoped namespace declarations.
Issues / Concerns
1. Code fixer not bundled in Roslyn-version-specific paths (potential packaging gap)
The analyzer is packed at both:
analyzers/dotnet/cs(generic fallback)analyzers/dotnet/roslyn4.14/cs(specific Roslyn version)
But the code fixer is only at:
<None Include=...TUnit.AspNetCore.Analyzers.CodeFixers.dll
Pack=true PackagePath=analyzers/dotnet/cs />IDEs and tooling that prefer the versioned path (e.g., JetBrains Rider picking roslyn4.14/cs) will find the analyzer but may not find the paired code fixer. The fix would be to also pack the code fixer at each versioned analyzer path, or confirm that NuGet's fallback-to-generic behavior covers this case.
2. Null-guard asymmetry for testWebApplicationFactory
When testWebApplicationFactory is null (i.e., TUnit.AspNetCore isn't in the compilation), the analyzer still fires the diagnostic. The code fix then adds using TUnit.AspNetCore; and rewrites the base type — but the type doesn't exist in the compilation, producing a new compile error. In practice this is fine because the analyzer is bundled within the package that includes TestWebApplicationFactory<T>, so both are always present together. But the intent isn't clear from the code itself:
// Current: silently warns even when TestWebApplicationFactory<T> is unavailable
if (testWebApplicationFactory is not null &&
SymbolEqualityComparer.Default.Equals(type.OriginalDefinition, testWebApplicationFactory))
{
return;
}A comment explaining why the null case still warns would prevent future confusion, or alternatively bail early when null if you want to be conservative:
// If TestWebApplicationFactory<T> isn't in scope, nothing to migrate to — skip.
if (testWebApplicationFactory is null)
return;This would also make the code fixer safe to apply in any context.
3. Missing test: file-scoped namespace using deduplication
The ContainsUsingInNamespace logic handles FileScopedNamespaceDeclarationSyntax (via BaseNamespaceDeclarationSyntax), but there's no test for:
using TUnit.AspNetCore; // file-scoped namespace with inner using
namespace App;
public class MyFactory : WebApplicationFactory<Program> { }If the code fix incorrectly duplicates the using in this case, it would be a silent regression. A test covering this form would lock in the expected behavior.
4. Missing test: partial class declarations
If someone splits their factory across two files and only one declares the base type, GetBaseTypeLocation iterates DeclaringSyntaxReferences and returns the first match. This is correct, but there's no test confirming the diagnostic fires exactly once (not twice) for a partial class.
Summary
The core logic is solid and the design is consistent with the rest of the analyzer suite. The packaging asymmetry (#1) is the only concern that could affect real users — worth verifying whether the generic analyzers/dotnet/cs path is sufficient or if a versioned path is needed for the code fixer. Items #2–4 are lower priority (defensive clarity + test coverage).
If `TUnit.AspNetCore` isn't referenced, warning a user to migrate to a type they can't resolve is unhelpful and would make the code fix produce a compile error. Bail out of the analyzer early in that case. Also adds tests for file-scoped-namespace using deduplication and partial class declarations.
|
Thanks for round 2: 1. Packaging (re-raised): Same situation as the first review — the existing 2. Null-guard asymmetry: Good call. Fixed in cbdc125 — now bailing out at the compilation-start action when 3. File-scoped namespace test: Added 4. Partial class test: Added 25/25 tests pass on net9.0. |
There was a problem hiding this comment.
Follow-up Review: TUnit0064 — All Prior Concerns Addressed
Reviewing the two follow-up commits (5aeb7cdeb491, cbdc125a2a89) against the issues raised in previous reviews.
Previous concerns — status
1. Code fixer not in Roslyn-versioned packaging paths
✅ This is a non-issue by design. Checking TUnit.Core.csproj, TUnit.Analyzers.CodeFixers is also only packed at analyzers/dotnet/cs — code fixers consistently use the generic path while only analyzers get versioned paths. The current approach matches the established repo pattern.
2. Null-guard asymmetry (testWebApplicationFactory is null)
✅ Fixed in cbdc125a2a89. The analyzer now bails out early when TestWebApplicationFactory<T> isn't in the compilation, so the fixer can never produce a broken-reference compile error.
3. Namespace-scoped using deduplication
✅ Fixed in 5aeb7cdeb491. ContainsUsingInNamespace now recurses into BaseNamespaceDeclarationSyntax (covers both block-scoped and file-scoped namespaces), and AddUsingIfMissing checks both compilationUnit.Usings and namespace members before inserting.
4. Missing test: file-scoped namespace deduplication
✅ Added: Does_Not_Duplicate_Using_In_File_Scoped_Namespace.
5. Missing test: partial class fires once
✅ Added: Warning_Fires_Once_For_Partial_Class.
One remaining minor observation
There's no test covering the case where WebApplicationFactory<T> is present but TestWebApplicationFactory<T> is not (i.e., the null-guard path added in cbdc125a2a89). This is a narrow scenario (possible only when someone references Microsoft.AspNetCore.Mvc.Testing without TUnit.AspNetCore), but a test like No_Warning_When_TestWebApplicationFactory_Not_In_Scope would lock in that behavior:
[Test]
public async Task No_Warning_When_TestWebApplicationFactory_Not_In_Scope()
{
// Only stub WebApplicationFactory, not TestWebApplicationFactory
await Verifier.VerifyAnalyzerAsync("""
namespace Microsoft.AspNetCore.Mvc.Testing
{
public class WebApplicationFactory<T> where T : class { }
}
public class Program { }
public class MyFactory : Microsoft.AspNetCore.Mvc.Testing.WebApplicationFactory<Program> { }
""");
}This is low priority — the fix is correct and the path is implicitly exercised — but the test would prevent a future refactor from re-introducing the issue silently.
Summary
All substantive concerns from the prior reviews have been addressed cleanly and correctly. The analyzer design, diagnostic placement, code fix robustness, and packaging are all solid. The PR is ready to merge — the null-guard test above is purely additive polish, not a blocker.
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.35.2 to 1.37.10. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.37.10 <!-- Release notes generated using configuration in .github/release.yml at v1.37.10 --> ## What's Changed ### Other Changes * docs(test-filters): add migration callout for --filter → --treenode-filter by @johnkattenhorn in thomhurst/TUnit#5628 * fix: re-enable RPC tests and modernize harness (#5540) by @thomhurst in thomhurst/TUnit#5632 * fix(mocks): propagate [Obsolete] and null-forgiving raise dispatch (#5626) by @JohnVerheij in thomhurst/TUnit#5631 * ci: use setup-dotnet built-in NuGet cache by @thomhurst in thomhurst/TUnit#5635 * feat(playwright): propagate W3C trace context into browser contexts by @thomhurst in thomhurst/TUnit#5636 ### Dependencies * chore(deps): update tunit to 1.37.0 by @thomhurst in thomhurst/TUnit#5625 ## New Contributors * @johnkattenhorn made their first contribution in thomhurst/TUnit#5628 * @JohnVerheij made their first contribution in thomhurst/TUnit#5631 **Full Changelog**: thomhurst/TUnit@v1.37.0...v1.37.10 ## 1.37.0 <!-- Release notes generated using configuration in .github/release.yml at v1.37.0 --> ## What's Changed ### Other Changes * fix: stabilize flaky tests across analyzer, OTel, and engine suites by @thomhurst in thomhurst/TUnit#5609 * perf: engine hot-path allocation wins (#5528 B) by @thomhurst in thomhurst/TUnit#5610 * feat(analyzers): detect collection IsEqualTo reference equality (TUnitAssertions0016) by @thomhurst in thomhurst/TUnit#5615 * perf: consolidate test dedup + hook register guards (#5528 A) by @thomhurst in thomhurst/TUnit#5612 * perf: engine discovery/init path cleanup (#5528 C) by @thomhurst in thomhurst/TUnit#5611 * fix(assertions): render collection contents in IsEqualTo failure messages (#5613 B) by @thomhurst in thomhurst/TUnit#5619 * feat(analyzers): code-fix for TUnit0015 to insert CancellationToken (#5613 D) by @thomhurst in thomhurst/TUnit#5621 * fix(assertions): add Task reference forwarders on AsyncDelegateAssertion by @thomhurst in thomhurst/TUnit#5618 * test(asp-net): fix race in FactoryMethodOrderTests by @thomhurst in thomhurst/TUnit#5623 * feat(analyzers): code-fix for TUnit0049 to insert [MatrixDataSource] (#5613 C) by @thomhurst in thomhurst/TUnit#5620 * fix(pipeline): isolate AOT publish outputs to stop clobbering pack DLLs (#5622) by @thomhurst in thomhurst/TUnit#5624 ### Dependencies * chore(deps): update tunit to 1.36.0 by @thomhurst in thomhurst/TUnit#5608 * chore(deps): update modularpipelines to 3.2.8 by @thomhurst in thomhurst/TUnit#5614 **Full Changelog**: thomhurst/TUnit@v1.36.0...v1.37.0 ## 1.36.0 <!-- Release notes generated using configuration in .github/release.yml at v1.36.0 --> ## What's Changed ### Other Changes * fix: don't render test's own trace as Linked Trace in HTML report by @thomhurst in thomhurst/TUnit#5580 * fix(docs): benchmark index links 404 by @thomhurst in thomhurst/TUnit#5587 * docs: replace repeated benchmark link suffix with per-test descriptions by @thomhurst in thomhurst/TUnit#5588 * docs: clearer distributed tracing setup and troubleshooting by @thomhurst in thomhurst/TUnit#5597 * fix: auto-suppress ExecutionContext flow for hosted services (#5589) by @thomhurst in thomhurst/TUnit#5598 * feat: auto-align DistributedContextPropagator to W3C by @thomhurst in thomhurst/TUnit#5599 * feat: TUnit0064 analyzer + code fix for direct WebApplicationFactory inheritance by @thomhurst in thomhurst/TUnit#5601 * feat: auto-propagate test trace context through IHttpClientFactory by @thomhurst in thomhurst/TUnit#5603 * feat: TUnit.OpenTelemetry zero-config tracing package by @thomhurst in thomhurst/TUnit#5602 * fix: restore [Obsolete] members removed in v1.27 (#5539) by @thomhurst in thomhurst/TUnit#5605 * feat: generalize OTLP receiver for use outside TUnit.Aspire by @thomhurst in thomhurst/TUnit#5606 * feat: auto-configure OpenTelemetry in TestWebApplicationFactory SUT by @thomhurst in thomhurst/TUnit#5607 ### Dependencies * chore(deps): update tunit to 1.35.2 by @thomhurst in thomhurst/TUnit#5581 * chore(deps): update dependency typescript to ~6.0.3 by @thomhurst in thomhurst/TUnit#5582 * chore(deps): update dependency coverlet.collector to v10 by @thomhurst in thomhurst/TUnit#5600 **Full Changelog**: thomhurst/TUnit@v1.35.2...v1.36.0 Commits viewable in [compare view](thomhurst/TUnit@v1.35.2...v1.37.10). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
TUnit0064(Warning) — flags classes directly inheritingMicrosoft.AspNetCore.Mvc.Testing.WebApplicationFactory<T>instead ofTUnit.AspNetCore.TestWebApplicationFactory<T>.TestWebApplicationFactory<T>and addsusing TUnit.AspNetCore;.TUnit.AspNetCore.Analyzers.CodeFixersproject, bundled into theTUnit.AspNetCorenupkg.examples/aspnet.mdandguides/distributed-tracing.md.Closes #5596.
Details
Direct inheritance silently loses:
ILoggeroutput from inside the app isn't routed to the right test output.TestContext.Currentwon't resolve inside request handlers.The analyzer reports on the base type location (not the class identifier) so the squiggle sits where the offending inheritance is declared. It ignores
TestWebApplicationFactory<T>itself. Transitive cases (class inherits a user type that inheritsTestWebApplicationFactory<T>) are implicitly safe: the direct base isn'tWebApplicationFactory, so the analyzer exits early.Test plan
TestWebApplicationFactory<T>TestWebApplicationFactory<T>inheritanceusing TUnit.AspNetCore;analyzers/dotnet/cs/in theTUnit.AspNetCorenupkg