Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #3813

@claude
Copy link
Contributor

claude bot commented Nov 12, 2025

Claude finished @thomhurst's task —— View job


Code Review Complete

Reviewing PR #3816: "feat: override tests via new keyword in derived classes"

Review Checklist:

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

Review Feedback

⚠️ Critical Issue: Missing Dual-Mode Implementation

This PR violates TUnit's fundamental Rule 1: Dual-Mode Implementation.

The feature is only implemented in the source generator path (TUnit.Core.SourceGenerator) but is missing the corresponding reflection mode implementation in TUnit.Engine. According to CLAUDE.md:

If you implement only ONE mode, the feature is INCOMPLETE and MUST NOT be committed.

Missing Implementation in Reflection Mode

The reflection path in ReflectionTestDataCollector.cs currently:

  1. ✅ Correctly gets all test methods via GetAllTestMethods()
  2. Does NOT filter out hidden methods when using [InheritsTestsAttribute]

The GetAllTestMethods() function collects all methods from the inheritance hierarchy but doesn't apply the same hiding logic that the source generator now has with IsMethodHiding().

Required Fix

You need to implement method hiding detection in the reflection path by:

  1. Adding an equivalent IsMethodHiding() function for MethodInfo types
  2. Updating the test method collection logic to filter out hidden base methods
  3. Ensuring identical behavior between both execution modes

📋 Code Quality Assessment

Strengths:

  1. Correct Implementation Logic: The IsMethodHiding() method properly identifies method hiding scenarios
  2. Performance Conscious: Uses SymbolEqualityComparer.Default for type comparisons
  3. Comprehensive Hiding Detection: Correctly checks name, parameters, inheritance chain, and excludes overrides
  4. Good Test Coverage: Includes practical test scenario showing the feature in action

Source Generator Changes Look Good:

  • Logic in IsMethodHiding() is sound: checks same name, non-override, matching parameters, inheritance relationship
  • CollectInheritedTestMethods() properly filters out hidden methods
  • Uses existing ParametersMatch() helper consistently

Issues Found:

  1. Missing Dual-Mode Implementation (Critical)
  2. No Snapshot Tests: Changes to source generator output require snapshot test updates per Rule 2
  3. Missing Comprehensive Testing: Only one basic test case; needs edge cases like:
    • Generic method hiding
    • Multiple levels of inheritance
    • Interface method hiding
    • Static method hiding
    • Methods with different parameter types

🔍 Potential Bugs

  1. Generic Type Handling: The ParametersMatch() method mentions handling generics, but it's unclear if IsMethodHiding() properly handles cases where base class is generic
  2. Interface Methods: The implementation doesn't appear to handle interface method hiding scenarios
  3. Static vs Instance: Need to verify static method hiding works correctly

🚀 Performance Considerations

  • ✅ Good: Reuses existing ParametersMatch() helper
  • ✅ Good: Uses efficient SymbolEqualityComparer
  • ✅ Good: Early returns in IsMethodHiding()
  • ⚠️ Potential: The inheritance chain walking could be cached for repeated calls

🔐 Security Concerns

No security issues identified - this is purely test discovery logic.

📝 Recommendations

  1. MUST DO: Implement reflection mode support - Add equivalent logic to TUnit.Engine/Discovery/ReflectionTestDataCollector.cs
  2. SHOULD DO: Update snapshot tests - Run dotnet test TUnit.Core.SourceGenerator.Tests and accept new snapshots
  3. SHOULD DO: Add comprehensive tests for edge cases mentioned above
  4. CONSIDER: Performance optimization - Cache inheritance relationships if this becomes a hot path

📚 Additional Context

The test scenario (BaseServiceTestsImplementationATests) is well-designed and demonstrates the real-world use case effectively. The [Skip] attribute on the overridden method makes practical sense.

This is a valuable feature that fills a gap in TUnit's inheritance support, but it needs the reflection mode implementation to be complete.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Source Generator Error when Overriding Inherited Tests

2 participants