feat: add parsing assertions for various types with format provider support

[thomhurst]

Fixes #2447

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3164 - Parsing Assertions

Todo List:

• Read and analyze all 6 new files
• Check compliance with TUnit coding standards
• Verify dual execution mode considerations
• Review test coverage adequacy
• Check for performance implications
• Evaluate security concerns
• Provide comprehensive feedback

---

✅ Overall Assessment

This is a well-implemented feature that adds string parsing assertions with format provider support. The code follows TUnit's modern C# standards and architectural patterns. However, there are several areas for improvement regarding performance, code duplication, and AOT compatibility.

📋 Detailed Review

Code Quality & Best Practices

✅ Strengths:

• Follows TUnit coding standards (file-scoped namespaces, modern C# patterns, proper naming)
• Good use of [DynamicallyAccessedMembers] for AOT compatibility
• Proper async patterns with ValueTask<T>
• Clean API design with fluent chaining support
• Comprehensive XML documentation

⚠️ Areas for Improvement:

1. Code Duplication - The parsing logic is duplicated across three files:

   • ParseConversionAssertCondition.cs:37-143
   • StringIsNotParsableCondition.cs:32-136
   • StringIsParsableCondition.cs:30-115

   Consider extracting to a shared helper class like TryParseHelper<T>.

2. Inconsistent Return Patterns - In StringIsNotParsableCondition.cs:109, there's a missing ValueTask wrapper:

   return AssertionResult.FailIf(true, $"parsing succeeded with value: {parsedValue}");

   Should be:

   return new ValueTask<AssertionResult>(AssertionResult.FailIf(true, $"parsing succeeded with value: {parsedValue}"));

Performance Considerations

⚠️ Critical Issues:

1. Reflection Overhead - Heavy use of reflection in hot paths without caching:

   • GetMethod() calls on every assertion
   • GetInterfaces().Any() calls
   • Multiple method lookups per parse attempt

   Recommendation: Cache MethodInfo instances using ConcurrentDictionary<Type, ParseMethodCache>.

2. Array Allocations - Multiple parameter arrays created per parse:

   var parameters = new object?[] { value, formatProvider, null }; // ParseConversionAssertCondition.cs:52

   Recommendation: Use object pooling or reusable arrays.

3. Boxing for Value Types - All parsed values get boxed as object? before casting back.

Security Concerns

✅ Good Security Practices:

• Proper null handling throughout
• No unsafe code or unvalidated inputs
• AOT annotations prevent reflection issues

⚠️ Minor Concerns:

• No protection against extremely long strings that could cause DoS
• Consider adding length validation in hot paths

AOT/Trimming Compatibility

✅ Excellent AOT Support:

• Proper use of [UnconditionalSuppressMessage] with clear justifications
• DynamicallyAccessedMembers attributes correctly applied
• No dynamic code generation

Test Coverage

✅ Good Basic Coverage:

• Tests common parsing scenarios (int, double, Guid, DateTime, bool)
• Tests format provider functionality
• Tests chaining with WhenParsedInto

⚠️ Missing Test Cases:

• Edge cases: empty strings, whitespace-only strings
• Error scenarios: null inputs, invalid formats
• Performance edge cases: very long strings
• Culture-specific parsing edge cases
• Types that don't support parsing

Architecture Compliance

✅ Dual Execution Mode Consideration:

• Code works identically in both source-generated and reflection modes
• No mode-specific code paths

✅ Follows TUnit Patterns:

• Consistent with existing assertion API
• Proper use of IValueSource<T> pattern
• Maintains fluent API design

🔧 Recommended Improvements

1. Extract Common Logic:

   internal static class TryParseHelper<T>
   {
       private static readonly ConcurrentDictionary<Type, ParseMethodCache> _methodCache = new();
       // Shared parsing logic here
   }

2. Add Performance Benchmarks (as per CLAUDE.md requirements):

   [Benchmark]
   public void IsParsableInto_Performance() { /* benchmark parsing assertions */ }

3. Expand Test Coverage:

   [Test] public async Task IsParsableInto_EmptyString_Fails() { }
   [Test] public async Task IsParsableInto_UnsupportedType_Fails() { }

4. Fix Return Type Inconsistency in StringIsNotParsableCondition.cs:109

📊 Impact Assessment

• Breaking Changes: None
• Performance Impact: Moderate negative (reflection overhead)
• Memory Impact: Moderate negative (allocations)
• API Surface: Additive only

✅ Approval Recommendation

Approve with minor changes. This feature adds valuable functionality and follows TUnit's architectural patterns well. The identified issues are optimization opportunities rather than blocking problems.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3164 - Parsing Assertions

Todo List:

• Read TUnit development guidelines from CLAUDE.md
• Analyze all changed files for code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage adequacy
• Provide comprehensive feedback

---

✅ Overall Assessment

This is a well-implemented feature that adds string parsing assertions with format provider support. The code follows TUnit's modern C# standards and architectural patterns. However, there are several areas for improvement regarding performance, code duplication, and a bug.

📋 Detailed Review

Code Quality & Best Practices

✅ Strengths:

• Follows TUnit coding standards (file-scoped namespaces, modern C# patterns, proper naming)
• Good use of [DynamicallyAccessedMembers] for AOT compatibility
• Proper async patterns with ValueTask<T>
• Clean API design with fluent chaining support
• Comprehensive XML documentation

⚠️ Areas for Improvement:

1. Code Duplication - The parsing logic is duplicated across three files:

   • ParseConversionAssertCondition.cs:37-143
   • StringIsNotParsableCondition.cs:32-136
   • StringIsParsableCondition.cs:30-115

   Consider extracting to a shared helper class like TryParseHelper<T>.

2. 🐛 Bug: Inconsistent Return Pattern - In StringIsNotParsableCondition.cs:109, there's a missing ValueTask wrapper:

   return AssertionResult.FailIf(true, $"parsing succeeded with value: {parsedValue}");

   Should be:

   return new ValueTask<AssertionResult>(AssertionResult.FailIf(true, $"parsing succeeded with value: {parsedValue}"));

Performance Considerations

⚠️ Critical Issues:

1. Reflection Overhead - Heavy use of reflection in hot paths without caching:

   • GetMethod() calls on every assertion
   • GetInterfaces().Any() calls
   • Multiple method lookups per parse attempt

   Recommendation: Cache MethodInfo instances using ConcurrentDictionary<Type, ParseMethodCache>.

2. Array Allocations - Multiple parameter arrays created per parse:

   var parameters = new object?[] { value, formatProvider, null }; // ParseConversionAssertCondition.cs:52

   Recommendation: Use object pooling or reusable arrays.

3. Boxing for Value Types - All parsed values get boxed as object? before casting back.

Security Concerns

✅ Good Security Practices:

• Proper null handling throughout
• No unsafe code or unvalidated inputs
• AOT annotations prevent reflection issues

⚠️ Minor Concerns:

• No protection against extremely long strings that could cause DoS
• Consider adding length validation in hot paths

AOT/Trimming Compatibility

✅ Excellent AOT Support:

• Proper use of [UnconditionalSuppressMessage] with clear justifications
• DynamicallyAccessedMembers attributes correctly applied
• No dynamic code generation

Test Coverage

✅ Good Basic Coverage:

• Tests common parsing scenarios (int, double, Guid, DateTime, bool)
• Tests format provider functionality
• Tests chaining with WhenParsedInto

⚠️ Missing Test Cases:

• Edge cases: empty strings, whitespace-only strings
• Error scenarios: null inputs, invalid formats
• Performance edge cases: very long strings
• Culture-specific parsing edge cases
• Types that don't support parsing

Architecture Compliance

✅ Dual Execution Mode Consideration:

• Code works identically in both source-generated and reflection modes
• No mode-specific code paths

✅ Follows TUnit Patterns:

• Consistent with existing assertion API
• Proper use of IValueSource<T> pattern
• Maintains fluent API design

🔧 Recommended Improvements

1. Fix the Bug in StringIsNotParsableCondition.cs:109

2. Extract Common Logic:

   internal static class TryParseHelper<T>
   {
       private static readonly ConcurrentDictionary<Type, ParseMethodCache> _methodCache = new();
       // Shared parsing logic here
   }

3. Add Performance Benchmarks (as per CLAUDE.md requirements):

   [Benchmark]
   public void IsParsableInto_Performance() { /* benchmark parsing assertions */ }

4. Expand Test Coverage:

   [Test] public async Task IsParsableInto_EmptyString_Fails() { }
   [Test] public async Task IsParsableInto_UnsupportedType_Fails() { }

📊 Impact Assessment

• Breaking Changes: None
• Performance Impact: Moderate negative (reflection overhead)
• Memory Impact: Moderate negative (allocations)
• API Surface: Additive only

✅ Approval Recommendation

Approve with minor changes. This feature adds valuable functionality and follows TUnit's architectural patterns well. The identified issues are optimization opportunities rather than blocking problems, except for the bug which should be fixed.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3164 - Parsing Assertions

Todo List:

• Read TUnit development guidelines from CLAUDE.md
• Analyze all changed files for code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage adequacy
• Provide comprehensive feedback

---

✅ Overall Assessment

This is a well-implemented feature that adds string parsing assertions with format provider support. The code follows TUnit's modern C# standards and architectural patterns. However, there are several areas for improvement regarding performance, code duplication, and a bug.

📋 Detailed Review

Code Quality & Best Practices

✅ Strengths:

• Follows TUnit coding standards (file-scoped namespaces, modern C# patterns, proper naming)
• Good use of [DynamicallyAccessedMembers] for AOT compatibility
• Proper async patterns with ValueTask<T>
• Clean API design with fluent chaining support
• Comprehensive XML documentation

⚠️ Areas for Improvement:

1. Code Duplication - The parsing logic is duplicated across three files:

   • ParseConversionAssertCondition.cs:37-143
   • StringIsNotParsableCondition.cs:32-136
   • StringIsParsableCondition.cs:30-115

   Consider extracting to a shared helper class like TryParseHelper<T>.

2. 🐛 Bug: Inconsistent Return Pattern - In StringIsNotParsableCondition.cs:109, there's a missing ValueTask wrapper:

   return AssertionResult.FailIf(true, $"parsing succeeded with value: {parsedValue}");

   Should be:

   return new ValueTask<AssertionResult>(AssertionResult.FailIf(true, $"parsing succeeded with value: {parsedValue}"));

Performance Considerations

⚠️ Critical Issues:

1. Reflection Overhead - Heavy use of reflection in hot paths without caching:

   • GetMethod() calls on every assertion
   • GetInterfaces().Any() calls
   • Multiple method lookups per parse attempt

   Recommendation: Cache MethodInfo instances using ConcurrentDictionary<Type, ParseMethodCache>.

2. Array Allocations - Multiple parameter arrays created per parse:

   var parameters = new object?[] { value, formatProvider, null }; // ParseConversionAssertCondition.cs:52

   Recommendation: Use object pooling or reusable arrays.

3. Boxing for Value Types - All parsed values get boxed as object? before casting back.

Security Concerns

✅ Good Security Practices:

• Proper null handling throughout
• No unsafe code or unvalidated inputs
• AOT annotations prevent reflection issues

⚠️ Minor Concerns:

• No protection against extremely long strings that could cause DoS
• Consider adding length validation in hot paths

AOT/Trimming Compatibility

✅ Excellent AOT Support:

• Proper use of [UnconditionalSuppressMessage] with clear justifications
• DynamicallyAccessedMembers attributes correctly applied
• No dynamic code generation

Test Coverage

✅ Good Basic Coverage:

• Tests common parsing scenarios (int, double, Guid, DateTime, bool)
• Tests format provider functionality
• Tests chaining with WhenParsedInto

⚠️ Missing Test Cases:

• Edge cases: empty strings, whitespace-only strings
• Error scenarios: null inputs, invalid formats
• Performance edge cases: very long strings
• Culture-specific parsing edge cases
• Types that don't support parsing

Architecture Compliance

✅ Dual Execution Mode Consideration:

• Code works identically in both source-generated and reflection modes
• No mode-specific code paths

✅ Follows TUnit Patterns:

• Consistent with existing assertion API
• Proper use of IValueSource<T> pattern
• Maintains fluent API design

🔧 Recommended Improvements

1. Fix the Bug in StringIsNotParsableCondition.cs:109

2. Extract Common Logic:

   internal static class TryParseHelper<T>
   {
       private static readonly ConcurrentDictionary<Type, ParseMethodCache> _methodCache = new();
       // Shared parsing logic here
   }

3. Add Performance Benchmarks (as per CLAUDE.md requirements):

   [Benchmark]
   public void IsParsableInto_Performance() { /* benchmark parsing assertions */ }

4. Expand Test Coverage:

   [Test] public async Task IsParsableInto_EmptyString_Fails() { }
   [Test] public async Task IsParsableInto_UnsupportedType_Fails() { }

📊 Impact Assessment

• Breaking Changes: None
• Performance Impact: Moderate negative (reflection overhead)
• Memory Impact: Moderate negative (allocations)
• API Surface: Additive only

✅ Approval Recommendation

Approve with minor changes. This feature adds valuable functionality and follows TUnit's architectural patterns well. The identified issues are optimization opportunities rather than blocking problems, except for the bug which should be fixed.

---