Skip to content

Conversation

@thomhurst
Copy link
Owner

Summary

  • Add FileAssert conversion support for NUnit migration
  • Add DirectoryAssert conversion support for NUnit migration
  • Add ExpectedException attribute conversion to Assert.ThrowsAsync()
  • Automatically add System.IO using when File/Directory classes are used

Changes

FileAssert Support

  • FileAssert.Exists(path)Assert.That(File.Exists(path)).IsTrue()
  • FileAssert.DoesNotExist(path)Assert.That(File.Exists(path)).IsFalse()
  • FileAssert.AreEqual(expected, actual)Assert.That(File.ReadAllBytes(actual)).IsEquivalentTo(File.ReadAllBytes(expected))
  • FileAssert.AreNotEqual(expected, actual)Assert.That(File.ReadAllBytes(actual)).IsNotEquivalentTo(File.ReadAllBytes(expected))

DirectoryAssert Support

  • DirectoryAssert.Exists(path)Assert.That(Directory.Exists(path)).IsTrue()
  • DirectoryAssert.DoesNotExist(path)Assert.That(Directory.Exists(path)).IsFalse()
  • DirectoryAssert.AreEqual/AreNotEqual → Directory content comparison

ExpectedException Support

  • Converts [ExpectedException(typeof(T))] attribute to Assert.ThrowsAsync<T>() wrapper
  • Handles both sync and async test methods (uses async lambda when await expressions are present)

Test plan

  • Added tests for FileAssert.Exists, DoesNotExist, AreEqual
  • Added tests for DirectoryAssert.Exists, DoesNotExist
  • Added tests for ExpectedException (sync and async)
  • Verified all NUnit migration tests pass (91 tests)
  • Verified all MSTest migration tests pass (32 tests)
  • Verified all XUnit migration tests pass (42 tests)

🤖 Generated with Claude Code

…pport

- Add FileAssert conversion (Exists, DoesNotExist, AreEqual, AreNotEqual)
- Add DirectoryAssert conversion (Exists, DoesNotExist, AreEqual, AreNotEqual)
- Add ExpectedException attribute conversion to Assert.ThrowsAsync<T>()
- Automatically add System.IO using when File/Directory classes are used
- Support async lambdas in ExpectedException conversion when await expressions exist

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@thomhurst
Copy link
Owner Author

Summary

Adds NUnit migration support for FileAssert, DirectoryAssert, and ExpectedException attributes.

Critical Issues

1. DirectoryAssert.AreEqual semantic difference

The conversion of DirectoryAssert.AreEqual to Directory.GetFiles(...).IsEquivalentTo(...) only compares file names (paths), not file contents. NUnit's DirectoryAssert.AreEqual recursively compares both directory structure AND file contents.

Location: TUnit.Analyzers.CodeFixers/NUnitMigrationCodeFixProvider.cs:1584-1592

Impact: Tests using DirectoryAssert.AreEqual will behave differently after migration. A directory with files of the same name but different contents would pass the migrated test but fail the original NUnit test.

Recommendation:

  • Add a comment in the generated code warning users about this limitation, OR
  • Use a more comprehensive approach that actually compares file contents, OR
  • Document this known limitation in migration docs

Example of what could go wrong:

// NUnit - fails if file1.txt contents differ
DirectoryAssert.AreEqual(expectedDir, actualDir);

// Migrated TUnit - only checks file names exist, not contents
await Assert.That(Directory.GetFiles(actualDir, "*", SearchOption.AllDirectories))
    .IsEquivalentTo(Directory.GetFiles(expectedDir, "*", SearchOption.AllDirectories));

Suggestions

None - the rest of the implementation looks solid. The ExpectedException conversion is well-handled with proper async lambda detection, and System.IO using statements are correctly added.

Previous Review Status

No previous comments.

Verdict

⚠️ REQUEST CHANGES - The DirectoryAssert.AreEqual semantic difference is a critical behavior change that could silently break tests after migration.

… difference

Address review feedback: NUnit's DirectoryAssert.AreEqual compares both file paths
AND file contents recursively. The TUnit migration only compares file paths.
Add a TODO comment to warn users about this limitation.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@thomhurst
Copy link
Owner Author

