Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 9, 2025

The SharedType.PerTestSession sharing behavior was not working correctly when ClassDataSource attributes were defined on intermediate base classes in an inheritance hierarchy.

Problem

When using inheritance with ClassDataSource properties, the sharing mechanism would fail if the attributes were moved from the concrete class to an abstract base class:

// This worked correctly
public class TestingServer : BaseApplicationTestingServer<Program>
{
    [ClassDataSource<PostgreSqlContainerWrapper>(Shared = SharedType.PerTestSession)]
    public required PostgreSqlContainerWrapper PostgresContainer { get; set; }
}

// But this broke PerTestSession sharing
public abstract class ModuleTestingServer : BaseApplicationTestingServer<Program>  
{
    [ClassDataSource<PostgreSqlContainerWrapper>(Shared = SharedType.PerTestSession)]
    public required PostgreSqlContainerWrapper PostgresContainer { get; set; }
}

public class TestingServer : ModuleTestingServer
{
    // Inherits the ClassDataSource property
}

In the broken scenario, containers would be disposed after the first test class finished instead of being shared across the entire test session.

Root Cause

The PropertyInjectionSourceGenerator was incorrectly setting the ContainingType metadata to the concrete class (classSymbol) rather than the actual class where the property was declared (propInfo.Property.ContainingType). This caused the test class type resolution to vary between concrete classes, breaking the global sharing behavior expected for PerTestSession.

Fix

Updated the source generator to use the correct containing type:

// Before (incorrect)
sb.AppendLine($"ContainingType = typeof({classSymbol.ToDisplayString()}),");

// After (correct)
sb.AppendLine($"ContainingType = typeof({propInfo.Property.ContainingType.ToDisplayString()}),");

This ensures consistent sharing behavior across inheritance hierarchies by using the type where the property was actually declared, not where it's being consumed.

Fixes #3064.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits September 9, 2025 23:57
Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
…ourceGenerator

Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] SharedType.PerTestSession not being respected with inheritance Fix SharedType.PerTestSession not being respected with inheritance Sep 10, 2025
@Copilot Copilot AI requested a review from thomhurst September 10, 2025 00:04
Copilot finished work on behalf of thomhurst September 10, 2025 00:04
@thomhurst thomhurst marked this pull request as ready for review September 10, 2025 00:09
Copy link
Contributor

claude bot commented Sep 10, 2025

Claude finished @Copilot's task —— View job


PR Review: Fix SharedType.PerTestSession inheritance

Todo List

  • Read and analyze the PropertyInjectionSourceGenerator.cs changes
  • Review the test files for coverage and quality
  • Assess code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Review security concerns
  • Provide comprehensive feedback

Code Review Summary

This PR successfully addresses a critical inheritance bug in the PropertyInjectionSourceGenerator where SharedType.PerTestSession was not working correctly when ClassDataSource attributes were defined on base classes.

The Fix is Correct

Root Cause Identified: Line 276 in PropertyInjectionSourceGenerator.cs was incorrectly using the concrete class type (classSymbol) instead of the property's actual containing type (propInfo.Property.ContainingType).

Solution: Changed from:

sb.AppendLine($"ContainingType = typeof({classSymbol.ToDisplayString()}),");

To:

sb.AppendLine($"ContainingType = typeof({propInfo.Property.ContainingType.ToDisplayString()}),");

This ensures consistent type resolution for sharing behavior across inheritance hierarchies.

📋 Code Quality Assessment

Strengths

  • Minimal, targeted fix: Only changes the problematic line without affecting other functionality
  • Consistent with existing patterns: The fix aligns with how UnsafeAccessor methods already use ContainingType (lines 234, 318, 328, 335)
  • Maintains behavioral parity: Ensures source-generated and reflection modes produce identical results
  • Performance-conscious: No performance impact from the change

Test Coverage

  • Comprehensive test scenarios: Tests cover both working and broken inheritance patterns
  • Real-world reproduction: InheritanceSharedTypeRepro.cs provides a thorough reproduction with multiple test classes
  • Simple verification: SimpleInheritanceTest.cs offers a minimal repro case
  • Proper test structure: Uses NotInParallel with ordering to ensure deterministic execution
  • Clear verification: Tests validate that containers aren't disposed prematurely

🔍 Technical Analysis

Security: ✅ No concerns

  • No security implications from this change
  • Continues to use safe type resolution practices

Performance: ✅ No impact

  • Change is purely compile-time metadata generation
  • No runtime performance implications
  • Maintains efficient source generation patterns

Thread Safety: ✅ No issues

  • Generator code is stateless and thread-safe
  • No shared mutable state introduced

Potential Edge Cases: ✅ Well handled

  • The fix correctly handles complex inheritance hierarchies
  • Works with both init-only and settable properties
  • Maintains compatibility with UnsafeAccessor usage
  • Handles generic types and nullable references properly

📝 Minor Observations

  1. Code consistency: The fix brings line 276 in alignment with the existing usage of ContainingType throughout the file (lines 234, 318, 328, 335)

  2. Test quality: The test files are well-structured with clear naming conventions and comprehensive coverage of the inheritance scenarios

  3. Documentation: The PR description clearly explains the problem, root cause, and solution

🎯 Recommendation

✅ APPROVE - This is a high-quality fix that:

  • Correctly identifies and resolves the root cause
  • Maintains code consistency and performance
  • Includes comprehensive test coverage
  • Follows TUnit's coding standards and patterns
  • Has no security or performance concerns

The change is minimal, targeted, and addresses the exact issue described in #3064 without introducing any side effects.

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.

SharedType.PerTestSession not being respected with inheritance

2 participants