test: Add large-scale pipeline tests (100+ modules)

[thomhurst]

Summary

• Adds ScaleTests.cs with 4 large-scale pipeline tests
• Tests 100 independent modules, 50-deep chains, fan-out (1+50), and fan-in (50+1) patterns
• Uses ExecutionTracker with ConcurrentDictionary for thread-safe module tracking
• Uses generic types with marker structs for unique module types

Closes #2002

🤖 Generated with Claude Code

[thomhurst]

Summary

This PR adds large-scale pipeline tests to verify scalability with 100+ modules across 4 test scenarios: independent modules, deep chains, fan-out, and fan-in patterns.

Critical Issues

1. Static mutable state causes test isolation issues

The ExecutionTracker uses static fields that are shared across all test instances. While Reset() is called at the beginning of each test, this creates a race condition risk. Tests are marked with [NotInParallel] to prevent parallel execution, but static state violates test isolation principles.

Recommendation: Consider using instance-level tracking or test-scoped services via dependency injection instead of static state.

2. Missing test cleanup verification

The tests verify CompletedCount matches expected counts, but don't verify that the ExecutionTracker was properly reset between tests. If a test fails mid-execution, the next test might see stale state.

Suggestions

1. Add validation for dependency ordering in chain test

The test Pipeline_With50ModuleDeepChain_CompletesInOrder should verify that modules actually execute in sequential order (Chain1 before Chain2, etc.), not just that all 50 complete.

2. Consider performance/timing assertions

For scale tests, it would be valuable to assert that parallel tests complete significantly faster than serial equivalents.

3. Add test for error propagation at scale

Consider adding a test where one module in a large chain fails to verify that failure handling works correctly at scale.

Verdict

⚠️ REQUEST CHANGES - Critical issue #1 (static mutable state) should be addressed or explicitly acknowledged as a known limitation.

[thomhurst]

Summary

Adds comprehensive scale testing for ModularPipelines with 100+ module scenarios testing independent execution, dependency chains, fan-out, and fan-in patterns.

Critical Issues

None found ✅

Previous Review Status

The previous review raised concerns about static mutable state. Reviewing the current code:

✅ RESOLVED: ExecutionTracker uses instance-level state, not static

• ExecutionTracker is a regular class with instance fields (ConcurrentDictionary, int counter)
• Fresh instance created per test via var tracker = new ExecutionTracker()
• Properly injected into modules via DI: .AddSingleton(tracker)
• await Assert.That(tracker.IsClean).IsTrue() verifies clean state at test start

The static concern appears to have been based on a misreading or has since been fixed.

✅ Test isolation: Each test creates its own tracker instance, ensuring proper isolation

Suggestions

1. Add dependency ordering validation in chain test (line 323)

The deep chain test verifies all 50 modules complete but doesn't validate sequential execution order. Consider adding:

// Verify sequential execution order
for (int i = 1; i < 50; i++)
{
    var current = tracker.GetRecord($"Chain{i}");
    var next = tracker.GetRecord($"Chain{i+1}");
    await Assert.That(current!.EndTime).IsLessThanOrEqualTo(next!.StartTime);
}

This would verify Chain1 completes before Chain2 starts, validating dependency enforcement.

2. Minor: ChainModule classes are repetitive (lines 227-290)

625 lines of nearly-identical module class definitions could be simplified with source generators or reflection-based module creation. However, for test code clarity and debuggability, explicit definitions may be preferable.

Verdict

✅ APPROVE - No critical issues, well-designed scale tests with proper isolation