perf: engine-wide performance optimizations

[thomhurst]

Summary

• Fix ConstraintKeysCollection hash/equals: GetHashCode() was returning constant 1 with no documentation. Replaced LINQ Intersect().Any() in Equals with allocation-free nested loop (optimized for typical 1-2 constraint keys). Hash remains a constant (0) with a comment explaining why — intersection-based equality makes content-based hashing impossible.
• Replace O(n²) topological sort in TestDiscoveryService.DiscoverTestsFullyStreamingAsync with Kahn's algorithm using in-degree counting and reverse-dependency map — O(V+E).
• Replace ConcurrentBag with ConcurrentQueue in TestDiscoveryService._cachedTests for O(n) enumeration without snapshot copying.
• Single-pass categorization in GitHubReporter.AfterRunAsync — replaces 6 separate LINQ passes over all test results with one switch-based loop.
• Thread-safety fixes in GitHubReporter and JUnitReporter — List<T> replaced with ConcurrentQueue<T>, with a parallel _latestUpdates dictionary for O(1) last-element access. Removed dead _updates field from JUnitReporter.
• Eliminate GroupBy + Count() in EventReceiverOrchestrator and TestLifecycleCoordinator — replaced with single-pass counter initialization loops.
• Cache reflection results: ExplicitAttribute class lookup, ConstructorInfo for TreeNodeFilter (with explicit typed lookup), static PropertyBag, and regex-stripped filter strings in MetadataFilterMatcher.
• Guard trace logging in TestGroupingService — wrap all trace log sites with IsTraceEnabled checks to avoid string interpolation when tracing is off. Extract FormatParallelLimiterInfo helper to deduplicate 4 identical snippets.
• Prevent memory leak: Add TestContext.RemoveById() and call it after test completion to clean up the static _testContextsById dictionary in long-running test host scenarios.
• Remove redundant hook call: EnsureTestSessionHooksExecutedAsync was called in TestCoordinator but already runs unconditionally in TestExecutor.ExecuteAsync.
• Fast-path XML sanitization: Scan for invalid chars first in JUnitXmlWriter.SanitizeForXml; return original string unchanged (zero allocation) when clean.
• Static lambdas: Make factory lambdas static in reporters to avoid delegate allocations.
• Remove LINQ from BatchResolveDependencies: Replace .Where().ToList() with manual loop.
• Pre-allocated array fill in TestScheduler — remove redundant .OrderBy() on already-sorted SortedDictionary.
• Comparer deduplication: ConstraintKeysCollectionEqualityComparer now delegates to instance Equals/GetHashCode instead of duplicating the logic.
• Use Assembly reference as dictionary key in EventReceiverOrchestrator instead of Assembly.GetName().FullName string.

Test plan

• Full CI passes (all target frameworks: net8.0, net9.0, net10.0)
• Run TUnit.TestProject with filtered tests to verify discovery and execution
• Verify GitHubReporter output in a GitHub Actions run
• Verify JUnit XML output is well-formed
• Run tests with [DependsOn] to verify topological sort correctness
• Run tests with [NotInParallel] constraint keys to verify hash-based grouping

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 2 minor

Alerts:
⚠ 3 issues (≤ 0 issues of at least minor severity)

Results:
3 new issues

Category	Results
ErrorProne	1 high
CodeStyle	2 minor

View in Codacy

🟢 Metrics 21 complexity

Metric	Results
Complexity	21

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review: perf/engine-wide-performance-optimizations

Overall this is a solid, well-motivated PR with real improvements across the board. The O(n²) → O(V+E) topological sort, ConcurrentBag → ConcurrentQueue, single-pass categorization, and logging guard fixes are all clearly correct. A few areas warrant attention.

---

1. [Medium] ConstraintKeysCollection.GetHashCode() violates the hash/equals contract

The new XOR hash is inconsistent with the intersection-based Equals semantics. Consider:

var a = new ConstraintKeysCollection(["keyA", "keyB"]);  // hash = H("keyA") ^ H("keyB")
var b = new ConstraintKeysCollection(["keyB", "keyC"]);  // hash = H("keyB") ^ H("keyC")

a.Equals(b); // true — they share "keyB"
a.GetHashCode() == b.GetHashCode(); // almost certainly false

