feat: add Count method with inner assertion support for collections

[thomhurst]

No description provided.

[claude]

Pull Request Review: Count with Inner Assertion Support

Thank you for this feature addition! This is a nice enhancement to the assertion API that provides better expressiveness.

Positives

1. Good API Design: The Count(item => item.IsGreaterThan(2)) pattern is more intuitive and expressive
2. Comprehensive Test Coverage: 9 well-structured tests covering various scenarios
3. Proper Snapshot Updates: All public API snapshots correctly updated across all target frameworks
4. Type Safety: Using IAssertionSource maintains type safety throughout the assertion chain
5. OverloadResolutionPriority: Correctly used to prefer the assertion-based overload

Issues and Concerns

1. CRITICAL: Catch All Exceptions (CollectionCountValueAssertion.cs:83-91)

The bare catch block is an anti-pattern explicitly mentioned in CLAUDE.md. Catching all exceptions without type specification silently swallows critical exceptions like OutOfMemoryException, StackOverflowException, and ThreadAbortException.

Recommendation: Replace with specific exception type that assertions throw when they fail.

2. Performance: Allocations in Hot Path (CollectionCountValueAssertion.cs:76-100)

The foreach loop creates allocations on every iteration:

• String interpolation allocates a new string for each item
• ValueAssertion constructor creates a new StringBuilder internally
• New ValueAssertion and Assertion objects per item

Impact: For 10,000 items, this creates 30,000+ allocations.

3. Documentation: Missing Performance Notes

The XML documentation should mention O(n) complexity and suggest using the simple predicate overload for better performance.

4. Testing: Missing Failure Case Tests

Consider adding tests for assertion failure scenarios, complex nested assertions, and null item handling.

Action Items

Must Fix Before Merge:

1. Replace bare catch with specific exception type

Should Fix:
2. Optimize allocations in the loop
3. Add performance notes to XML documentation

Nice to Have:
4. Add failure scenario tests

Summary

Solid feature addition with good test coverage. Main concerns are the catch-all exception handler (critical fix needed) and performance implications in hot paths.

Overall Assessment: Needs Changes (primarily exception handling fix)

[claude]

Code Review: Add Count method with inner assertion support

Summary

This PR adds a powerful new API for counting collection items using the full assertion builder instead of simple predicates. The implementation is well-designed and follows TUnit's patterns, but there are several important concerns to address.

✅ Strengths

1. Excellent API Design: The new Count(item => item.IsGreaterThan(10)) syntax is intuitive and leverages existing assertion infrastructure beautifully.

2. Good Use of Overload Resolution Priority: Using [OverloadResolutionPriority(1)] on the new method ensures it's preferred over the predicate version, enabling a smooth migration path.

3. Comprehensive Test Coverage: The test suite covers all major scenarios including edge cases (empty collections, none match, all match).

4. Proper Deprecation: Marking the old predicate-based method as [Obsolete] with a clear migration message is the right approach.

5. Documentation Updated: docs/docs/assertions/collections.md properly reflects the new API.

🚨 Critical Issues

1. Performance - Exception-Based Control Flow (CRITICAL)

Location: CollectionCountValueAssertion.cs:83-91

The implementation uses exceptions for control flow, which violates TUnit's performance-first principle:

try
{
    await resultingAssertion.AssertAsync();
    count++;
}
catch
{
    // Item did not satisfy the assertion, don't count it
}

Problem: Exceptions are extremely expensive in .NET (~1000x slower than normal control flow). For large collections, this could cause significant performance degradation.

Impact: Users counting items in collections with thousands of elements will see severe slowdowns, especially when many items don't match.

Suggested Fix: The assertion framework should expose a TryAssertAsync() method that returns AssertionResult instead of throwing. If that doesn't exist, consider:

// Option 1: Check if assertion passes without executing
var result = await resultingAssertion.GetResultAsync(); // Returns AssertionResult
if (result.Passed)
{
    count++;
}

// Option 2: If no alternative exists, document the performance implications
// and consider adding a fast-path for common assertions

2. Overly Broad Exception Catching

Location: CollectionCountValueAssertion.cs:88

catch
{
    // Item did not satisfy the assertion, don't count it
}

Problem: Catching all exceptions swallows important errors:

• OutOfMemoryException
• StackOverflowException
• Bugs in assertion code
• Cancellation exceptions

Fix: At minimum, catch only AssertionException or whatever specific exception type the assertion framework throws:

catch (AssertionException)
{
    // Item did not satisfy the assertion, don't count it
}

⚠️ Important Issues

3. Missing Cancellation Token Support

The CreateIntContext method is async but doesn't accept a CancellationToken. According to CLAUDE.md Rule 5, async methods should accept cancellation tokens.

Impact: Long-running counts cannot be cancelled, potentially blocking shutdown or timeouts.

4. Potential N+1 Assertion Context Creation

Location: CollectionCountValueAssertion.cs:78

var itemAssertion = new ValueAssertion<TItem>(item, $"item[{index}]");

Creating a new ValueAssertion for each item involves:

• String allocation for "item[{index}]"
• StringBuilder allocation in ValueAssertion constructor
• AssertionContext allocation

Impact: For large collections, this could cause significant GC pressure.

Consideration: Since these assertions are temporary and only used to check pass/fail, consider using an object pool or a lightweight assertion path if available.

💡 Minor Suggestions

5. Test Naming Convention

Test names like Count_WithInnerAssertion_IsGreaterThan are clear, but could follow a more BDD-style pattern:

