Extend method lookup to internal methods

[thomhurst]

Fixes #3337

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3343: "Extend method lookup to internal methods"

Todo List:

• Read and understand the main code changes
• Review the new test file
• Analyze snapshot changes to understand impact
• Provide feedback on code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage

Review Summary

This PR successfully extends TUnit's reflection-based method lookup to include internal methods by adding BindingFlags.NonPublic to the existing binding flags. The changes are minimal, focused, and well-tested.

🎯 Changes Made

The core changes are in two files:

TUnit.Core.SourceGenerator/CodeGenerationHelpers.cs:44-48

• Added BindingFlags.NonPublic to both GetMethods() and GetMethod() calls
• This allows discovery of internal methods with arguments during source generation

TUnit.Core.SourceGenerator/Utilities/MetadataGenerationHelper.cs:361,366

• Added BindingFlags.NonPublic to reflection calls in the metadata generation helper
• Ensures consistent behavior between source generation and reflection modes

✅ Code Quality & Best Practices

Excellent:

• Minimal, surgical changes that don't affect existing functionality
• Consistent application across both code paths (source-gen and reflection)
• Proper snapshot test updates showing the generated code changes
• Clean separation of concerns maintained

Good Practices Followed:

• Changes maintain the existing dual-mode implementation requirement (source-gen + reflection)
• All snapshot tests properly updated to reflect the new binding flags
• New test file demonstrates the functionality comprehensively

🐛 Potential Issues

Low Risk:

• The changes expose internal methods through reflection, which could potentially access more than intended if users have poorly designed internal APIs
• No validation that internal methods are actually test methods (but this is consistent with existing public method behavior)

No Significant Issues Found:

• Implementation correctly handles both static and instance methods
• Generic method handling remains intact
• Type parameter resolution unchanged

⚡ Performance Considerations

Positive:

• No performance regression expected - just expanding the search scope
• Still uses efficient GetMethod() with specific parameter types when possible
• Falls back to GetMethods().FirstOrDefault() only for generic methods (unchanged pattern)

Neutral:

• Slightly larger search space during reflection, but negligible impact
• Source generation remains compile-time, so no runtime impact there

🔒 Security Concerns

Low Impact:

• Change only affects test discovery, not production code execution
• Internal methods were already accessible within the same assembly
• No new attack vectors introduced

🧪 Test Coverage

Excellent Coverage:
The new InternalMethodWithArgumentsTests.cs covers:

• ✅ Internal methods with string arguments
• ✅ Internal methods with multiple argument sets
• ✅ Internal methods with no arguments
• ✅ Mix of internal and public methods
• ✅ Both Task and void return types

Comprehensive Snapshot Updates:

• 50 snapshot files updated showing the binding flags change propagated correctly
• Demonstrates the change affects all test discovery scenarios

📋 Recommendations

1. ✅ Approve - This is a clean, well-implemented fix
2. Consider: Adding a brief comment in the code explaining why NonPublic is included (e.g., "// Include internal methods for test discovery")
3. Future: Consider if there should be any opt-out mechanism for internal method discovery, though it's likely not needed

🎯 Verdict

This PR is ready to merge. It's a focused bug fix that solves the issue of internal test methods not being discovered, with excellent test coverage and proper dual-mode implementation. The changes are minimal, safe, and maintain all existing functionality while adding the requested capability.

The implementation correctly follows TUnit's architecture patterns and the extensive snapshot test updates provide confidence that the change works across all scenarios.

---