fix: add decimal equality tests and enhance argument attribute handling

[thomhurst]

Fixes #2764

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Decimal Equality Tests and Argument Attribute Handling

I've completed my review of this pull request. This appears to be a well-thought-out fix for decimal precision handling in TUnit's source generator.

Todo List:

• Read repository guidelines and context
• Analyze TestMetadataGenerator.cs changes
• Analyze DecimalArgumentTests.cs changes
• Review against TUnit development guidelines
• Check for potential bugs and issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

---

Review Findings

✅ Strengths

1. Preserves Decimal Precision: The implementation correctly preserves decimal literal precision by using source syntax rather than TypedConstant values, which is crucial for decimal accuracy.

2. Robust Fallback Strategy: Good defensive programming with try-catch fallback to default AttributeWriter behavior when parameter type matching fails (TestMetadataGenerator.cs:671-675).

3. Comprehensive Test Coverage: Excellent test coverage including edge cases:

   • High precision decimals (e.g., 2_000.00000000000000001)
   • Numeric separators (123_999)
   • String-based decimal inputs for edge values
   • Multiple parameter scenarios
   • Min/max decimal values

4. Follows TUnit Guidelines: Uses modern C# syntax, proper error handling, and follows the repository's coding standards.

⚠️ Potential Issues

1. Source-Generator Only Implementation: This appears to only fix the source-generated mode. According to TUnit's MANDATORY RULE Repeat attributes  #1, ALL changes must work identically in both source-generated AND reflection modes. The reflection path in TUnit.Engine may still have the decimal precision issue.

2. Performance Consideration: The new logic adds semantic model access and syntax tree traversal (TestMetadataGenerator.cs:722-725). While likely minimal impact, this is in the source generator hot path.

3. Type Safety: The decimal suffix logic (TestMetadataGenerator.cs:743-747) uses string manipulation on expression syntax. Consider validating that the expression is actually a numeric literal before applying the 'm' suffix.

🔧 Recommendations

1. Critical: Verify that the reflection mode (TUnit.Engine) handles decimal precision correctly, or implement the same fix there to comply with dual-mode requirements.

2. Testing: Consider adding tests that verify identical behavior between source-generated and reflection execution modes.

3. Snapshot Tests: Since this modifies source generator behavior, ensure snapshot tests are run and updated per the CLAUDE.md guidelines:

   dotnet test TUnit.Core.SourceGenerator.Tests
   # If snapshots differ, accept them after verification

4. Performance: Consider caching the semantic model if this method is called frequently to minimize performance impact.

🔒 Security & Safety

• No security concerns identified
• Code is defensive with proper null checks and exception handling
• String manipulation appears safe (no injection vectors)

🎯 Test Coverage Assessment

Excellent test coverage including:

• ✅ Various decimal precisions and formats
• ✅ Edge cases (min/max values)
• ✅ Multiple parameter scenarios
• ✅ String-to-decimal conversions
• ❓ Missing: Tests verifying identical behavior between source-gen and reflection modes

Overall Assessment

This is a solid implementation that addresses decimal precision issues in the source generator. The main concern is ensuring dual-mode compatibility as required by TUnit's architecture. The test coverage is thorough and the code quality is high.

Recommendation: ✅ Approve after confirming reflection mode compatibility

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Decimal Equality Tests and Argument Attribute Handling

I've completed my review of this pull request that fixes issue #2764. Here's my comprehensive analysis:

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage
• Provide comprehensive feedback

---

Code Quality & Best Practices ✅

Strengths:

• Modern C# syntax: Proper use of collection expressions, var declarations, and method formatting
• Error handling: Graceful fallback in GenerateArgumentsAttributeWithParameterTypes with try-catch for robustness
• Code organization: New functionality is well-encapsulated in a separate method
• Documentation: Clear inline comments explaining the decimal precision preservation logic
• Naming conventions: Follows TUnit's established patterns

