[code-simplifier] refactor: simplify TestRunCache.OnTestCompletion null checks

[Jakub Jareš (nohwnd)]

Code Simplification — 2026-06-26

This PR simplifies OnTestCompletion in TestRunCache to improve clarity and correctness following recent changes in #16165.

Files Simplified

• src/Microsoft.TestPlatform.CrossPlatEngine/Execution/TestRunCache.cs — removed dead null check and applied project null-check conventions

Improvements Made

1. Removed dead-code null check

   • PR [efficiency-improver] perf: pre-allocate List(T) capacity in DiscoveryResultCache and TestRunCache #16165 changed _inProgressTests from ICollection<TestCase> to List<TestCase> (non-nullable). The guard _inProgressTests == null became unreachable after that change.
   • Per project coding standards: "Trust the C# null annotations and don't add null checks when the type system says a value cannot be null."

2. Applied is null / is not null conventions

   • completedTest == null → completedTest is null
   • inProgressTest != null → inProgressTest is not null
   • Per project coding standards: "Always use is null or is not null instead of == null or != null."

3. Fixed misleading warning message

   • "TestRunCache: InProgressTests is null" → "TestRunCache: InProgressTests is empty"
   • The condition was always checking Count == 0; the message now accurately describes what was detected.

Changes Based On

Recent changes from:

• [efficiency-improver] perf: pre-allocate List(T) capacity in DiscoveryResultCache and TestRunCache #16165 — [efficiency-improver] perf: pre-allocate List(T) capacity in DiscoveryResultCache and TestRunCache

Testing

• ✅ Build succeeds (0 errors, same pre-existing IL-trimming warnings)
• ✅ No functional changes — behaviour is identical; only unreachable code paths removed and log messages corrected

Review Focus

Please verify:

• The _inProgressTests == null branch was indeed unreachable (the field is always initialised to a new List<TestCase>(...) in the constructor and only ever replaced with another non-null list)
• Null-pattern syntax (is null / is not null) is consistent with the rest of the codebase
• Warning message text reflects the actual condition

Generated by Code Simplifier · 1.1K AIC · ⌖ 31.6 AIC · ⊞ 31.6K · ◷

• expires on Jun 27, 2026, 3:54 PM UTC

[Copilot]

Pull request overview

This PR simplifies TestRunCache.OnTestCompletion in the CrossPlatEngine execution layer by removing an unreachable null-check branch and aligning null-check syntax and diagnostics with established project conventions.

Changes:

• Replaced == null / != null with is null / is not null in OnTestCompletion.
• Removed the dead _inProgressTests == null guard (the field is always initialized to a non-null List<TestCase> and only re-assigned to another non-null list).
• Updated the warning text to correctly describe the Count == 0 condition (“empty” rather than “null”).

[Jakub Jareš (nohwnd)]

🧠 Reviewed by expert-reviewer · Parallel Execution & Scheduling Safety · Null Safety & Boundary Validation · Error Reporting & Diagnostic Clarity

All three changes are correct and safe. Here's the key reasoning:

_inProgressTests null guard removal

Every write to _inProgressTests assigns a fresh new List<TestCase>(...):

• Constructor (line 95): _inProgressTests = new List<TestCase>(InitialCapacity(cacheSize));
• SendResults() (line 343): _inProgressTests = new List<TestCase>(InitialCapacity(_cacheSize));

SendResults() is only ever invoked from CheckForCacheHit() / CheckForCacheHitOnTimer(), both of which are themselves called under lock (_syncObject). OnTestCompletion also holds the same lock. The field can therefore never be null at the point of the removed check — the guard was provably dead code after #16165 changed the field type from ICollection<TestCase> to List<TestCase>.

is null / is not null patterns

TestCase is a sealed class, so FirstOrDefault correctly returns null (not a default struct value) when no matching element is found. The is not null check is semantically identical to the old != null and aligns with project conventions.

Warning message fix

The old message said "InProgressTests is null" but the condition guarded was Count == 0. The corrected message "InProgressTests is empty" accurately reflects the actual diagnostic condition.

Description alignment ✅

The PR description precisely describes all three changes with no undescribed edits and no stale claims.

🧠 Reviewed by Expert Code Reviewer 🧠

[github-actions]

This pull request was automatically closed because it expired on 2026-06-27T15:54:22.603Z.

Closed by Workflow