feat: add SkipIfEmpty property to data source attributes to control test execution behavior

[thomhurst]

Fixes #3448

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3520: feat: add SkipIfEmpty property to data source attributes to control test execution behavior

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze core interface changes (IDataSourceAttribute)
• Review attribute implementations for SkipIfEmpty property
• Examine engine logic changes (TestBuilder.cs)
• Check test coverage and quality
• Verify dual-mode implementation (source-gen + reflection)
• Assess performance impact
• Check for security concerns
• Provide comprehensive feedback

---

Review Summary

This PR successfully implements the SkipIfEmpty property for data source attributes, allowing tests to be skipped when data sources return no data instead of failing. The implementation is well-designed and follows TUnit's development guidelines.

✅ Positive Aspects

1. Comprehensive Implementation

• ✅ Correctly implements the SkipIfEmpty property across all data source attribute types
• ✅ Updates the core IDataSourceAttribute interface consistently
• ✅ Proper inheritance implementation in derived attributes

2. Engine Logic Integration

• ✅ Well-implemented logic in TestBuilder.cs lines 395-440 and 444-489
• ✅ Creates skipped tests with appropriate reasons ("Data source returned no data")
• ✅ Handles both class and method data sources correctly
• ✅ Proper fallback for generic type resolution when skipping

3. Test Coverage

• ✅ Good test coverage in EmptyDataSourceTests.cs
• ✅ Tests verify both normal behavior and skip behavior
• ✅ Includes edge case testing (EmptyStaticDataSource_WithSkipIfEmpty_ShouldBeSkipped)

4. API Consistency

• ✅ Public API snapshots properly updated across all target frameworks
• ✅ Consistent property implementation with proper inheritance
• ✅ Follows TUnit's code style guidelines

⚠️ Areas for Improvement

1. Source Generator Gap (CRITICAL)

This violates TUnit's mandatory dual-mode requirement

From CLAUDE.md: "ALL changes must work identically in both source-generated and reflection modes"

• ❌ No source generator implementation found for SkipIfEmpty logic
• ❌ Source generator tests don't cover the new functionality
• ❌ Risk: Feature works in reflection mode but fails in source-gen mode

Recommendation: The source generator needs to be updated to handle SkipIfEmpty property checks when generating test methods. Look at existing source generator patterns in TUnit.Core.SourceGenerator for implementing data source logic.

2. Documentation Clarity

• ℹ️ Interface documentation could specify default behavior more clearly
• ℹ️ Current: "When true, if the data source returns no data, the test will be skipped instead of failing"
• 💡 Suggestion: "When true, tests are skipped if the data source returns no data. When false (default), tests execute once with empty parameters."

3. Edge Case Considerations

• ❓ No explicit handling of SkipIfEmpty in NoDataSource (though it has the property)
• ❓ Interaction with RepeatAttribute when data source is empty and SkipIfEmpty=true

🔍 Technical Analysis

Performance Impact: ✅ Minimal

• Simple boolean check with minimal overhead
• Skip logic creates single test instance vs. multiple empty parameter tests
• Actually improves performance by avoiding unnecessary test execution

Security: ✅ No Concerns

• No external inputs or unsafe operations
• Standard boolean property with clear semantics

Thread Safety: ✅ Safe

• Property is read-only during test execution
• No shared mutable state

🚨 Blocking Issue

The missing source generator implementation is a blocking issue. According to TUnit's development guidelines, this feature cannot be merged until it works identically in both execution modes.

🔧 Required Actions

1. MUST FIX: Implement SkipIfEmpty logic in the source generator
2. MUST ADD: Source generator tests for the new functionality
3. SHOULD: Verify snapshot tests pass after source generator changes

Overall Assessment

This is a well-implemented feature that follows TUnit's patterns and provides valuable functionality. However, it has a critical gap in source generator support that must be addressed before merging.

Recommendation: ❌ Request changes - Fix source generator implementation before approval.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3520: feat: add SkipIfEmpty property to data source attributes to control test execution behavior

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze core interface changes (IDataSourceAttribute)
• Review attribute implementations for SkipIfEmpty property
• Examine engine logic changes (TestBuilder.cs)
• Check test coverage and quality
• Verify dual-mode implementation (source-gen + reflection)
• Assess performance impact
• Check for security concerns
• Provide comprehensive feedback

---

Review Summary

This PR successfully implements the SkipIfEmpty property for data source attributes, allowing tests to be skipped when data sources return no data instead of failing. The implementation is well-designed and follows TUnit's development guidelines.

✅ Positive Aspects

1. Comprehensive Implementation

• ✅ Correctly implements the SkipIfEmpty property across all data source attribute types
• ✅ Updates the core IDataSourceAttribute interface consistently
• ✅ Proper inheritance implementation in derived attributes

2. Engine Logic Integration

• ✅ Well-implemented logic in TestBuilder.cs lines 395-440 and 444-489
• ✅ Creates skipped tests with appropriate reasons ("Data source returned no data")
• ✅ Handles both class and method data sources correctly
• ✅ Proper fallback for generic type resolution when skipping

3. Test Coverage

• ✅ Good test coverage in EmptyDataSourceTests.cs
• ✅ Tests verify both normal behavior and skip behavior
• ✅ Includes edge case testing (EmptyStaticDataSource_WithSkipIfEmpty_ShouldBeSkipped)

4. API Consistency

• ✅ Public API snapshots properly updated across all target frameworks
• ✅ Consistent property implementation with proper inheritance
• ✅ Follows TUnit's code style guidelines

⚠️ Areas for Improvement

1. Source Generator Gap (CRITICAL)

This violates TUnit's mandatory dual-mode requirement

From CLAUDE.md: "ALL changes must work identically in both source-generated and reflection modes"

• ❌ No source generator implementation found for SkipIfEmpty logic
• ❌ Source generator tests don't cover the new functionality
• ❌ Risk: Feature works in reflection mode but fails in source-gen mode

Recommendation: The source generator needs to be updated to handle SkipIfEmpty property checks when generating test methods. Look at existing source generator patterns in TUnit.Core.SourceGenerator for implementing data source logic.

2. Documentation Clarity

• ℹ️ Interface documentation could specify default behavior more clearly
• ℹ️ Current: "When true, if the data source returns no data, the test will be skipped instead of failing"
• 💡 Suggestion: "When true, tests are skipped if the data source returns no data. When false (default), tests execute once with empty parameters."

3. Edge Case Considerations

• ❓ No explicit handling of SkipIfEmpty in NoDataSource (though it has the property)
• ❓ Interaction with RepeatAttribute when data source is empty and SkipIfEmpty=true

🔍 Technical Analysis

Performance Impact: ✅ Minimal

• Simple boolean check with minimal overhead
• Skip logic creates single test instance vs. multiple empty parameter tests
• Actually improves performance by avoiding unnecessary test execution

Security: ✅ No Concerns

• No external inputs or unsafe operations
• Standard boolean property with clear semantics

Thread Safety: ✅ Safe

• Property is read-only during test execution
• No shared mutable state

🚨 Blocking Issue

The missing source generator implementation is a blocking issue. According to TUnit's development guidelines, this feature cannot be merged until it works identically in both execution modes.

🔧 Required Actions

1. MUST FIX: Implement SkipIfEmpty logic in the source generator
2. MUST ADD: Source generator tests for the new functionality
3. SHOULD: Verify snapshot tests pass after source generator changes

Overall Assessment

This is a well-implemented feature that follows TUnit's patterns and provides valuable functionality. However, it has a critical gap in source generator support that must be addressed before merging.

Recommendation: ❌ Request changes - Fix source generator implementation before approval.

---