fix: update invocation logic for dynamic test expressions

[thomhurst]

Fixes #3173

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR: fix: update invocation logic for dynamic test expressions

Todo List:

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

---

Summary

This PR fixes a critical bug in how dynamic test expressions are invoked in TUnit's test discovery process. The issue was that both the AOT and reflection collectors were incorrectly compiling and directly invoking lambda expressions instead of extracting the method information and using the actual test arguments.

Analysis

Bug Understanding

The problem was in how DynamicTest<T> expressions with TestMethodArguments were being executed. The framework was:

1. Compiling the lambda expression directly
2. Invoking it with just the test instance
3. Ignoring the actual TestMethodArguments provided by the user

This meant that dynamic tests using DynamicTestHelper.Argument<T>() placeholders couldn't receive their actual argument values.

Changes Made

Identical fixes in both execution paths (✅ follows dual-mode requirement):

• TUnit.Engine/Building/Collectors/AotTestDataCollector.cs:197-250
• TUnit.Engine/Discovery/ReflectionTestDataCollector.cs:2041-2094

Both now:

1. Extract MethodInfo from the lambda expression body
2. Use methodInfo.Invoke(testInstance, args) with the actual test arguments
3. Handle both direct MethodCallExpression and UnaryExpression wrapping

Code Quality Assessment

✅ Excellent

• Dual-mode implementation: Both AOT and reflection paths updated identically
• Error handling: Comprehensive null checks and descriptive error messages
• Code consistency: Same logic pattern applied in both collectors
• Modern C# syntax: Uses pattern matching and null-conditional operators appropriately

✅ Test Coverage

The new test file TUnit.TestProject/Bugs/3173/DynamicTestArgumentsTests.cs provides excellent coverage:

• Tests string, int, and multiple argument scenarios
• Uses DynamicTestHelper.Argument<T>() pattern correctly
• Validates actual vs. expected values
• Marked with [EngineTest(ExpectedResult.Pass)] for integration testing

Technical Review

✅ Performance Considerations

• Improved performance: Eliminates unnecessary expression compilation
• Direct method invocation: More efficient than compiled expression delegation
• Reduced allocations: No delegate creation overhead

✅ Security Concerns

• Maintained safety: Same reflection-based invocation as existing code
• Proper error propagation: Uses TargetInvocationException handling
• No new attack vectors: Changes are internal to existing execution paths

✅ Best Practices

• Exception handling: Proper unwrapping of TargetInvocationException
• Null safety: Comprehensive null checks throughout
• AOT compatibility: Maintains existing AOT attribution patterns
• Code comments: Clear documentation of the fix purpose

Potential Issues

⚠️ Minor: Expression Pattern Matching

The fix handles MethodCallExpression and UnaryExpression cases, but there might be edge cases with other expression types. However, this matches the existing pattern used elsewhere in the codebase (line 122-130 in the same file), so it's consistent.

✅ Verification Against Guidelines

• Dual-mode requirement: ✅ Both execution paths updated
• Performance first: ✅ More efficient than previous approach
• Thread safety: ✅ No shared state introduced
• AOT compatibility: ✅ Maintains existing patterns

Recommendation

APPROVE - This is a well-implemented bug fix that:

1. Correctly addresses the issue: Dynamic test arguments now work as expected
2. Follows framework architecture: Maintains dual-mode execution requirement
3. Improves performance: More efficient than expression compilation
4. Includes proper testing: Comprehensive test coverage for the fix
5. Maintains code quality: Clean, readable, and well-documented changes

The fix demonstrates excellent understanding of the TUnit architecture and follows all established patterns and guidelines.

---