-
-
Notifications
You must be signed in to change notification settings - Fork 92
Fix SharedType.PerTestSession not being respected with inheritance #3082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
…ourceGenerator Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
SharedType.PerTestSession
not being respected with inheritance
Claude finished @Copilot's task —— View job PR Review: Fix SharedType.PerTestSession inheritance
Todo List
Code Review SummaryThis PR successfully addresses a critical inheritance bug in the ✅ The Fix is CorrectRoot Cause Identified: Line 276 in 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 AssessmentStrengths
Test Coverage
🔍 Technical AnalysisSecurity: ✅ No concerns
Performance: ✅ No impact
Thread Safety: ✅ No issues
Potential Edge Cases: ✅ Well handled
📝 Minor Observations
🎯 Recommendation✅ APPROVE - This is a high-quality fix that:
The change is minimal, targeted, and addresses the exact issue described in #3064 without introducing any side effects. |
The
SharedType.PerTestSession
sharing behavior was not working correctly whenClassDataSource
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: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 theContainingType
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 forPerTestSession
.Fix
Updated the source generator to use the correct containing type:
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.