Areas for improvement:

• The catch block in TestMetadataGenerator.cs:671-675 swallows all exceptions. Consider logging or being more specific about expected exception types.

Potential Bugs & Issues ⚠️

Critical TUnit Requirement - Dual Mode Implementation:

• ❌ This implementation only addresses the source generator mode. According to CLAUDE.md, ALL changes must work identically in both source-generated AND reflection modes.
• The reflection mode execution path in TUnit.Engine does not appear to have corresponding changes for decimal precision handling.
• This violates TUnit's mandatory rule Repeat attributes  #1: "Dual-mode implementation required"

Other Issues:

• String manipulation for decimal suffix removal (TrimEnd('d', 'D', 'f', 'F', 'm', 'M')) could be fragile with complex expressions
• No validation that the source text represents a valid decimal literal before adding 'm' suffix

Performance Considerations ✅

Good performance practices:

• Syntax tree access is appropriately gated behind null checks
• Fallback to efficient TypedConstant-based formatting when syntax isn't available
• No obvious performance regressions in hot paths

Security Concerns ✅

No security issues identified:

• Code generation is properly scoped to decimal literals
• Uses semantic model for safe expression rewriting
• No injection vulnerabilities in the string manipulation

Test Coverage ✅

Excellent test coverage:

• Comprehensive decimal test cases covering edge cases:
  • High precision decimals (2_000.00000000000000001)
  • String-based decimal arguments
  • Min/max decimal values
  • Multiple decimal parameters
  • Mixed argument types
• Tests validate both value equality and precision preservation
• Good use of CultureInfo.InvariantCulture for consistent formatting

Technical Implementation Review

Source Generator Enhancement (TestMetadataGenerator.cs):

• ✅ Proper integration with existing attribute handling pipeline
• ✅ Intelligent syntax-based vs TypedConstant-based approach
• ✅ Leverages existing TypedConstantFormatter infrastructure
• ✅ Preserves source text for decimal literals to maintain precision

Test Implementation (DecimalArgumentTests.cs):

• ✅ Comprehensive coverage of decimal argument scenarios
• ✅ Tests both basic and edge cases
• ✅ Validates precision preservation with .ToString() assertions

Recommendations

High Priority:

1. Implement reflection mode support - Add corresponding decimal handling in TUnit.Engine to ensure dual-mode compatibility
2. Run snapshot tests - Since this modifies source generator code, run dotnet test TUnit.Core.SourceGenerator.Tests and accept snapshots if needed

Medium Priority:
3. Improve error handling - Consider more specific exception handling or logging in the fallback catch block
4. Add input validation - Validate decimal literal format before adding suffix

Low Priority:
5. Consider edge cases - Test with scientific notation, negative decimals, and other edge formats

Overall Assessment

This is a solid implementation that addresses decimal precision issues in argument attributes. The test coverage is excellent and the technical approach is sound. However, the critical missing piece is reflection mode support, which is mandatory for TUnit. The PR should not be merged until dual-mode implementation is complete.

Recommendation: Request changes - Implement reflection mode support before merging.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Decimal Equality Tests and Argument Attribute Handling

I've completed my comprehensive review of this pull request that fixes issue #2764. Here's my detailed analysis:

Review Checklist:

• Read repository guidelines and understand TUnit requirements
• Analyze TestMetadataGenerator.cs changes
• Analyze DecimalArgumentTests.cs changes
• Evaluate code quality and best practices
• Check for potential bugs and issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage
• Check compliance with TUnit's dual-mode requirements
• Provide comprehensive feedback

---

Code Quality & Best Practices ✅

Strengths:

• Modern C# syntax: Proper use of collection expressions, var declarations, and clean method formatting
• Error handling: Graceful fallback mechanism with try-catch in GenerateArgumentsAttributeWithParameterTypes (lines 667-676)
• Code organization: New functionality is well-encapsulated in a dedicated method
• Documentation: Clear inline comments explaining decimal precision preservation logic
• Naming conventions: Follows TUnit's established patterns consistently

