refactor: implement two-phase architecture for xUnit migration code fixer

[thomhurst]

Summary

This PR introduces a robust two-phase migration architecture for the xUnit code fixer that separates semantic analysis from syntax transformation, eliminating semantic model staleness issues.

• Phase 1 (Analysis): Collects all conversion targets while the semantic model is valid
• Phase 2 (Transformation): Applies pure syntax transformations using SyntaxAnnotations
• Result: 95% code reduction in XUnitMigrationCodeFixProvider (1,849 → 87 lines)

Key Improvements

Feature	Before	After
Assert.Throws/ThrowsAsync	Converted unnecessarily	Kept unchanged (TUnit compatible)
.Because() messages	Lost	Preserved for True/False
[Before(Test)]/[After(Test)]	Incorrect parsing	Properly generated
Semantic model issues	Frequent staleness errors	Eliminated via two-phase design

New Files

• Base/TwoPhase/MigrationAnalyzer.cs - Base class for framework analyzers
• Base/TwoPhase/MigrationTransformer.cs - Syntax transformation engine
• Base/TwoPhase/ConversionPlan.cs - Data structures for conversion targets
• TwoPhase/XUnitTwoPhaseAnalyzer.cs - xUnit-specific implementation
• Base/MigrationContext.cs - Error tracking for legacy fallback

xUnit Conversions Supported

• ✅ 27+ assertion types (Equal, True, Null, Throws, Contains, etc.)
• ✅ All major attributes ([Fact], [Theory], [InlineData], [MemberData], etc.)
• ✅ IAsyncLifetime → [Before(Test)]/[After(Test)] or IAsyncInitializer
• ✅ TheoryData → IEnumerable
• ✅ Record.Exception → try-catch blocks
• ✅ ITestOutputHelper → Console.WriteLine
• ✅ IClassFixture → ClassDataSource attribute

Test plan

• All 551 analyzer tests pass (1 skipped for assembly attributes)
• Verified xUnit migration tests specifically (212 tests, 208 passed, 4 skipped)
• Kitchen sink comprehensive test validates complex scenarios
• Legacy rewriter code removed, pass-through implementations verified

[thomhurst]

Summary

This PR refactors the xUnit migration code fixer to use a two-phase architecture that separates semantic analysis from syntax transformation, reducing code from 1,849 to 87 lines (95% reduction) and eliminating semantic model staleness issues.

Critical Issues

None found ✅

Suggestions

1. Exception Handling Granularity

The PR adds broad catch (Exception ex) blocks throughout the two-phase implementation. While this provides robustness for migration failures, consider that some exceptions (e.g., OutOfMemoryException, StackOverflowException) should potentially abort rather than being silently recorded. Impact is low since code fixers are isolated operations.

2. Allocation Patterns

Multiple .ToList() calls in analysis phase occur during code fixer execution (not hot paths). This is acceptable since code fixers run on-demand, not during test execution.

3. Testing Coverage

The PR description states all 551 analyzer tests pass. No .verified.txt or .received.txt changes in the diff suggests output is identical to before (expected for refactor).

TUnit Rule Compliance

✅ Dual-Mode: N/A - Changes are to analyzer/code fixer, not runtime engine
✅ Snapshot Testing: No .received.txt files committed
✅ No VSTest: No VSTest dependencies detected
✅ Performance First: Allocations occur in code fixer (not hot path)
✅ AOT Compatible: N/A - Analyzers run at design time, not in AOT context

Verdict

✅ APPROVE - No critical issues

This is a well-executed refactoring that eliminates semantic model staleness bugs through proper separation of concerns, reduces complexity dramatically, maintains backward compatibility, and follows modern C# patterns with SyntaxAnnotation for node tracking.

[thomhurst]

Summary

This PR refactors the xUnit migration code fixer to use a two-phase architecture (semantic analysis → syntax transformation), eliminating semantic model staleness issues and reducing XUnitMigrationCodeFixProvider by 95% (1,849 → 87 lines).

Critical Issues

None found ✅

TUnit Rules Compliance

✅ No dual-mode requirements - Changes isolated to code fixer infrastructure
✅ No snapshot updates needed - No .verified.txt/.received.txt files modified
✅ No VSTest usage - Clean ✓
✅ No async blocking - No .Result or .GetAwaiter().GetResult() ✓
✅ AOT compatible - Syntax-based analysis (not runtime reflection) ✓

Observations

Architecture improvements:

• Two-phase design cleanly separates concerns (analysis with semantic model → transformation without)
• SyntaxAnnotation tracking elegant solution for node identity across tree modifications
• MigrationContext provides graceful degradation for frameworks not yet migrated

Code quality:

• Massive simplification (1,762 lines removed from XUnitMigrationCodeFixProvider)
• Test suite validates the refactor (551 tests passing)
• Legacy fallback path ensures existing frameworks continue working

Minor notes:

• The new architecture files (MigrationAnalyzer, MigrationTransformer, etc.) are well-structured
• ConversionPlan as data structure is clean separation of analysis from transformation
• Error handling via MigrationContext.RecordFailure() provides good diagnostics

Verdict

✅ APPROVE - Well-designed refactor with no TUnit rule violations or critical issues detected

[thomhurst]

Summary

This PR introduces a two-phase migration architecture for the xUnit code fixer, separating semantic analysis (Phase 1) from syntax transformation (Phase 2) to eliminate semantic model staleness issues.

Critical Issues

None found ✅

Suggestions

1. Missing Assertion Tests for New Event Assertions

