[fix] Fix DataDrivenTest parent result being double-counted in TRX summary

[nohwnd]

Summary

🤖 This is an automated fix generated by Copilot.

Fixes #15643

Root Cause

When a data-driven test (e.g., a test with [DataRow] attributes) runs, the TRX logger receives two types of TestResultHandler events:

1. One for the parent DataDrivenTest result (a container that holds all data row results)
2. One for each inner DataDrivenDataRow result (the actual per-row execution)

Previously, all events incremented _totalTestCount, _passedTestCount, and _failedTestCount unconditionally. This caused N+1 counts (parent + N data rows) instead of the correct N counts (just the data rows).

Fix

In TestResultHandler, after CreateTestResult returns, detect when the first inner data-driven result has been added to its parent (parentTestElement.TestType == UnitTestType and parentAggregation.InnerResults.Count == 1). At that point, subtract the parent's already-counted contribution from the totals.

This mirrors how the console logger works (which does report the correct total).

Test

• Updated the existing TestResultHandlerShouldAddHierarchicalResultsIfParentTestResultIsPresent test to assert TotalTestCount == 2 (not 3) for a data-driven test with 1 parent + 2 data rows.
• Added a new test TestResultHandlerShouldCountOnlyDataRowResultsNotParentForDataDrivenTests that explicitly validates TotalTestCount, PassedTestCount, and FailedTestCount are correct for a data-driven test scenario.

All 69 TrxLogger unit tests pass.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

[Copilot]

Pull request overview

This PR fixes TRX ResultSummary totals for MSTest data-driven tests so that only the individual data row results contribute to TotalTestCount/PassedTestCount/FailedTestCount, matching console logger behavior and resolving #15643.

Changes:

• Adjust TrxLogger.TestResultHandler to “undo” the previously-counted parent data-driven result when the first inner data-row result is attached to it.
• Update an existing hierarchical-results unit test to expect data-row-only totals.
• Add a dedicated unit test validating total/passed/failed counts for a parent + 2 data-row scenario.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs	Fixes summary counting by subtracting the parent data-driven container’s contribution once the first inner data-row result is observed.
test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs	Updates expectations for hierarchical/data-driven results and adds coverage for passed/failed counting with inner data rows.

[nohwnd]

The fix correctly addresses the N+1 counting bug for data-driven tests in the TRX logger. The logic — detect the first inner data-row result and subtract the parent's already-counted contribution — mirrors how the console logger handles the same scenario, and the new tests validate the correct counts.

One threading concern is flagged inline: the InnerResults.Count == 1 sentinel can be missed if two inner results are dispatched concurrently, leaving the parent contribution uncorrected. A ConcurrentDictionary-based "already corrected" flag would make the correction exactly-once and race-free.

---

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

[Threading] The InnerResults.Count == 1 sentinel relies on the assumption that TestResultHandler is never called concurrently. If two inner data-row results arrive on different threads simultaneously, both threads call testResultAggregation.InnerResults.Add(testResult) (inside CreateTestResult, line 727) on a non-thread-safe List<ITestResult> before either checks the count. After both adds complete, Count is already 2, so neither thread sees Count == 1 and the parent's contribution is never decremented — leaving _totalTestCount inflated by 1.

The fact that the surrounding code already uses Interlocked for the counters signals that concurrent invocation is a real concern (defensive or otherwise).

Suggestion: Instead of checking Count == 1, use a per-aggregation "already corrected" flag (e.g., a ConcurrentDictionary<Guid, bool> keyed by parentExecutionId) and a TryAdd/CompareExchange to ensure the decrement fires exactly once regardless of arrival order:

if (parentTestElement != null
    && parentTestElement.TestType.Equals(TrxLoggerConstants.UnitTestType)
    && parentTestResult is TestResultAggregation
    && _parentResultsAlreadyCorrected.TryAdd(parentExecutionId, true))
{
    Interlocked.Decrement(ref _totalTestCount);
    ...
}

This is safe even under concurrent dispatch because TryAdd is atomic.

[nohwnd]

Fixed. Replaced InnerResults.Count == 1 with a ConcurrentDictionary<Guid, bool> (_parentResultsCorrected) that uses TryAdd to atomically ensure the decrement fires exactly once, regardless of arrival order under concurrent dispatch. Build succeeded (0 warnings/errors, Release config) and all TrxLogger unit tests pass.

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

The threading fix has been implemented, built (Release config, 0 warnings/errors), and all tests pass (69 TrxLogger tests + full suite).

Changes made (commit 8ad7cbf):

• Added _parentResultsCorrected field (ConcurrentDictionary<Guid, bool>?)
• Added nameof(_parentResultsCorrected) to both MemberNotNullWhen and MemberNotNull attributes on IsInitialized/Initialize
• Initialized it in Initialize() alongside the other dictionaries
• Replaced parentTestResult is TestResultAggregation parentAggregation && parentAggregation.InnerResults.Count == 1 with parentTestResult is TestResultAggregation && _parentResultsCorrected!.TryAdd(parentExecutionId, true) — atomically ensures exactly-once decrement, race-free under concurrent dispatch

⚠️ Push credentials are unavailable (GH_AW_GITHUB_TOKEN not configured as a workflow secret) — the commit is ready locally but cannot be pushed. Please configure the secret or push manually:

git push origin fix/issue-15643-3bbb86948b5f5308

🔧 Iterated by PR Iteration Agent 🔧

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Commit pushed: b09b63d

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

The threading concern from the previous review (using InnerResults.Count == 1 as a sentinel) has been addressed correctly. The updated fix uses ConcurrentDictionary<Guid, bool>.TryAdd to ensure the parent's contribution is decremented exactly once, regardless of concurrent inner-result dispatch.

All aspects of the fix look solid:

• TryAdd atomicity: Correctly ensures exactly-once decrement even under concurrent TestResultHandler calls
• Outcome snapshot: The parent's Outcome is evaluated at the time of correction — that's the outcome set when the parent result was processed, which is the right value to undo
• Guid.Empty guard: When parentExecutionId == Guid.Empty, parentTestElement is reset to null (line 294), so the correction block is never entered with a degenerate key
• Test coverage: The new test independently validates TotalTestCount, PassedTestCount, and FailedTestCount

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[nohwnd]

Closing in favor of #15766 which uses a cleaner atomic approach (AddInnerResult on TestResultAggregation) rather than a separate ConcurrentDictionary.