Areas for improvement:

• The catch block (lines 671-675) swallows all exceptions without logging. Consider being more specific about expected exception types or adding telemetry.

Critical Issue: Dual-Mode Requirement Violation ❌

MAJOR CONCERN: This implementation violates TUnit's MANDATORY RULE #1 from CLAUDE.md:

"Dual-mode implementation required: ALL changes must work identically in both source-generated and reflection modes"

Analysis:

• ✅ Source-generated mode: Enhanced with decimal precision preservation in TestMetadataGenerator.cs
• ❌ Reflection mode: No corresponding changes found in TUnit.Engine
• The ArgumentsAttribute.cs (lines 6-18) shows the reflection mode uses the raw Values array from constructor arguments
• When decimal values are passed to the constructor, they lose precision through the TypedConstant conversion that this PR specifically aims to fix

Potential Bugs & Issues ⚠️

Technical Issues:

1. String manipulation fragility: Line 755 uses TrimEnd('d', 'D', 'f', 'F', 'm', 'M') which could be unsafe with complex expressions
2. Missing validation: No verification that the source text represents a valid decimal literal before adding 'm' suffix
3. Inconsistent behavior: Tests like Equality3 and Equality4 (lines 107-127) explicitly skip reflection mode, indicating known precision loss

Logic concerns:

• The semantic model access (lines 735-736) adds overhead in the source generator hot path
• Fall-through logic may mask genuine syntax processing errors

Performance Considerations ✅

Assessment:

• Syntax tree access is appropriately gated behind null checks
• Fallback to efficient TypedConstant-based formatting when syntax isn't available
• Semantic model usage is minimal and only when needed
• No obvious performance regressions in hot paths

Security Concerns ✅

Assessment:

• No security issues identified
• Code generation is properly scoped to decimal literals
• Uses semantic model for safe expression rewriting
• String manipulation appears safe with no injection vectors

Test Coverage ✅

Excellent test coverage includes:

• Comprehensive decimal cases: High precision decimals (2_000.00000000000000001), numeric separators (123_999)
• Edge cases: Min/max decimal values via string conversion
• Multiple scenarios: Multiple decimal parameters, mixed argument types
• String-based inputs: Edge values that can't be represented as literals
• Precision validation: Uses CultureInfo.InvariantCulture for consistent .ToString() assertions

Test quality highlights:

• Lines 21-26 specifically validate precision preservation
• Lines 109-127 acknowledge reflection mode limitations with conditional skips
• Good use of SourceRegistrar.IsEnabled to handle mode differences

Technical Implementation Analysis

Source Generator Enhancement (TestMetadataGenerator.cs):

• ✅ Intelligent approach: Syntax-based precision preservation for decimals, falls back to TypedConstant for other types
• ✅ Integration: Properly integrated with existing AttributeWriter infrastructure
• ✅ Robustness: Handles missing syntax gracefully with comprehensive fallback logic
• ✅ Type-aware: Uses target parameter types to apply decimal-specific handling

Test Implementation (DecimalArgumentTests.cs):

• ✅ Comprehensive: Covers basic and edge cases thoroughly
• ✅ Realistic: Tests actual usage scenarios developers would encounter
• ✅ Mode-aware: Acknowledges and handles dual-mode differences appropriately

Recommendations

High Priority (Blocking):

1. 🚨 Implement reflection mode support - This is MANDATORY per TUnit guidelines. The reflection path in TUnit.Engine must preserve decimal precision identically to the source generator
2. Run snapshot tests - Execute dotnet test TUnit.Core.SourceGenerator.Tests and accept snapshots per CLAUDE.md requirements

