Skip to content

Conversation

@thomhurst
Copy link
Owner

Summary

  • Add NUnit CollectionAssert migration support for all common methods:

    • Contains, DoesNotContain
    • IsEmpty, IsNotEmpty
    • AreEqual, AreNotEqual, AreEquivalent, AreNotEquivalent
    • AllItemsAreUnique, AllItemsAreNotNull
    • IsSubsetOf, IsSupersetOf, IsOrdered
  • Add NUnit StringAssert migration support for all methods:

    • Contains, DoesNotContain
    • StartsWith, DoesNotStartWith
    • EndsWith, DoesNotEndWith
    • IsMatch, DoesNotMatch
    • AreEqualIgnoringCase, AreNotEqualIgnoringCase
  • Fix MSTest IsSameReference -> IsSameReferenceAs method name (was using incorrect method name)

Test plan

  • All 109 NUnit migration tests pass (10 new tests added)
  • All 32 MSTest migration tests pass
  • All 42 xUnit migration tests pass (1 skipped for assembly attributes)

🤖 Generated with Claude Code

- Add NUnit CollectionAssert migration support for:
  - Contains, DoesNotContain
  - IsEmpty, IsNotEmpty
  - AreEqual, AreNotEqual, AreEquivalent, AreNotEquivalent
  - AllItemsAreUnique, AllItemsAreNotNull
  - IsSubsetOf, IsSupersetOf, IsOrdered

- Add NUnit StringAssert migration support for:
  - Contains, DoesNotContain
  - StartsWith, DoesNotStartWith
  - EndsWith, DoesNotEndWith
  - IsMatch, DoesNotMatch
  - AreEqualIgnoringCase, AreNotEqualIgnoringCase

- Fix MSTest IsSameReference -> IsSameReferenceAs method name

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

Summary

Adds NUnit migration support for CollectionAssert (11 methods) and StringAssert (9 methods), plus fixes MSTest IsSameReference method name.

Critical Issues

1. Missing TUnit assertion method: IsInAscendingOrder

NUnitMigrationCodeFixProvider.cs:565 maps:

"IsOrdered" when arguments.Count >= 1 => CreateTUnitAssertion("IsInAscendingOrder", arguments[0].Expression),

However, TUnit.Assertions only provides:

  • IsInOrder() - for collections implementing IComparable
  • IsOrderedBy(keySelector) - for custom ordering

There is no IsInAscendingOrder() extension method. This will cause compilation errors for migrated code.

Fix: Change to IsInOrder which is the correct TUnit equivalent:

"IsOrdered" when arguments.Count >= 1 => CreateTUnitAssertion("IsInOrder", arguments[0].Expression),

2. Missing TUnit assertion methods: DoesNotStartWith and DoesNotEndWith

NUnitMigrationCodeFixProvider.cs:590-593 maps:

"DoesNotStartWith" => CreateTUnitAssertion("DoesNotStartWith", ...)
"DoesNotEndWith" => CreateTUnitAssertion("DoesNotEndWith", ...)

However, TUnit.Assertions only provides:

  • StartsWith() and EndsWith()
  • No DoesNotStartWith() or DoesNotEndWith() methods exist

Fix: These assertions don't exist in TUnit yet. You should either:

  1. Remove these two conversions (and add TODO comments), OR
  2. Add these assertion methods to TUnit.Assertions first, THEN add migration support

Suggestions

  1. Test coverage gap: The new tests don't cover DoesNotStartWith, DoesNotEndWith, or all CollectionAssert methods. Consider adding tests for:

    • CollectionAssert.AreEqual, AreNotEqual, AllItemsAreNotNull, IsSubsetOf, IsSupersetOf
    • StringAssert.DoesNotStartWith, DoesNotEndWith
    • StringAssert.AreEqualIgnoringCase, AreNotEqualIgnoringCase
  2. MSTest fix is good ✅: Changing IsSameReferenceIsSameReferenceAs correctly aligns with TUnit.Assertions/Conditions/ReferenceEqualityAssertions.cs:10,47.

Previous Review Status

No previous comments found.

Verdict

⚠️ REQUEST CHANGES - Critical issues found (non-existent assertion methods will cause compilation errors)

…h/DoesNotEndWith assertions

- Change IsInAscendingOrder to IsInOrder (TUnit uses IsInOrder for ascending order)
- Add StringDoesNotStartWithAssertion and StringDoesNotEndWithAssertion
- Update tests to match new assertion names
- Migration now correctly converts NUnit Does.Not.StartWith and Does.Not.EndWith

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

Addressed Review Feedback