Summary

Adds NUnit migration support for FileAssert, DirectoryAssert, and ExpectedException attributes.

Critical Issues

None found - The TODO comment in the generated code adequately addresses the DirectoryAssert.AreEqual limitation.

Suggestions

  1. Consider handling ExpectedMessage parameter for ExpectedException attribute
  2. Use semantic model type info instead of naming heuristics for FileInfo/DirectoryInfo detection

Verdict

APPROVE - No critical issues blocking merge

- Add FileInfo.HasSameContentAs(other) assertion for binary file comparison
- Add DirectoryInfo.HasSameStructureAs(other) for path structure comparison
- Add DirectoryInfo.IsEquivalentTo(other) for full directory equivalence
- Add DirectoryInfo.IsNotEquivalentTo(other) negated version
- Simplify NUnit FileAssert/DirectoryAssert migration to use new assertions
- Update AddSystemIOUsing to detect FileInfo/DirectoryInfo object creation

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@thomhurst
Copy link
Owner Author

Summary

Adds NUnit migration support for FileAssert, DirectoryAssert, and ExpectedException attributes.

Critical Issues

None found

Suggestions

  1. Consider handling ExpectedMessage parameter for ExpectedException attribute
  2. Type detection heuristics could be more robust using semantic model
  3. HasSameContentAs assertion lacks negated version for consistency

Previous Review Status

Previous review raised critical issue about DirectoryAssert.AreEqual semantic differences. This has been addressed with proper IsEquivalentTo extension that compares both structure AND file contents.

Compliance with TUnit Rules

All applicable rules satisfied: No VSTest dependencies, uses modern C# features appropriately.

Verdict

APPROVE - No critical issues. Suggestions are minor improvements for follow-up PRs.

- Add FileInfo.DoesNotHaveSameContentAs(other) assertion for negated file comparison
- Update NUnit migration to use DoesNotHaveSameContentAs for FileAssert.AreNotEqual
- Add test for FileAssert.AreNotEqual migration

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@thomhurst
Copy link
Owner Author

Summary

Adds NUnit migration support for FileAssert, DirectoryAssert, and ExpectedException attributes with proper file content comparison.

Critical Issues

None found ✅

Suggestions

1. Performance concern: File.ReadAllBytes() for large files

The HasSameContentAs, DoesNotHaveSameContentAs, and IsEquivalentTo assertions load entire files into memory using File.ReadAllBytes(). This could cause memory issues when comparing large files (e.g., GB-sized files).

Location: TUnit.Assertions/Conditions/FileSystemAssertions.cs:218, 258, 407

Suggestion: Consider using streaming comparison for large files, or document the memory limitation. For example:

// Stream-based comparison for large files
using (var stream1 = File.OpenRead(value.FullName))
using (var stream2 = File.OpenRead(expected.FullName))
{
    byte[] buffer1 = new byte[8192];
    byte[] buffer2 = new byte[8192];
    // Compare chunks...
}

Impact: Low - Most test files are small. This is a nice-to-have optimization for future consideration.

2. Type detection heuristics

The CreateFileInfoExpression and CreateDirectoryInfoExpression methods use string-based heuristics (.Contains("path"), .EndsWith("Path")) to detect whether an expression is a string path or FileInfo/DirectoryInfo object.

Location: TUnit.Analyzers.CodeFixers/NUnitMigrationCodeFixProvider.cs:1513-1519, 1596-1602

Issue: This could misfire on variables like mypath, filepath, etc. that are actually FileInfo/DirectoryInfo objects.

Suggestion: Use semantic model to check actual types instead of naming heuristics (mentioned in previous reviews). However, this is already a known limitation and the heuristic works for common cases.

Impact: Low - Edge case that rarely occurs in practice. The migration tool is already very functional.

Previous Review Status

✅ All previous critical issues have been addressed:

  • DirectoryAssert.AreEqual now properly compares both structure AND file contents via IsEquivalentTo
  • DoesNotHaveSameContentAs assertion added for FileInfo negation
  • Comprehensive tests added for all migrations

