feat: enable conditional source generation based on configuration property

[thomhurst]

No description provided.

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Conditional Source Generation

I've reviewed this PR which adds conditional source generation based on the EnableTUnitSourceGeneration configuration property. Here's my comprehensive feedback:

✅ Code Quality & Best Practices

Excellent Implementation:

• ✅ Consistent Pattern: All 4 generators implement the same conditional logic pattern
• ✅ Modern C# Syntax: Uses proper incremental generator patterns with Combine() operations
• ✅ Early Exit Strategy: Generators exit early when disabled, avoiding unnecessary work
• ✅ Configuration Key: Uses standardized MSBuild property pattern (build_property.EnableTUnitSourceGeneration)

Specific Implementation Details:

// Consistent across all generators (AotConverter, HookMetadata, PropertyInjection, TestMetadata)
var enabledProvider = context.AnalyzerConfigOptionsProvider
    .Select((options, _) =>
    {
        options.GlobalOptions.TryGetValue("build_property.EnableTUnitSourceGeneration", out var value);
        return !string.Equals(value, "false", StringComparison.OrdinalIgnoreCase);
    });

✅ Performance Considerations

Positive Performance Impact:

• ✅ Hot Path Optimization: Aligns with TUnit's Rule 4 (Performance First) from CLAUDE.md
• ✅ Early Termination: When disabled, generators return immediately in RegisterSourceOutput
• ✅ Minimal Overhead: The configuration check is lightweight and cached via Combine()
• ✅ Incremental Generator Friendly: Uses proper incremental patterns that minimize recomputation

Performance Analysis by Generator:

• TestMetadataGenerator: Most impactful - skips expensive test discovery and metadata generation
• HookMetadataGenerator: Prevents costly hook registration and delegate generation
• PropertyInjectionSourceGenerator: Avoids property scanning and unsafe accessor generation
• AotConverterGenerator: Prevents expensive type scanning and conversion operator analysis

✅ Security Considerations

No Security Issues Identified:

• ✅ Safe Configuration Handling: Uses standard MSBuild property access
• ✅ No Code Injection: All generated code uses safe, compile-time patterns
• ✅ Proper String Handling: Case-insensitive comparison prevents bypass attempts

⚠️ Potential Issues & Recommendations

1. Dual-Mode Architecture Compliance (CRITICAL per CLAUDE.md Rule 1)

+ CONCERN: This only affects SOURCE-GENERATED mode
+ IMPACT: Could break dual-mode parity if reflection mode doesn't respect this setting
+ RECOMMENDATION: Verify TUnit.Engine also respects EnableTUnitSourceGeneration

2. Default Behavior Clarification

// Current logic: Default is ENABLED when property is missing/empty
return !string.Equals(value, "false", StringComparison.OrdinalIgnoreCase);

// Consider documenting the default behavior clearly
// Missing property = ENABLED (good for backwards compatibility)
// "false" = DISABLED
// Any other value = ENABLED

3. Snapshot Testing Impact (CRITICAL per CLAUDE.md Rule 2)

+ REQUIREMENT: This changes source generator output
+ ACTION NEEDED: Run TUnit.Core.SourceGenerator.Tests
+ ACTION NEEDED: Accept/update .verified.txt snapshots

✅ Test Coverage Assessment

Coverage Gaps to Address:

1. Unit Tests: Test the conditional logic with different configuration values
2. Integration Tests: Verify both enabled/disabled states produce correct behavior
3. Dual-Mode Tests: Ensure reflection mode respects the same configuration
4. Snapshot Tests: Update generator output snapshots

Recommended Test Cases:

[Test]
[Arguments("true", true)]
[Arguments("false", false)]
[Arguments("TRUE", true)]
[Arguments("FALSE", false)]
[Arguments(null, true)]  // Default behavior
[Arguments("", true)]    // Empty string behavior
[Arguments("other", true)] // Non-false values
public void EnableTUnitSourceGeneration_ParsesCorrectly(string value, bool expected) { }

🔧 Specific File Feedback

AotConverterGenerator.cs:15-20 ✅ Excellent integration with existing error handling
HookMetadataGenerator.cs:16-21 ✅ Proper placement before expensive hook discovery
PropertyInjectionSourceGenerator.cs:14-19 ✅ Good early exit in source output registration
TestMetadataGenerator.cs:24-29 ✅ Critical for large test suites - excellent placement

📋 Pre-Commit Checklist (per CLAUDE.md)

Based on CLAUDE.md requirements, before merging:

□ Source generator changed: Run TUnit.Core.SourceGenerator.Tests + accept snapshots
□ Dual-mode verification: Ensure TUnit.Engine respects same configuration
□ Performance verification: Benchmark large test suites with feature disabled
□ Documentation: Add EnableTUnitSourceGeneration to project documentation

🎯 Overall Assessment

Strengths:

• Consistent implementation across all generators
• Performance-first approach aligning with TUnit philosophy
• Clean, maintainable code following established patterns
• Proper incremental generator usage

Priority Actions:

1. HIGH: Verify dual-mode architecture compliance (Rule 1)
2. HIGH: Update source generator snapshots (Rule 2)
3. MEDIUM: Add comprehensive test coverage
4. LOW: Document the configuration option

✅ Final Recommendation

APPROVE with required actions: This is a solid implementation that provides valuable functionality for disabling source generation when needed. The code quality is excellent and follows TUnit's established patterns. However, the critical requirements from CLAUDE.md (dual-mode compliance and snapshot updates) must be addressed before merging.

---

Todo List:

• Read CLAUDE.md for TUnit-specific guidelines
• Analyze each modified source generator file
• Review conditional generation pattern consistency
• Check adherence to TUnit's dual-mode architecture requirements
• Assess performance implications
• Review security considerations
• Evaluate test coverage needs
• Provide comprehensive feedback

---