Skip to content

Conversation

@thomhurst
Copy link
Owner

No description provided.

@claude
Copy link
Contributor

claude bot commented Oct 28, 2025

Claude finished @thomhurst's task —— View job


Pull Request Review: Dynamic Test Variants

I've completed a comprehensive review of this PR that introduces dynamic test variant registration for property-based testing scenarios.

🚨 CRITICAL ISSUES

1. Missing Dual-Mode Implementation (BLOCKER)

According to CLAUDE.md rule #1, ALL changes must work identically in both source-generated and reflection modes. However, this PR only implements the reflection mode:

  • TestRegistry.cs contains reflection-based implementation for CreateTestVariant
  • No source generator implementation found for runtime test variant creation
  • AOT attributes indicate reflection-only approach (RequiresUnreferencedCode)
  • This violates the mandatory requirement for dual-mode implementation

2. Potential Thread Safety Issues

In TestRegistry.cs:19, ConcurrentQueue<PendingDynamicTest> is used, but the processing in ProcessPendingDynamicTests() could have race conditions:

  • while (_pendingTests.TryDequeue(out var pendingTest)) could miss tests added between dequeue operations
  • Multiple threads calling ProcessPendingDynamicTests() simultaneously could cause duplicate processing

⚠️ MAJOR CONCERNS

3. Performance Implications

Several performance concerns in hot paths:

TUnit.Engine/Services/TestRegistry.cs:

  • Line 143-154: Heavy reflection usage in CreateTestVariant including GetMethod() with complex binding flags
  • Line 173-204: Expression tree compilation on every variant creation
  • Line 318: lambdaExpression.Compile() is expensive and not cached

TUnit.Engine/Services/DynamicTestQueue.cs:

  • Line 42: await _messageBus.Discovered(test.Context) on every enqueue could become a bottleneck
  • Unbounded channel could consume excessive memory under load

4. API Design Issues

TUnit.Core/Extensions/TestContextExtensions.cs:

  • Line 70: Missing null check for context.GetService<ITestRegistry>()
  • Default parameter relationship = Enums.TestRelationship.Derived might not be appropriate for all use cases

TUnit.Core/Interfaces/ITestRegistry.cs:

  • Line 47: Nullable object?[]? arguments parameter could be confusing vs empty array
  • Missing validation for conflicting parameters (e.g., null arguments with non-null properties)

POSITIVE ASPECTS

5. Good Architecture Decisions

  • Clean separation of concerns with IDynamicTestQueue interface
  • Proper use of channels for thread-safe queuing
  • Comprehensive documentation in the markdown file
  • Good use of AOT annotations where reflection is required

6. Test Coverage

  • TestVariantTests.cs provides basic coverage for the new functionality
  • Tests verify relationship tracking and parent test ID assignment

7. Documentation Quality

  • Excellent documentation in docs/docs/advanced/test-variants.md
  • Clear examples and use case explanations
  • Good distinction between test variants vs dynamic tests

🔧 RECOMMENDATIONS

Must Fix (Blockers):

  1. Implement source generator support for test variants to satisfy dual-mode requirement
  2. Fix thread safety in TestRegistry.ProcessPendingDynamicTests()
  3. Add performance optimizations - cache compiled expressions, batch message bus calls

Should Fix (Important):

  1. Add input validation in CreateTestVariant for argument/property combinations
  2. Implement expression caching to avoid repeated compilation costs
  3. Add comprehensive error handling for malformed variant requests
  4. Consider memory limits for the unbounded dynamic test queue

Consider:

  1. Batch processing for dynamic test discovery notifications
  2. Background processing for expensive reflection operations
  3. Metrics/telemetry for dynamic test creation and execution rates

Final Verdict

This PR introduces a valuable feature for property-based testing, but cannot be merged due to the missing source generator implementation, which violates TUnit's mandatory dual-mode requirement. The thread safety and performance issues also need addressing before this can be production-ready.


@thomhurst thomhurst merged commit 3bac242 into main Oct 28, 2025
13 checks passed
@thomhurst thomhurst deleted the feature/dynamic-pbt branch October 28, 2025 14:24
This was referenced Oct 28, 2025
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.

1 participant