Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #4040

…lder for further collection assertion chaining
Copilot AI review requested due to automatic review settings December 10, 2025 18:48
@thomhurst thomhurst enabled auto-merge (squash) December 10, 2025 18:48
@claude
Copy link
Contributor

claude bot commented Dec 10, 2025

Pull Request Review

Summary

This PR successfully fixes issue #4040 by redesigning the Count() assertion API to return collection-aware types that preserve the collection context for proper chaining.

Strengths

  1. Excellent API Design - CollectionCountSource and CollectionCountEqualsAssertion properly inherit from CollectionAssertionBase enabling fluent chaining
  2. Comprehensive Test Coverage - Tests basic comparisons, inner assertions, chaining, and the exact failing example from issue Broken API to chain collection assertions #4040
  3. Proper Snapshot Updates - Public API snapshots updated for all target frameworks
  4. Good Documentation - Clear XML docs, updated usage examples, helpful comments

Critical Issue

Silent Exception Swallowing (Lines 198-206, 303-311 in CollectionCountSource.cs)

Bare catch blocks swallow ALL exceptions including critical ones like OutOfMemoryException, StackOverflowException, ThreadAbortException, and OperationCanceledException.

Must fix: Use specific exception handling or rethrow critical exceptions.

Should Fix

  1. Incomplete Error Messages (Lines 235, 310) - Include expected values and comparison type in failure messages
  2. Missing Rationale (Line 151 CollectionAssertionBase.cs) - Add comment explaining why OverloadResolutionPriority is -1

Minor Issues

  1. Multiple Enumerations - Collections may be enumerated multiple times when chained (typical for assertion libraries, consider documenting)
  2. Unused Field - _actualCount field could be used in error messages or made local

Conclusion

This is a well-designed fix with clean architecture and comprehensive tests. With the critical exception handling issue fixed, this PR will be ready to merge.

Recommendation: Request changes to address bare catch blocks, then approve.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes issue #4040 by refactoring the Count() assertion methods to preserve the collection type, enabling fluent chaining of additional collection assertions. Previously, count assertions returned an int-focused assertion builder that broke the collection context. Now, count assertions return collection-aware builders that allow patterns like .Count().IsEqualTo(5).And.Contains(3).

Key changes:

  • Replaced CollectionCountValueAssertion<TCollection, TItem> (which extended ValueAssertion<int>) with CollectionCountSource<TCollection, TItem> (returns collection-aware assertions)
  • Introduced CollectionCountEqualsAssertion<TCollection, TItem> that inherits from CollectionAssertionBase to preserve collection type
  • Removed deprecated Count(Func<TItem, bool> predicate) method (was marked obsolete)
  • Added CollectionCountWithInlineAssertionAssertion<TCollection, TItem> for inline count assertions like Count(c => c.IsEqualTo(5))

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
TUnit.Assertions/Sources/CollectionAssertionBase.cs Updated Count() methods to return collection-preserving assertion types; removed obsolete predicate-based Count() method
TUnit.Assertions/Conditions/CollectionCountValueAssertion.cs Deleted - replaced with new architecture that preserves collection type
TUnit.Assertions/Conditions/CollectionCountSource.cs New file implementing collection-preserving count assertions with various comparison operators (IsEqualTo, IsGreaterThan, etc.)
TUnit.Assertions.Tests/CollectionAssertionTests.cs Added comprehensive tests for collection chaining after count assertions; removed obsolete predicate-based count test
docs/docs/assertions/collections.md Added documentation section demonstrating count assertion chaining with examples
TUnit.PublicAPI/Tests.Assertions_Library_Has_No_API_Changes.*.verified.txt Updated API snapshots across all .NET versions (4.7, 8.0, 9.0, 10.0) reflecting the breaking API changes

Comment on lines +203 to +206
catch
{
// Item did not satisfy the assertion, don't count it
}
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bare catch block swallows all exceptions without distinguishing between assertion failures and other errors (e.g., NullReferenceException, InvalidOperationException). This makes debugging difficult when unexpected errors occur during item evaluation.

Consider catching specific exception types like AssertionException, or at minimum, add logging or re-throw unexpected exceptions. This pattern is used elsewhere in the codebase (e.g., CollectionAssertions.cs lines 675, 742) where catch blocks catch Exception explicitly and provide error context.

Copilot uses AI. Check for mistakes.
Comment on lines +307 to +311
catch
{
// Count assertion failed
return AssertionResult.Failed($"count was {_actualCount}");
}
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bare catch block swallows all exceptions without distinguishing between assertion failures and other errors. This makes debugging difficult when unexpected errors occur during count assertion evaluation.

Consider catching specific exception types like AssertionException, or at minimum, add logging or re-throw unexpected exceptions to provide better diagnostics when genuine errors occur.

Copilot uses AI. Check for mistakes.
// Test with lambda-wrapped collection
await Assert.That(() => items).Count(item => item.IsGreaterThan(2)).IsEqualTo(3);
}

Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new collection chaining functionality introduced in this PR lacks test coverage for failure scenarios. While the happy path tests verify that chaining works when assertions pass, there are no tests verifying:

  1. Error messages when count assertions fail (e.g., expected 5 but found 3)
  2. Chained assertions after a failed count assertion
  3. Null collection handling with chained assertions
  4. Exception handling during item assertion evaluation in Count(item => ...)

Consider adding negative test cases like:

  • Count assertion fails with descriptive error message
  • Count with item filter fails and shows correct count
  • Chained assertion after count shows proper error context

Copilot uses AI. Check for mistakes.
@thomhurst thomhurst disabled auto-merge December 10, 2025 19:15
@thomhurst thomhurst merged commit ec0347e into main Dec 10, 2025
17 of 19 checks passed
@thomhurst thomhurst deleted the assertion/count-with-inner-assert-lambda2 branch December 10, 2025 19:15
@claude claude bot mentioned this pull request Dec 10, 2025
1 task
This was referenced Dec 15, 2025
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.

Broken API to chain collection assertions

2 participants