Skip to content

fix: xUnit code fixer bugs for Assert.Single, Assert.Collection, and Assert.All#4821

Merged
thomhurst merged 1 commit intomainfrom
fix/xunit-fixer-assertion-bugs
Feb 18, 2026
Merged

fix: xUnit code fixer bugs for Assert.Single, Assert.Collection, and Assert.All#4821
thomhurst merged 1 commit intomainfrom
fix/xunit-fixer-assertion-bugs

Conversation

@thomhurst
Copy link
Owner

Summary

Fixes three xUnit code fixer bugs reported in #4805, #4806, and #4811:

Both the assertion code fixer (XUnitAssertionCodeFixProvider) and the two-phase migration analyzer (XUnitTwoPhaseAnalyzer) are updated where applicable.

Test plan

  • New tests added for Assert.Single with and without predicate in assertion code fixer
  • Existing Assert_Collection_Adds_Todo_Comment test updated to match improved TODO message
  • All 52 assertion code fixer tests pass
  • All 268 two-phase migration analyzer tests pass

🤖 Generated with Claude Code

@claude
Copy link
Contributor

claude bot commented Feb 15, 2026

Code Review

I found one architectural issue that affects both code paths in this PR:

🔴 Missing using System.Linq; Directive

Location 1: TUnit.Assertions.Analyzers.CodeFixers/XUnitAssertionCodeFixProvider.cs:192
Location 2: TUnit.Analyzers.CodeFixers/TwoPhase/XUnitTwoPhaseAnalyzer.cs:356

Issue: Both the code fixer and two-phase analyzer generate .Where(predicate) when converting Assert.Single(collection, predicate) to Assert.That(collection.Where(predicate)).HasSingleItem(), but neither adds the required using System.Linq; directive.

Impact: If the user's source file doesn't already have using System.Linq;, the generated code will fail to compile with error CS1061 ("'IEnumerable' does not contain a definition for 'Where'").

Root Cause Analysis:

  1. The XUnitAssertionCodeFixProvider generates the .Where() call directly via SyntaxFactory.ParseExpression() without modifying the using directives.

  2. The XUnitTwoPhaseAnalyzer generates the .Where() pattern, but:

    • The AnalyzeUsings() method (lines 1587-1591) only removes xUnit prefixes
    • MigrationHelpers.AddTUnitUsings() only conditionally adds System.Threading.Tasks and System.IO
    • There's no logic to add System.Linq anywhere in the pipeline
  3. The test suite doesn't catch this because:

    • CSharpCodeFixVerifier.cs sets CompilerDiagnostics = CompilerDiagnostics.None (line 110)
    • The Roslyn test framework validates syntax transformation but doesn't compile the result
    • The test expectations themselves don't include using System.Linq;

Suggested Architectural Fix:

This should be fixed at the infrastructure level rather than case-by-case:

  1. For XUnitAssertionCodeFixProvider: Add using directive management similar to AwaitAssertionCodeFixProvider.cs:76-78:

    // After generating the expression with .Where()
    document = await document.AddUsingDirectiveIfNotExistsAsync("System.Linq");
  2. For XUnitTwoPhaseAnalyzer: Extend MigrationHelpers.AddTUnitUsings() to track which namespaces are needed based on the conversion operations used (similar to how it tracks System.Threading.Tasks).

  3. For Tests: Consider enabling compiler diagnostics for a subset of tests to catch these issues, or add a dedicated test that verifies the fixed code compiles in a minimal project without pre-existing using directives.

Why This Matters:

Automated migration tools should generate code that compiles without manual intervention. This bug breaks that promise - users would discover the compilation error only after running the fixer, then need to manually add using System.Linq;. This undermines trust in the migration tooling.

The fix is straightforward using existing infrastructure (AddUsingDirectiveIfNotExistsAsync), so this should be addressed before merging to maintain the quality bar for the migration experience.


Overall Assessment: The bug fixes in this PR are good and well-tested for the logic changes. However, the missing using directive is a systemic issue that will cause compilation failures for users. I recommend fixing this before merge.

…Assert.All (#4805, #4806, #4811)

- Assert.Single with predicate now converts to .Where(predicate).HasSingleItem()
  instead of dropping the predicate lambda
- Assert.Collection TODO comment now preserves element inspector code so
  assertions aren't silently lost during migration
- Assert.All with multi-statement lambda bodies no longer produces invalid
  TUnit code; skips conversion when predicate can't be extracted

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@thomhurst
Copy link
Owner Author

The review concern about missing using System.Linq; is now resolved — after rebasing onto main (which includes #4832 feat: add HasSingleItem(predicate) overload), the code no longer generates .Where(predicate).HasSingleItem(). Instead, both the code fixer and two-phase analyzer now generate HasSingleItem(predicate) directly, which doesn't require System.Linq.

All tests pass after rebase (13 assertion code fixer tests, 67 migration analyzer tests).

@claude
Copy link
Contributor

claude bot commented Feb 18, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

The PR successfully fixes three xUnit migration bugs:

  1. Assert.Single predicate preservation - Predicates are no longer silently dropped
  2. Assert.Collection inspector preservation - Element inspectors are now preserved in detailed TODO comments
  3. Assert.All invalid code prevention - Multi-statement lambda bodies are now handled safely

All logic changes are correct and improve the migration tooling without introducing any regressions.

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