-
-
Notifications
You must be signed in to change notification settings - Fork 92
Refactor test invoker generation in source generator #3293
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
Claude finished @thomhurst's task —— View job Code Review AnalysisReviewing PR #3293: Refactor test invoker generation in source generator Todo List:
🔍 Code Review SummaryThis PR removes the ✅ Positive AspectsCode Quality & Best Practices
Architecture & Design
🔒 Security & SafetyRuntime Safety
Mitigation Required// Current problematic code:
var invokeTest = metadata.TestInvoker ?? throw new InvalidOperationException("Test invoker is null");
// Should be updated to:
var invokeTest = metadata.TestInvoker ??
(async (instance, args) => await metadata.InvokeTypedTest(instance, args, CancellationToken.None)); ⚡ Performance ImpactPositive
Neutral
🧪 Test Coverage AnalysisComprehensive Snapshot Updates
❗ Critical Issues Found
📋 RecommendationsHigh Priority
Medium Priority
🎯 VerdictExcellent refactoring concept with incomplete implementation. The architectural approach is sound and aligns with TUnit's performance-first philosophy, but the PR needs engine-side updates to handle the breaking change safely. Recommended Action: Request engine updates before merging to prevent runtime failures. |
Claude finished @thomhurst's task —— View job Code Review AnalysisReviewing PR #3293: Refactor test invoker generation in source generator Todo List:
🔍 Code Review SummaryThis PR removes the ✅ Positive AspectsCode Quality & Best Practices
Architecture & Design
🔒 Security & SafetyRuntime Safety
Mitigation Required// Current problematic code:
var invokeTest = metadata.TestInvoker ?? throw new InvalidOperationException("Test invoker is null");
// Should be updated to:
var invokeTest = metadata.TestInvoker ??
(async (instance, args) => await metadata.InvokeTypedTest(instance, args, CancellationToken.None)); ⚡ Performance ImpactPositive
Neutral
🧪 Test Coverage AnalysisComprehensive Snapshot Updates
❗ Critical Issues Found
📋 RecommendationsHigh Priority
Medium Priority
🎯 VerdictExcellent refactoring concept with incomplete implementation. The architectural approach is sound and aligns with TUnit's performance-first philosophy, but the PR needs engine-side updates to handle the breaking change safely. Recommended Action: Request engine updates before merging to prevent runtime failures. |
Removes redundant/unused/duplicated property