The PR includes new event assertion methods (PropertyChanged, PropertyChangedAsync, Raises, RaisesAsync, RaisesAny, RaisesAnyAsync) in TUnit.Assertions/Extensions/Assert.cs but these appear to be primarily tested in EventAssertionTests.cs. Consider whether additional test coverage is needed for edge cases like:

• Null object handling
• Multiple event subscriptions
• Event handler exceptions

2. New StrictEquality Assertions

The new IsStrictlyEqualTo and IsNotStrictlyEqualTo assertions (TUnit.Assertions/Assertions/StrictEqualityAssertions.cs:16-24) use object.Equals which may have surprising behavior with value types due to boxing. Consider adding a doc comment warning about this, or examples showing when to use strict vs regular equality.

3. Xunit.Sdk Using Removal

Good catch adding Xunit.Sdk to the usings removal list (TUnit.Analyzers/Migrators/Base/MigrationHelpers.cs:183). This prevents leftover xUnit SDK references.

Observations

Architecture Quality

The two-phase architecture is well-designed:

• Phase 1 (Analysis): Collects all semantic information upfront while the semantic model is valid
• Phase 2 (Transformation): Pure syntax operations using SyntaxAnnotations to track nodes
• Proper separation of concerns with abstract base classes (MigrationAnalyzer, MigrationTransformer) and framework-specific implementations

Performance Considerations

• Uses span-based lookups (TextSpan) to correlate nodes between original and modified trees - efficient
• Pre-collects interface-implementing methods to avoid repeated semantic queries - good caching strategy
• No obvious allocations in hot paths

Error Handling

• Graceful degradation: failures are recorded in ConversionPlan.Failures rather than aborting
• Legacy fallback path preserved for frameworks not yet migrated
• TODO comments added to generated code for manual review - good UX

Test Updates

Test changes show the migration improvements:

• Skip parameter ordering fix (now correctly placed after other parameters)
• Attribute ordering improvements (e.g., ClassDataSource before NotInParallel)
• Better handling of Is.InstanceOf constraint extraction
• Removal of extra await in async assertion chains

Verdict

✅ APPROVE - No critical issues

This is a solid refactoring that addresses a real problem (semantic model staleness) with a clean architectural solution. The two-phase approach is well-executed, maintains backward compatibility through the legacy fallback path, and improves code quality significantly (95% reduction in XUnitMigrationCodeFixProvider as noted in the PR description).

[thomhurst]

Summary

Implements a robust two-phase architecture for the xUnit migration code fixer, separating semantic analysis from syntax transformation to eliminate semantic model staleness issues.

Critical Issues

Missing PublicAPI Snapshot Updates ⚠️

This PR adds 6 new public methods to TUnit.Assertions/Extensions/Assert.cs:

• PropertyChanged
• PropertyChangedAsync
• Raises<T>
• RaisesAsync<T>
• RaisesAny<T>
• RaisesAnyAsync<T>

According to TUnit Rule #2 (Snapshot Testing), changes to public APIs require running snapshot tests and updating .verified.txt files:

dotnet test TUnit.PublicAPI
# Then review and accept the changes (Linux/macOS)
for f in *.received.txt; do mv "$f" "$\{f%.received.txt\}.verified.txt"; done
git add *.verified.txt

See CLAUDE.md and .claude/docs/mandatory-rules.md for details.

Suggestions

None - the two-phase architecture is well-designed and the code quality is excellent.

Verdict

⚠️ REQUEST CHANGES - Missing required PublicAPI snapshot updates for new public assertion methods.

[thomhurst]

Summary

Implements a two-phase architecture for the xUnit migration code fixer, separating semantic analysis from syntax transformation and reducing XUnitMigrationCodeFixProvider by 95% (1,849 → 87 lines).

Critical Issues

None found ✅

Previous Review Status

Previous review requested PublicAPI snapshot updates - this has been ADDRESSED ✅

• All 4 .verified.txt files have been updated for the new assertion methods
• No .received.txt files committed (correct per TUnit rules)

TUnit Rules Compliance

✅ Dual-Mode: N/A - Changes isolated to code fixer/analyzer infrastructure
✅ Snapshot Testing: .verified.txt files properly updated, no .received.txt committed
✅ No VSTest: No VSTest dependencies detected
✅ Performance First: Allocations occur in code fixer (not hot paths)
✅ AOT Compatible: N/A - Analyzers run at design time
✅ No async blocking: .Wait() usage is appropriate (methods with ref/out parameters that cannot be async)

Observations

Architecture Quality:

• Clean separation: Phase 1 collects semantic info upfront, Phase 2 does pure syntax transformations
• SyntaxAnnotation tracking eliminates semantic model staleness issues
• Proper abstraction with base classes (MigrationAnalyzer, MigrationTransformer)
• Graceful degradation via MigrationContext for frameworks not yet migrated

New Assertion APIs:

• Event assertions (PropertyChanged, Raises, etc.) added with comprehensive tests
• StrictEquality assertions (IsStrictlyEqualTo/IsNotStrictlyEqualTo) for object.Equals semantics
• All properly documented and public API snapshots updated

Test Coverage:

• 551 analyzer tests passing (1 skipped for assembly attributes)
• 212 xUnit-specific migration tests (208 passed, 4 skipped)
• Kitchen sink comprehensive test validates complex scenarios

Verdict

✅ APPROVE - Excellent refactoring with no critical issues or TUnit rule violations

This is a well-executed architectural improvement that solves real semantic model staleness bugs while dramatically simplifying the codebase. The two-phase design is clean, maintainable, and follows modern C# patterns.