perf: use TryAdd in TestDependencyResolver dependency dedupe by thomhurst · Pull Request #6068 · thomhurst/TUnit (original) (raw)

Follow-up Review (after second commit 57dc8bc)

The second commit addressed the previous finding: ResolveDependenciesForTestLockFree now also uses TryAdd. Both sibling methods are consistent. The core change is correct and complete.

Remaining observations

1. ResolvePendingDependencies iterates a collection it may indirectly mutate

This is pre-existing (not introduced by this PR), but since the file is in scope I'll flag it concretely. The method at lines 175–182:

private void ResolvePendingDependencies() { _testsWithPendingDependencies.RemoveWhere(static test => test.Dependencies.Length > 0); foreach (var test in _testsWithPendingDependencies) { ResolveDependenciesForTest(test); // can call _testsWithPendingDependencies.Add(test) on failure } }

ResolveDependenciesForTest calls _testsWithPendingDependencies.Add(test) when a dependency still cannot be found. Because _testsWithPendingDependencies is a HashSet\<T> and the re-added test was just iterated, the Add happens to be a no-op in practice (same reference, already in the set), but the enumerator is technically invalidated by any structural change. On .NET this throws InvalidOperationException in debug builds if the collection is modified during iteration. The fact that re-adding an existing element does not change the backing store is an implementation detail of HashSet\<T> — it does not mutate the version counter — but relying on that is fragile and non-obvious.

Safer alternative: snapshot before iterating:

private void ResolvePendingDependencies() { _testsWithPendingDependencies.RemoveWhere(static test => test.Dependencies.Length > 0); foreach (var test in _testsWithPendingDependencies.ToList()) { ResolveDependenciesForTest(test); } }

Or, since ResolveAllDependencies already retries in a loop, it might be simpler to have ResolvePendingDependencies return early without iterating and let the retry loop drive re-attempts.

2. FindMatchingTests — redundant null check on dependency.MethodName after string.IsNullOrEmpty

Lines 148–157:

else if (!string.IsNullOrEmpty(dependency.MethodName)) { if (dependency.MethodName != null && _testsByMethodName.TryGetValue(...))

The inner dependency.MethodName != null check is unreachable: the outer !string.IsNullOrEmpty guard already implies the value is non-null and non-empty. Minor, but adds noise.

3. Minor: Parallel.ForEach lambda can be a static method reference

Parallel.ForEach(testsWithDependencies, test => { ResolveDependenciesForTestLockFree(test); });

The lambda captures this to call the instance method. With the method group form this is still the same allocation, but it can be expressed more cleanly as:

Parallel.ForEach(testsWithDependencies, ResolveDependenciesForTestLockFree);

This is purely cosmetic but consistent with TUnit's style of keeping hot-path code tight.

Summary

The PR goal is complete — both deduplication sites now use TryAdd. Items 1–3 above are pre-existing or cosmetic; item 1 is worth a follow-up issue. No blockers.