Medium Priority:
3. Improve error handling - Make exception handling more specific or add telemetry to the catch block
4. Add input validation - Validate decimal literal format before adding 'm' suffix
5. Add dual-mode tests - Include tests that verify identical behavior between source-gen and reflection execution

Low Priority:
6. Consider edge cases - Test scientific notation, negative decimals, and cultural formatting scenarios

Overall Assessment

This is a technically sound implementation that effectively addresses decimal precision issues in the source generator. The code quality is high, test coverage is excellent, and the approach is well-architected.

However, the critical missing piece is reflection mode support, which violates TUnit's core architectural requirement. The PR demonstrates clear understanding of the problem and provides a robust solution for one execution mode, but fails to address the dual-mode mandate.

Recommendation: 🔴 Request changes - Implement reflection mode support before merging to ensure compliance with TUnit's mandatory dual-mode requirement.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Decimal Equality Tests and Argument Attribute Handling

I've completed my comprehensive review of this pull request that fixes issue #2764. Here's my detailed analysis:

Review Checklist:

• Read repository guidelines and understand TUnit requirements
• Analyze TestMetadataGenerator.cs changes
• Analyze DecimalArgumentTests.cs changes
• Evaluate code quality and best practices
• Check for potential bugs and issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage
• Check compliance with TUnit's dual-mode requirements
• Provide comprehensive feedback

---

Code Quality & Best Practices ✅

Strengths:

• Modern C# syntax: Proper use of collection expressions, var declarations, and clean method formatting
• Error handling: Graceful fallback mechanism with try-catch in GenerateArgumentsAttributeWithParameterTypes (lines 667-676)
• Code organization: New functionality is well-encapsulated in a dedicated method
• Documentation: Clear inline comments explaining decimal precision preservation logic
• Naming conventions: Follows TUnit's established patterns consistently

Areas for improvement:

• The catch block (lines 671-675) swallows all exceptions without logging. Consider being more specific about expected exception types or adding telemetry.

Critical Issue: Dual-Mode Requirement Violation ❌

MAJOR CONCERN: This implementation violates TUnit's MANDATORY RULE #1 from CLAUDE.md:

"Dual-mode implementation required: ALL changes must work identically in both source-generated and reflection modes"

Analysis:

• ✅ Source-generated mode: Enhanced with decimal precision preservation in TestMetadataGenerator.cs
• ❌ Reflection mode: No corresponding changes found in TUnit.Engine
• The ArgumentsAttribute.cs (lines 6-18) shows the reflection mode uses the raw Values array from constructor arguments
• When decimal values are passed to the constructor, they lose precision through the TypedConstant conversion that this PR specifically aims to fix

Potential Bugs & Issues ⚠️

Technical Issues:

1. String manipulation fragility: Line 755 uses TrimEnd('d', 'D', 'f', 'F', 'm', 'M') which could be unsafe with complex expressions
2. Missing validation: No verification that the source text represents a valid decimal literal before adding 'm' suffix
3. Inconsistent behavior: Tests like Equality3 and Equality4 (lines 107-127) explicitly skip reflection mode, indicating known precision loss

Logic concerns:

• The semantic model access (lines 735-736) adds overhead in the source generator hot path
• Fall-through logic may mask genuine syntax processing errors

Performance Considerations ✅

Assessment:

• Syntax tree access is appropriately gated behind null checks
• Fallback to efficient TypedConstant-based formatting when syntax isn't available
• Semantic model usage is minimal and only when needed
• No obvious performance regressions in hot paths

Security Concerns ✅

Assessment:

• No security issues identified
• Code generation is properly scoped to decimal literals
• Uses semantic model for safe expression rewriting
• String manipulation appears safe with no injection vectors

Test Coverage ✅

Excellent test coverage includes:

• Comprehensive decimal cases: High precision decimals (2_000.00000000000000001), numeric separators (123_999)
• Edge cases: Min/max decimal values via string conversion
• Multiple scenarios: Multiple decimal parameters, mixed argument types
• String-based inputs: Edge values that can't be represented as literals
• Precision validation: Uses CultureInfo.InvariantCulture for consistent .ToString() assertions