I've addressed the issues identified in the review:

1. IsInAscendingOrder → IsInOrder ✅

Changed all occurrences of IsInAscendingOrder to IsInOrder (TUnit's correct method name for ascending order).

2. DoesNotStartWith and DoesNotEndWith ✅

Instead of removing these conversions, I added the missing assertion methods to TUnit:

  • StringDoesNotStartWithAssertion - asserts that a string does NOT start with a substring
  • StringDoesNotEndWithAssertion - asserts that a string does NOT end with a substring

Both assertions follow the same pattern as existing string assertions and include:

  • IgnoringCase() modifier for case-insensitive comparison
  • WithComparison(StringComparison) modifier for custom comparison rules

Tests

All migration tests pass:

  • NUnit: 109 passed
  • MSTest: 32 passed
  • xUnit: 42 passed (1 skipped - unrelated)

@thomhurst
Copy link
Owner Author

Summary

Adds NUnit migration support for CollectionAssert and StringAssert methods, plus fixes MSTest IsSameReference method name and adds new string assertion methods to TUnit.

Critical Issues

None found ✅

Previous Review Status

All issues from the previous review have been addressed:

  1. IsInAscendingOrder → IsInOrder: Changed in 6 locations across NUnitMigrationCodeFixProvider.cs (lines 778, 780, 812, 980, 982, 1010) to use the correct TUnit method name.

  2. Missing DoesNotStartWith/DoesNotEndWith: Instead of removing these conversions, proper implementation added:

    • StringDoesNotStartWithAssertion - new assertion class with IgnoringCase() and WithComparison() modifiers
    • StringDoesNotEndWithAssertion - new assertion class with same modifiers
    • Both follow existing TUnit assertion patterns and are properly annotated with [AssertionExtension]

Additional Observations

TUnit Rules Compliance

  • No VSTest usage: Changes use Microsoft.Testing.Platform only
  • No blocking async: All async operations properly use Task.FromResult
  • AOT compatible: GetType() usage is only in exception messages (safe)
  • Dual-mode N/A: Changes only affect analyzers and assertions (not test discovery)
  • ℹ️ Snapshot tests N/A: No source generator output or public API breaking changes (only additions)

Code Quality

  • ✅ MSTest fix (IsSameReference → IsSameReferenceAs) correctly aligns with TUnit.Assertions
  • ✅ Argument order correctly handled (NUnit uses expected, actual order)
  • ✅ New assertions follow existing patterns (comparison modes, modifiers)
  • ✅ Test coverage: 10 new tests added covering key CollectionAssert and StringAssert methods

Minor Notes

  • Test coverage could be more comprehensive (only 10 tests for 20 methods), but key methods are covered
  • Methods not explicitly tested: AreEqual, AreNotEqual, AreNotEquivalent, AllItemsAreNotNull, IsSubsetOf, IsSupersetOf, IsNotEmpty, IsOrdered, DoesNotStartWith, DoesNotEndWith, AreEqualIgnoringCase, AreNotEqualIgnoringCase
  • However, the migration logic is straightforward and follows established patterns

Verdict

APPROVE - All critical issues resolved, TUnit rules followed, implementation is solid

@thomhurst
Copy link
Owner Author

Summary

Adds NUnit CollectionAssert and StringAssert migration support, plus fixes MSTest IsSameReferenceAs method name.

Critical Issues

None found ✅

Suggestions

Minor: Overly specific pattern matching in DoesNotMatch expectation

In TUnit.Assertions/Conditions/StringAssertions.cs around line 655 (in the new code):

if (expression.Contains(".DoesNotMatch(regex)") || expression.Contains(".DoesNotMatch(DoesNotMatch_") || expression.Contains(".DoesNotMatch(FindNumber"))

The FindNumber pattern seems overly specific. Consider using just the first two patterns (like the Matches assertion does at line 571), unless there's a specific test case that requires it.

TUnit Rules Compliance

Snapshot Testing (Rule 2): Public API changes correctly tracked with .verified.txt updates across all 4 target frameworks. No .received.txt files committed.

Code Quality:

  • New string assertions follow existing patterns consistently
  • Migration mappings correctly handle argument order differences between frameworks
  • MSTest bug fix (IsSameReferenceIsSameReferenceAs) is correct
  • AllItemsAreNotNullDoesNotContain(null) mapping is semantically equivalent

Test Coverage: PR description indicates all 183 migration tests pass (109 NUnit + 32 MSTest + 42 xUnit)

Verdict

APPROVE - No critical issues. The suggestion is a minor style preference that doesn't affect functionality.

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