The contract "equal objects must have equal hashes" is broken for multi-key collections. The original return 1 was degenerate but technically consistent with intersection-equality.

Why this matters: Any code using ConstraintKeysCollectionComparer as a Dictionary or HashSet comparer will silently misbehave (miss lookups) for multi-key collections. The static comparer property is public API surface inviting exactly that use case.

Better approach: Since the equality semantics is "these two tests share a constraint key and therefore cannot run in parallel together", the right fix is probably to change Equals to mean set equality (not intersection), which would allow a well-defined consistent hash. If intersection semantics must be preserved, the hash must be a function solely of the intersection — which can't be computed without knowing the other object, making consistent hashing impossible. In that case, return 0 (not return 1) with a comment explaining the intentional degenerate hash is the only correct option.

---

2. [Low-Medium] SingleOrDefault<TestNodeStateProperty>() can throw

GitHubReporter.cs and JUnitReporter.cs (implicitly)

var state = kvp.Value.TestNode.Properties.SingleOrDefault<TestNodeStateProperty>();
switch (state)
{
    case FailedTestNodeStateProperty or ErrorTestNodeStateProperty:
    ...

SingleOrDefault<T>() throws InvalidOperationException if the sequence contains more than one element of type T. The original six separate .Any() checks were more defensive and would silently categorise a test with multiple state properties (however unusual).

Better approach: Use FirstOrDefault<T>() (or .OfType<TestNodeStateProperty>().FirstOrDefault()) to be safe:

var state = kvp.Value.TestNode.Properties.OfType<TestNodeStateProperty>().FirstOrDefault();

This preserves the performance intent while avoiding a potential crash if the platform contract is looser than assumed.

---

3. [Low] _latestUpdates has a minor ordering race

In ConsumeAsync:

_updates.GetOrAdd(uid, static _ => []).Enqueue(testNodeUpdateMessage);
_latestUpdates[uid] = testNodeUpdateMessage;

These two operations are not atomic. If two updates arrive concurrently for the same test, the value in _latestUpdates might end up as the message from the first-completing thread, not the last one enqueued. In practice, test state transitions are monotonic (InProgress → terminal state), so this should rarely matter. Worth a code comment noting the intentional trade-off so a future reader doesn't "fix" it with a lock.

(Note: the original List<T>.Add() was a genuine data corruption race — this PR correctly moves to ConcurrentQueue, so this is a minor improvement over before.)

---

4. [Informational] Kahn's algorithm — cycle-break fallback is correct but undocumented

The cycle-break at the end:

if (yieldedCount < dependentTests.Count)
{
    foreach (var test in dependentTests)
    {
        if (!yieldedTests.Contains(test.TestId)) { yield return test; }
    }
}

This correctly handles cycles, but note: tests in a cycle will be yielded without respecting their dependency order, which means [DependsOn] tests in a cycle may start before their declared dependencies finish. This matches the old code's fallback behaviour, but a log warning here would make the breakage visible:

if (yieldedCount < dependentTests.Count)
{
    _logger.LogWarning("Circular dependency detected — some [DependsOn] tests will run out of order");
    ...
}

---

5. [Positive] Cleanly removed redundant EnsureTestSessionHooksExecutedAsync call

Confirmed TestExecutor.ExecuteAsync (line 80) already calls this unconditionally before any test work. The removal from TestCoordinator is safe and correct.

6. [Positive] TestScheduler — removing the redundant OrderBy on SortedDictionary

SortedDictionary<int, ...>.Values already enumerates in ascending key order. The removal of OrderBy(t => t.Key) is correct.

7. [Positive] TestContext.RemoveById placement is correct for retries

The call is at the very end of ExecuteTestInternalAsync, after all retry handling completes. No retry iteration will observe a missing context.

[claude]

Follow-up Review: perf/engine-wide-performance-optimizations

Previous review issues have been largely addressed by the fix commit (5cddaccc). Here's an assessment of each, plus new findings.

---

Previous Review — Resolution Status

• ✅ Issue #1 (Hash/equals contract): Correctly reverted GetHashCode() to a constant (return 0) with an explanatory comment. The comment accurately explains why a content-based hash would violate the contract given intersection-based Equals semantics.
• ✅ Issue #2 (SingleOrDefault can throw): Replaced with OfType<TestNodeStateProperty>().FirstOrDefault() — correct fix.
• ✅ Issue #3 (_latestUpdates ordering race): Acknowledged with a code comment explaining the intentional trade-off. Acceptable.
• ✅ Issue #4 (Cycle-break fallback): Comment updated to note that CircularDependencyDetector handles error reporting. Good.

---

New Issue: _updates field is dead code in both reporters [Medium]

GitHubReporter.cs and JUnitReporter.cs

After the refactor, _updates (the full history dictionary) is still populated in ConsumeAsync but is never read in AfterRunAsync. All reporting now goes through _latestUpdates. This means the entire update history for every test accumulates in memory for the lifetime of the reporter, serving no purpose.

// ConsumeAsync — writes to both:
_updates.GetOrAdd(uid, static _ => []).Enqueue(testNodeUpdateMessage);  // ← never read
_latestUpdates[uid] = testNodeUpdateMessage;                             // ← actually used

// AfterRunAsync — reads only _latestUpdates:
foreach (var kvp in _latestUpdates) { last[kvp.Key] = kvp.Value; }

Why this matters: In a large test suite with thousands of tests, each emitting multiple lifecycle updates (InProgress, then a terminal state), _updates grows unbounded and is garbage-collected only when the reporter is torn down. This partially undoes the memory improvement this PR intended to deliver.

Better approach: Remove _updates entirely from both reporters. If full update history is ever needed in future, it can be re-introduced intentionally. Alternatively, if there's a reason to keep _updates (e.g., planned future use), add a comment explaining why.

---

New Issue: PR description inconsistency [Low / Documentation]

The PR description still says:

Fix critical hash degradation: ConstraintKeysCollection.GetHashCode() was returning constant 1... Now uses XOR-based order-independent hash

But the actual code (after the fix commit) still uses a constant: return 0. The XOR approach was correctly reverted. The PR description should be updated to reflect the actual change: the constant hash is now documented with an explanatory comment, and return 1 → return 0 is a cosmetic/clarity change only.

---

Minor: _treeNodeFilterConstructor indexer assumption [Low]

MetadataFilterMatcher.cs

private static readonly ConstructorInfo _treeNodeFilterConstructor =
    typeof(TreeNodeFilter).GetConstructors(BindingFlags.NonPublic | BindingFlags.Instance)[0];

Using [0] assumes the first non-public instance constructor is the string-parameter one. If TreeNodeFilter ever gains an additional private constructor (e.g., a parameterless one for serialization), this silently picks the wrong constructor. The caching improvement is worthwhile; the selection is fragile.

More robust approach:

private static readonly ConstructorInfo _treeNodeFilterConstructor =
    typeof(TreeNodeFilter).GetConstructor(
        BindingFlags.NonPublic | BindingFlags.Instance,
        [typeof(string)])!;

This is explicit about which constructor signature is expected and will throw immediately at startup if the constructor is removed, rather than silently misbehaving at runtime.

---

Positives

• FormatParallelLimiterInfo helper cleanly eliminates 4 copy-pasted snippets. Good refactor.
• IsTraceEnabled guards throughout TestGroupingService — avoids string interpolation on every test when trace logging is disabled. This is the correct pattern.
• Kahn's algorithm in TestDiscoveryService is correctly implemented. The in-degree and reverse-dependency map construction handles pre-yielded independent tests correctly (their dependencies start at in-degree 0 and are seeded into readyQueue immediately).
• Assembly as dictionary key in EventReceiverOrchestrator is correct — Assembly objects use reference equality within an AppDomain, so the same loaded assembly always maps to the same key, avoiding repeated GetName().FullName string allocations.
• SortedDictionary OrderBy removal in TestScheduler is correct. SortedDictionary<TKey, TValue>.Values already enumerates in ascending key order.

---

Summary

The fix commit successfully addresses all four issues from the first review. The main new finding worth acting on is the dead _updates field in both reporters — it accumulates unbounded history that is never consumed. Removing it would be a genuine improvement that aligns with the PR's stated goal of reducing memory usage. The other two findings (description inconsistency and constructor indexer) are low-priority.

[claude]

Follow-up Review (3rd Pass)

All issues raised in the two prior reviews have been addressed. Here is the current status and any remaining observations.

---

Previous Review — Final Resolution Status

• ✅ Hash/equals contract — GetHashCode() returns constant 0 with an explanatory comment. PR description updated to match (no more XOR mention).
• ✅ SingleOrDefault can throw — Both occurrences in GitHubReporter replaced with .OfType<TestNodeStateProperty>().FirstOrDefault().
• ✅ _latestUpdates ordering race — Documented with a code comment explaining the intentional trade-off.
• ✅ Cycle-break fallback — Comment updated noting that CircularDependencyDetector handles the error path.
• ✅ Dead _updates in JUnitReporter — Removed in the fix commit. JUnitReporter now cleanly uses only _latestUpdates.
• ✅ _treeNodeFilterConstructor fragile indexer — Fixed. Now uses explicit typed GetConstructor(BindingFlags..., [typeof(string)]) that will fail loudly at startup if the API changes.

---

Clarification: GitHubReporter _updates is NOT dead code

The second review incorrectly flagged _updates in GitHubReporter as dead. It is still actively read in AfterRunAsync's flaky-detection loop:

foreach (var update in kvp.Value)   // iterating _updates full history
{
    var state = update.TestNode.Properties.OfType<TestNodeStateProperty>().FirstOrDefault();
    if (state is not null and not InProgressTestNodeStateProperty and not DiscoveredTestNodeStateProperty)
        finalStateCount++;
}

This is the correct design: _latestUpdates gives O(1) final state for bulk pass/fail categorisation; _updates provides the full per-test update history for counting retry/state transitions (flakiness detection). The fix commit rightly removed _updates from JUnitReporter (which never needed the history) while leaving it intact in GitHubReporter (which does).

---

One Minor Observation

_strippedFilterCache (MetadataFilterMatcher) is an unbounded static cache. In practice, filter strings are set at test run startup and bounded, so this will never grow uncontrollably. Worth keeping in mind only if TUnit ever moves to a long-running hot-reload server model where filters could accumulate. No action needed now — just noting for future awareness.

---

Overall Assessment

This PR is in excellent shape. The changes are well-motivated, correctly implemented, and all review concerns have been resolved. The combination of Kahn's O(V+E) topological sort, elimination of per-call reflection, logging guards, ConcurrentBag → ConcurrentQueue, and single-pass categorisation represents a meaningful, production-quality performance improvement.

Approved.

[claude]

Follow-up Review (4th Pass)

This review covers the fourth commit (de9cbf07e9), which landed after the 3rd-pass approval.

---

Changes in the 4th commit

GitHubReporter — replace full update queue with int counter

The previous fix introduced a ConcurrentQueue<TestNodeUpdateMessage> to store full update history (replacing the original List<T> data-corruption race). This commit replaces it with a ConcurrentDictionary<string, int> _terminalStateCounts that only counts terminal state transitions. This is correct:

• Flaky detection only needed a count, not the full message history. The full message for display was already in _latestUpdates.
• Memory reduction is significant: instead of N TestNodeUpdateMessage objects per flaky test, only one int per test.
• Thread-safety is equivalent: AddOrUpdate with a static lambda is already the pattern used elsewhere in this file.

The minor non-atomic gap between _terminalStateCounts.AddOrUpdate and _latestUpdates[uid] = remains, but this was already acknowledged in the 2nd review and documented there — the monotonic nature of test state transitions makes a stale read harmless at report time.

TestScheduler — added braces to nested foreach

Correctness-only; no logic change.

Removed narrating comments

Appropriate cleanup. The removed comments described what the code does rather than why, which adds noise without value.

---

Final Status of All Prior Issues

• ✅ Hash/equals contract — constant return 0 with explanatory comment
• ✅ SingleOrDefault → OfType<>.FirstOrDefault() — safe
• ✅ _latestUpdates ordering race — documented intentional trade-off
• ✅ Cycle-break fallback — CircularDependencyDetector noted in comment
• ✅ Dead _updates in JUnitReporter — removed
• ✅ Dead _updates in GitHubReporter — replaced with lightweight int counter
• ✅ _treeNodeFilterConstructor indexer — explicit typed lookup

---

Overall Assessment

The PR is clean and complete. All four commits tell a coherent story: initial optimization → two rounds of fixes → lightweight cleanup. The final state delivers the stated goals (Kahn's sort, ConcurrentBag → ConcurrentQueue, single-pass categorization, allocation-free fast paths) without introducing new issues.

Approved.