Test quality highlights:

• Lines 21-26 specifically validate precision preservation
• Lines 109-127 acknowledge reflection mode limitations with conditional skips
• Good use of SourceRegistrar.IsEnabled to handle mode differences

Technical Implementation Analysis

Source Generator Enhancement (TestMetadataGenerator.cs):

• ✅ Intelligent approach: Syntax-based precision preservation for decimals, falls back to TypedConstant for other types
• ✅ Integration: Properly integrated with existing AttributeWriter infrastructure
• ✅ Robustness: Handles missing syntax gracefully with comprehensive fallback logic
• ✅ Type-aware: Uses target parameter types to apply decimal-specific handling

Test Implementation (DecimalArgumentTests.cs):

• ✅ Comprehensive: Covers basic and edge cases thoroughly
• ✅ Realistic: Tests actual usage scenarios developers would encounter
• ✅ Mode-aware: Acknowledges and handles dual-mode differences appropriately

Recommendations

High Priority (Blocking):

1. 🚨 Implement reflection mode support - This is MANDATORY per TUnit guidelines. The reflection path in TUnit.Engine must preserve decimal precision identically to the source generator
2. Run snapshot tests - Execute dotnet test TUnit.Core.SourceGenerator.Tests and accept snapshots per CLAUDE.md requirements

Medium Priority:
3. Improve error handling - Make exception handling more specific or add telemetry to the catch block
4. Add input validation - Validate decimal literal format before adding 'm' suffix
5. Add dual-mode tests - Include tests that verify identical behavior between source-gen and reflection execution

Low Priority:
6. Consider edge cases - Test scientific notation, negative decimals, and cultural formatting scenarios

Overall Assessment

This is a technically sound implementation that effectively addresses decimal precision issues in the source generator. The code quality is high, test coverage is excellent, and the approach is well-architected.

However, the critical missing piece is reflection mode support, which violates TUnit's core architectural requirement. The PR demonstrates clear understanding of the problem and provides a robust solution for one execution mode, but fails to address the dual-mode mandate.

Recommendation: 🔴 Request changes - Implement reflection mode support before merging to ensure compliance with TUnit's mandatory dual-mode requirement.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Decimal Equality Tests and Argument Attribute Handling

I've completed my comprehensive review of this pull request that fixes issue #2764. Here's my detailed analysis:

Review Checklist:

• Read repository guidelines and understand TUnit requirements
• Analyze TestMetadataGenerator.cs changes
• Analyze DecimalArgumentTests.cs changes
• Evaluate code quality and best practices
• Check for potential bugs and issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage
• Check compliance with TUnit's dual-mode requirements
• Provide comprehensive feedback

---

Code Quality & Best Practices ✅

Strengths:

• Modern C# syntax: Proper use of collection expressions, var declarations, and clean method formatting
• Error handling: Graceful fallback mechanism with try-catch in GenerateArgumentsAttributeWithParameterTypes (lines 667-676)
• Code organization: New functionality is well-encapsulated in a dedicated method
• Documentation: Clear inline comments explaining decimal precision preservation logic
• Naming conventions: Follows TUnit's established patterns consistently

Areas for improvement:

• The catch block (lines 671-675) swallows all exceptions without logging. Consider being more specific about expected exception types or adding telemetry.

Critical Issue: Dual-Mode Requirement Violation ❌

MAJOR CONCERN: This implementation violates TUnit's MANDATORY RULE #1 from CLAUDE.md:

"Dual-mode implementation required: ALL changes must work identically in both source-generated and reflection modes"

Analysis:

• ✅ Source-generated mode: Enhanced with decimal precision preservation in TestMetadataGenerator.cs
• ❌ Reflection mode: No corresponding changes found in TUnit.Engine
• The ArgumentsAttribute.cs (lines 6-18) shows the reflection mode uses the raw Values array from constructor arguments
• When decimal values are passed to the constructor, they lose precision through the TypedConstant conversion that this PR specifically aims to fix

Potential Bugs & Issues ⚠️

Technical Issues:

1. String manipulation fragility: Line 755 uses TrimEnd('d', 'D', 'f', 'F', 'm', 'M') which could be unsafe with complex expressions
2. Missing validation: No verification that the source text represents a valid decimal literal before adding 'm' suffix
3. Inconsistent behavior: Tests like Equality3 and Equality4 (lines 107-127) explicitly skip reflection mode, indicating known precision loss

Logic concerns:

• The semantic model access (lines 735-736) adds overhead in the source generator hot path
• Fall-through logic may mask genuine syntax processing errors

Performance Considerations ✅

Assessment:

• Syntax tree access is appropriately gated behind null checks
• Fallback to efficient TypedConstant-based formatting when syntax isn't available
• Semantic model usage is minimal and only when needed
• No obvious performance regressions in hot paths

Security Concerns ✅

Assessment:

• No security issues identified
• Code generation is properly scoped to decimal literals
• Uses semantic model for safe expression rewriting
• String manipulation appears safe with no injection vectors

Test Coverage ✅

Excellent test coverage includes:

• Comprehensive decimal cases: High precision decimals (2_000.00000000000000001), numeric separators (123_999)
• Edge cases: Min/max decimal values via string conversion
• Multiple scenarios: Multiple decimal parameters, mixed argument types
• String-based inputs: Edge values that can't be represented as literals
• Precision validation: Uses CultureInfo.InvariantCulture for consistent .ToString() assertions

Test quality highlights:

• Lines 21-26 specifically validate precision preservation
• Lines 109-127 acknowledge reflection mode limitations with conditional skips
• Good use of SourceRegistrar.IsEnabled to handle mode differences

Technical Implementation Analysis

Source Generator Enhancement (TestMetadataGenerator.cs):

• ✅ Intelligent approach: Syntax-based precision preservation for decimals, falls back to TypedConstant for other types
• ✅ Integration: Properly integrated with existing AttributeWriter infrastructure
• ✅ Robustness: Handles missing syntax gracefully with comprehensive fallback logic
• ✅ Type-aware: Uses target parameter types to apply decimal-specific handling

Test Implementation (DecimalArgumentTests.cs):

• ✅ Comprehensive: Covers basic and edge cases thoroughly
• ✅ Realistic: Tests actual usage scenarios developers would encounter
• ✅ Mode-aware: Acknowledges and handles dual-mode differences appropriately

Recommendations

High Priority (Blocking):

1. 🚨 Implement reflection mode support - This is MANDATORY per TUnit guidelines. The reflection path in TUnit.Engine must preserve decimal precision identically to the source generator
2. Run snapshot tests - Execute dotnet test TUnit.Core.SourceGenerator.Tests and accept snapshots per CLAUDE.md requirements

Medium Priority:
3. Improve error handling - Make exception handling more specific or add telemetry to the catch block
4. Add input validation - Validate decimal literal format before adding 'm' suffix
5. Add dual-mode tests - Include tests that verify identical behavior between source-gen and reflection execution

Low Priority:
6. Consider edge cases - Test scientific notation, negative decimals, and cultural formatting scenarios

Overall Assessment

This is a technically sound implementation that effectively addresses decimal precision issues in the source generator. The code quality is high, test coverage is excellent, and the approach is well-architected.

However, the critical missing piece is reflection mode support, which violates TUnit's core architectural requirement. The PR demonstrates clear understanding of the problem and provides a robust solution for one execution mode, but fails to address the dual-mode mandate.

Recommendation: 🔴 Request changes - Implement reflection mode support before merging to ensure compliance with TUnit's mandatory dual-mode requirement.

---