Improve test framework migrations#6381
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 5 minor |
🟢 Metrics 0 complexity
Metric Results Complexity 0
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.
There was a problem hiding this comment.
Review: Improve test framework migrations (#6381)
Overall this is a solid, well-scoped change. The extraction of AddInvocationReplacement/AddExpressionReplacement into MigrationAnalyzer (and refactoring XUnitTwoPhaseAnalyzer.AnalyzeTestOutputHelperCalls to use the shared helper) is a good DRY cleanup — it removes a duplicated try/catch block and gives NUnit/MSTest a consistent way to register expression-level replacements. The is { Length: > 0 } x pattern-matching swaps for !string.IsNullOrEmpty(x) are a nice, low-risk modernization that also narrows nullability for the compiler.
Findings
1. ExpressionReplacement has no way to attach a warning/TODO, unlike AssertionConversion
AssertionConversion supports a TodoComment (used elsewhere in MigrationTransformer to leave a note in the generated code when a conversion may need manual review). ExpressionReplacement/AddExpressionReplacement has no equivalent — every expression rewrite is applied silently with full confidence.
That matters here because MSTestTwoPhaseAnalyzer maps both TestContext.TestDir and TestContext.DeploymentDirectory to TUnit.Core.TestContext.TestDirectory (which is just the test assembly's output directory). DeploymentDirectory is not always identical to the assembly directory — it's the deployment-item output directory (e.g. [DeploymentItem]-copied resources), which historically could diverge from the plain bin directory. For most modern MSTest-on-.NET projects the two coincide, but for anyone still relying on deployment items the migrated code will silently point somewhere different at runtime, with no compiler warning and no generated TODO to flag it.
Suggest reusing the TodoComment-style mechanism for ExpressionReplacement (or at minimum emitting a TODO comment specifically for the DeploymentDirectory → TestDirectory case), so users get a "verify this still points where you expect" nudge instead of a clean, silent build.
2. (Minor/low-confidence) Single-arg Assert.That(x) skips the boolean-type check that the 2-arg path applies
if (args.Count == 1)
return ConvertBooleanAssertThat(node, args[0], null);
if (args.Count >= 2 && IsMessageArgument(args[1]) && IsBooleanExpression(args[0].Expression))
return ConvertBooleanAssertThat(node, args[0], GetMessageArgument(args[1]));The two-argument branch verifies IsBooleanExpression(args[0]) before converting, but the single-argument branch does not. In practice Assert.That(x) with one argument is only valid in NUnit for bool/Func<bool>, so this is likely fine — but the Func<bool> case would still get rewritten to await Assert.That(() => x).IsTrue(), which needs a TestDelegate/deferred-invocation shape that may not match TUnit's fluent API the same way. Worth a quick test case if Func<bool> migration is in scope; otherwise not blocking.
Not blocking
TestContext.TestDirectoryas a pure alias forTestContext.OutputDirectory(public static string? TestDirectory => OutputDirectory;) does introduce two public names for the same value. Given issue #6338 explicitly asks for this API name to match NUnit/MSTest vocabulary for migration purposes, this seems intentional rather than an oversight.- Span-based node re-matching in
AddExpressionReplacement/AddInvocationReplacement(searchingcurrentRootbySpan) is the same pattern already used elsewhere in this file; since annotation-only replacements preserve node text/spans, this remains sound across the phases as far as I can tell.
Tests for the new behaviors (NUnit boolean overload, NUnit/MSTest TestContext shortcuts) are included and look reasonable. Didn't find correctness issues that would block merge — flagging (1) as worth addressing or at least discussing before merge, (2) as optional.
Summary
TestContext.TestDirectoryas the TUnit test assembly directory API.Assert.That(bool),TestContextoutput, directory, and attachment APIs.TestContextoutput, result-file, and directory APIs; keep xUnit output-helper migration on the shared invocation replacement path.Tests
dotnet test TUnit.Analyzers.Tests\TUnit.Analyzers.Tests.csproj --treenode-filter "/*/*/NUnitMigrationAnalyzerTests/*|/*/*/MSTestMigrationAnalyzerTests/*|/*/*/XUnitMigrationAnalyzerTests/*"dotnet test TUnit.PublicAPI\TUnit.PublicAPI.csproj --treenode-filter "/*/*/Tests/Core_Library_Has_No_API_Changes"Closes #6338