public async Task Count_WithInnerAssertion_CountsOnlyMatchingItems()
public async Task Count_WithInnerAssertion_EmptyCollection_ReturnsZero()

6. Missing Test Cases

Consider adding tests for:

• Complex nested assertions (e.g., Count(item => item.IsGreaterThan(10).And.IsLessThan(20)))
• Collections with null items (if supported)
• Performance test comparing to old predicate method
• Verify that inner assertion failures don't pollute outer assertion context

7. Snapshot Updates Look Correct

The public API snapshot files have been properly updated with the new method signatures and the obsolete attribute on the old method. ✓

8. Documentation Enhancement

The docs update is good, but consider adding a performance note:

> **Note**: The inner assertion approach provides a more expressive API but may be slower for large collections compared to simple predicates due to assertion framework overhead.

🎯 Recommendations

Must Fix Before Merge:

1. ✅ Fix exception-based control flow - This is a performance regression waiting to happen
2. ✅ Narrow exception catching - Don't swallow critical exceptions

Should Fix:

3. Add cancellation token support
4. Add performance test comparing old vs new approach

Nice to Have:

5. Add missing test cases
6. Consider object pooling for assertion contexts

📊 Code Quality Score

Category	Rating	Notes
API Design	⭐⭐⭐⭐⭐	Excellent, intuitive API
Test Coverage	⭐⭐⭐⭐	Comprehensive edge cases
Performance	⭐⭐	Exception-based control flow is a concern
Code Style	⭐⭐⭐⭐⭐	Follows TUnit conventions
Documentation	⭐⭐⭐⭐	Well documented

Conclusion

This is a valuable feature with great API design. The main blocker is the exception-based control flow which could cause performance issues in production. Please investigate if the assertion framework provides a non-throwing API for checking assertion results. If not, document the performance implications clearly.

Status: ⚠️ Requires changes before merge due to performance concerns

Great work overall! The feature itself is excellent—just needs some implementation refinements.

[Copilot]

Pull request overview

This PR adds a new overload for the Count() method on collections that accepts an assertion builder function instead of a simple predicate. This enables more expressive filtering using the full TUnit assertion API (e.g., Count(item => item.IsGreaterThan(10))) rather than just predicates.

Key Changes

• New CollectionCountWithAssertionValueAssertion<TCollection, TItem> class that evaluates assertions on collection items
• New Count() overload in CollectionAssertionBase marked with OverloadResolutionPriority(1) to disambiguate from the predicate overload
• Comprehensive test coverage for various scenarios including empty collections, no matches, all matches, and different data types

Reviewed changes

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

Show a summary per file

File	Description
TUnit.PublicAPI/Tests.Assertions_Library_Has_No_API_Changes.Net4_7.verified.txt	Updated API snapshot to include new Count overload and CollectionCountWithAssertionValueAssertion class
TUnit.PublicAPI/Tests.Assertions_Library_Has_No_API_Changes.DotNet9_0.verified.txt	Updated API snapshot with OverloadResolutionPriority attribute annotation for .NET 9.0
TUnit.PublicAPI/Tests.Assertions_Library_Has_No_API_Changes.DotNet8_0.verified.txt	Updated API snapshot for .NET 8.0 compatibility
TUnit.PublicAPI/Tests.Assertions_Library_Has_No_API_Changes.DotNet10_0.verified.txt	Updated API snapshot with OverloadResolutionPriority attribute for .NET 10.0
TUnit.Assertions/Sources/CollectionAssertionBase.cs	Added new Count overload accepting assertion builder with appropriate documentation and overload resolution priority
TUnit.Assertions/Conditions/CollectionCountValueAssertion.cs	Implemented CollectionCountWithAssertionValueAssertion class that counts items satisfying inner assertions
TUnit.Assertions.Tests/CollectionAssertionTests.cs	Added comprehensive tests covering various scenarios for the new Count overload

[Copilot]

String interpolation in a loop creates a new string for each iteration. Since this expression is only used for potential error messages in the inner assertion and not for the count logic itself, consider if this allocation is necessary in a hot path.

If the expression isn't critical for this use case, you might pass null or a constant string to avoid repeated allocations. Alternatively, use a cached format if the expression is needed.

Suggested change

	var itemAssertion = new ValueAssertion<TItem>(item, $"item[{index}]");
	var itemAssertion = new ValueAssertion<TItem>(item, null);

[Copilot]

Using exceptions for control flow is a performance anti-pattern. When items don't satisfy the assertion, an AssertionException is thrown and caught for each non-matching item. Exception creation and stack trace generation is expensive.

Consider checking if Assertion<TItem> provides a non-throwing method to evaluate the assertion result, similar to patterns used elsewhere in the codebase with CheckAsync returning AssertionResult. If not available, document this performance characteristic or consider an alternative approach that doesn't rely on exception handling for expected control flow.

Suggested change

	try
	{
	await resultingAssertion.AssertAsync();
	count++;
	}
	catch
	{
	// Item did not satisfy the assertion, don't count it
	}
	var result = await resultingAssertion.CheckAsync();
	if (result.IsPassed)
	{
	count++;
	}
	// If not passed, don't count it

[Copilot]

Bare catch blocks without exception type specification should be avoided as they catch all exceptions, including system-critical ones like OutOfMemoryException, StackOverflowException, and ThreadAbortException. This can hide unexpected bugs and make debugging difficult.

Change to catch (AssertionException) to specifically catch only assertion failures, which is the expected exception type from AssertAsync(). Other exception types (e.g., NullReferenceException, InvalidOperationException) likely indicate bugs that should not be silently ignored.