TUnit Rules Compliance

Dual-Mode: N/A - No source generator or reflection mode changes
Snapshot Testing: N/A - No source generator output changed
No VSTest: No VSTest dependencies detected
Performance First: Reasonable performance for typical test files
AOT Compatible: No reflection used

Verdict

APPROVE - No critical issues. The suggestions are minor optimizations that can be addressed in future PRs if needed. The PR successfully implements comprehensive NUnit migration support with correct semantics.

Path.GetRelativePath is not available in netstandard2.0, so use a
Uri-based implementation as a polyfill for that target framework.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@thomhurst
Copy link
Owner Author

Summary

Adds NUnit migration support for FileAssert, DirectoryAssert, and ExpectedException attributes with proper file content comparison.

Critical Issues

1. Infinite recursion in GetRelativePath method

Location: TUnit.Assertions/Conditions/FileSystemAssertions.cs:515

Issue: The #else branch of the GetRelativePath method calls itself recursively without any change:

#else
    return GetRelativePath(relativeTo, path);  // ← Infinite recursion!
#endif

This should call Path.GetRelativePath(relativeTo, path) instead:

#else
    return Path.GetRelativePath(relativeTo, path);
#endif

Impact: This will cause a StackOverflowException on non-netstandard2.0 targets (.NET 6+, which is the primary target for TUnit).

Suggestions

1. Performance: File.ReadAllBytes() loads entire files into memory

Locations:

  • TUnit.Assertions/Conditions/FileSystemAssertions.cs:218 (HasSameContentAs)
  • TUnit.Assertions/Conditions/FileSystemAssertions.cs:258 (DoesNotHaveSameContentAs)
  • TUnit.Assertions/Conditions/FileSystemAssertions.cs:407-408 (IsEquivalentTo)
  • TUnit.Assertions/Conditions/FileSystemAssertions.cs:482-483 (IsNotEquivalentTo)

Concern: For large files (GB-sized), this could cause memory issues. Consider streaming comparison for better memory efficiency (though most test files are small, so low priority).

2. Early optimization opportunity

Location: TUnit.Assertions/Conditions/FileSystemAssertions.cs:218

The code already checks file sizes match before reading bytes in HasSameContentAs, which is good. However, in IsEquivalentTo (line 407), the size check happens after reading both files. Consider checking sizes first to short-circuit before the expensive I/O.

Previous Review Status

The author has already addressed previous critical issues in self-review:
✅ DirectoryAssert.AreEqual now properly compares structure AND contents via IsEquivalentTo
✅ DoesNotHaveSameContentAs assertion added for negation
✅ Comprehensive tests added

TUnit Rules Compliance

Dual-Mode: N/A - No changes to core engine metadata collection
Snapshot Testing: N/A - Migration code doesn't affect source generator output
No VSTest: No VSTest dependencies detected
⚠️ Performance First: ReadAllBytes for entire files acceptable for test scenarios
AOT Compatible: No reflection used in new code

Verdict

⚠️ REQUEST CHANGES - Critical bug: infinite recursion in GetRelativePath will crash on .NET 6+ targets. Must be fixed before merge.

…ertions

Add new public API entries for:
- FileInfo.DoesNotHaveSameContentAs
- FileInfo.HasSameContentAs
- DirectoryInfo.HasSameStructureAs
- DirectoryInfo.IsEquivalentTo
- DirectoryInfo.IsNotEquivalentTo

Co-Authored-By: Claude Opus 4.5 <[email protected]>
The #else branch was calling GetRelativePath recursively instead of
Path.GetRelativePath, which would cause a StackOverflowException on
non-netstandard2.0 targets.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@thomhurst
Copy link
Owner Author

Summary

This PR adds NUnit migration support for FileAssert, DirectoryAssert, and ExpectedException attributes, converting them to TUnit's equivalent assertion patterns.

Critical Issues

None found ✅

Suggestions

Performance: Consider async file I/O for large files

The new file comparison assertions use synchronous I/O operations (File.ReadAllBytes) which could block for large files:

Location: TUnit.Assertions/Conditions/FileSystemAssertions.cs

var actualBytes = File.ReadAllBytes(value.FullName);
var expectedBytes = File.ReadAllBytes(expected.FullName);

Suggestion: For better performance with large files, consider using async I/O:

  • File.ReadAllBytesAsync
  • Change return type to Task<AssertionResult>
  • Use streaming comparison for very large files to reduce memory usage

This is not blocking since:

  • It matches NUnit's synchronous behavior (migration goal)
  • Test files are typically small
  • This is test code, not a hot path

But it could be a future enhancement for better scalability.

Code Quality: Directory comparison allocates full file lists

The IsEquivalentTo and IsNotEquivalentTo methods enumerate all files into lists:

var actualFiles = value.EnumerateFiles("*", SearchOption.AllDirectories)
    .Select(f => Path.GetRelativePath(value.FullName, f.FullName))
    .OrderBy(p => p)
    .ToList();

Minor optimization opportunity: For large directory trees, this could be memory-intensive. Consider using SequenceEqual on ordered enumerables without .ToList() for the equality check, only materializing lists when building error messages.

Again, not blocking - directory comparisons in tests are typically small.

Review Checklist

Snapshot Testing: .verified.txt files properly updated (not .received.txt)
No VSTest: No Microsoft.VisualStudio.TestPlatform references
AOT Compatible: No reflection requiring [DynamicallyAccessedMembers]
Public API: Snapshot files updated for all target frameworks
Test Coverage: Comprehensive tests for all new conversions (FileAssert, DirectoryAssert, ExpectedException)
TUnit Patterns: Uses preferred [GenerateAssertion] pattern
Dual-Mode: Not applicable (analyzer/assertion code, not core engine)

Verdict

APPROVE - No critical issues

This is a solid implementation of NUnit migration support. The suggestions above are minor performance optimizations that could be considered as future enhancements but don't block this PR. The code follows TUnit patterns, has excellent test coverage, and properly updates all snapshot files.

@thomhurst
Copy link
Owner Author

Summary

Adds NUnit migration support for FileAssert, DirectoryAssert, and ExpectedException attributes.

Critical Issues

None found ✅

Previous Review Status

The author (thomhurst) conducted thorough self-review and addressed all critical issues:

  • ✅ DirectoryAssert.AreEqual semantic differences resolved - now uses IsEquivalentTo with proper file content comparison
  • ✅ FileInfo.DoesNotHaveSameContentAs assertion added for negated comparisons
  • Infinite recursion bug in GetRelativePath fixed (commit 4ce827c) - the #else branch now correctly calls Path.GetRelativePath instead of recursively calling itself
  • ✅ Public API snapshots properly updated (.verified.txt files committed, no .received.txt files)

TUnit Rules Compliance

Dual-Mode: N/A - No changes to core engine metadata collection
Snapshot Testing: Public API snapshots properly updated across all target frameworks
No VSTest: No VSTest dependencies detected
Performance First: File.ReadAllBytes acceptable for test file scenarios (most test files are small)
AOT Compatible: No reflection used; all code is AOT-compatible
Modern C#: Uses modern patterns including [GenerateAssertion], pattern matching, and collection expressions

Code Quality Observations

Strengths:

  • Comprehensive test coverage for all migration scenarios
  • Proper handling of both sync and async code in ExpectedException conversion
  • Good use of [GenerateAssertion] attribute for FileSystemAssertions
  • Netstandard2.0 polyfill for Path.GetRelativePath is well-implemented
  • Clear XML documentation
  • Proper error messages with helpful context

Minor notes (not blocking):

  • File.ReadAllBytes loads entire files into memory - acceptable for test scenarios, though streaming could be considered for future optimization with very large test files
  • Type detection heuristics (checking for "Path" suffix) work well for common cases but could theoretically misfire on edge cases - acceptable trade-off for migration tool

Verdict

APPROVE - Excellent implementation. All critical issues have been resolved, TUnit rules are followed, and the code is production-ready. The PR successfully adds comprehensive NUnit migration support with correct semantics.

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.

2 participants