Fix hooks not properly awaiting in generic classes

[thomhurst]

Fixes #2935

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review Analysis

Reviewing this PR that fixes hooks not properly awaiting in generic classes.

Todo List:

• Read and analyze all changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage
• Provide comprehensive feedback

Review Summary

This PR fixes a critical bug where hooks in generic classes weren't properly awaiting async operations. The fix involves two main changes:

✅ Code Quality & Best Practices

TUnit.Core.SourceGenerator/Generators/HookMetadataGenerator.cs:627

• Good fix: Changed await AsyncConvert.ConvertObject(() => result) to await AsyncConvert.ConvertObject(result)
• Rationale: The lambda wrapper was unnecessary and potentially interfering with proper async conversion
• Clean implementation: The change is minimal and targeted

TUnit.Core/AsyncConvert.cs

• Improved simplicity: Removed complex Func handling that was overcomplicating the async conversion
• Better approach: Now directly handles Delegate types using DynamicInvoke() which is more reliable
• Code cleanup: Removed redundant whitespace and formatting inconsistencies

✅ Bug Fix Analysis

The root issue was in how the source generator was wrapping method invocation results:

// Before (problematic)
await AsyncConvert.ConvertObject(() => result);

// After (correct)  
await AsyncConvert.ConvertObject(result);

The lambda wrapper was preventing proper detection of async return types, especially problematic for generic classes where reflection is used.

✅ Test Coverage

Excellent test coverage with two comprehensive test files:

• GenericTests<T>: Tests generic class with parameterized type
• Tests: Control group testing non-generic class behavior
• Both test files include:
  • Both Task and ValueTask return types
  • Proper async/await patterns
  • State verification to ensure hooks actually executed
  • 5-second delays to ensure timing issues are caught

✅ Performance Considerations

• Positive impact: Removing unnecessary lambda allocation in hot path
• No regressions: Direct object handling is more efficient than func wrappers
• Reflection usage: Appropriately contained to generic scenarios only

✅ Security Concerns

• No security issues identified
• Proper error handling: Maintains existing exception propagation patterns
• AOT compatibility: Preserves existing AOT-friendly attributes and suppression

✅ Specific Code Review

AsyncConvert.cs changes are well-designed:

• Simplified control flow with early returns
• Proper null checking before delegate invocation
• Maintains compatibility with F# async and custom awaitables
• Clean formatting improvements

Source generator changes are minimal and correct:

• Single line change that directly addresses the root cause
• No side effects on other code paths

🔍 Minor Observations

1. Test timing: 5-second delays are quite long for unit tests, but appropriate for this specific bug verification
2. Test structure: Both test files follow consistent patterns, good for maintainability

Verdict

✅ APPROVE - This is a well-crafted fix that:

• Addresses the root cause directly
• Includes comprehensive test coverage
• Maintains code quality standards
• Has no security or performance concerns
• Follows TUnit's development guidelines

The changes demonstrate good understanding of the async conversion pipeline and provide a clean solution to the generic class hook awaiting issue.

---