[fix] Fix DataDriven test results double-counted in TRX ResultSummary

[Jakub Jareš (nohwnd)]

🤖 This PR was created automatically by the Issue Triage agent.

Fixes #15643

Root Cause

When a [DataRow]-driven test runs, vstest fires one result event per DataRow (inner results) plus one aggregate event for the parent test method. The TRX logger was incrementing its pass/fail/total counters for every event, including the parent aggregate, so a test with N data rows was counted N+1 times instead of N.

The parent result's ResultType is only set retroactively to "DataDrivenTest" when the first inner result arrives (inside CreateTestResult). By that time the parent's counters had already been incremented, leaving no clean way to skip counting it at arrival time.

Fix

After CreateTestResult returns, check whether the newly processed result is an inner DataDriven result (ResultType == InnerDataDrivenResultType) and is the first such result for its parent (InnerResults.Count == 1). If so, undo the parent's counter contribution (total, passed, or failed). Subsequent inner results are counted normally, so the final totals equal exactly the number of DataRow executions.

No change is made to Ordered test counting — that path does not set InnerDataDrivenResultType.

Tests

• Updated existing test TestResultHandlerShouldAddHierarchicalResultsIfParentTestResultIsPresent — it previously asserted TotalTestCount == 3 (parent + 2 inner), now correctly expects 2.
• Added new regression test TestResultHandlerShouldNotDoubleCountDataDrivenTestResults that verifies total, passed, and failed counts are correct for a 2-DataRow test (1 passed, 1 failed).

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

[Copilot]

Pull request overview

This PR updates the TRX logger’s result counting so data-driven parent aggregate results are not included in ResultSummary totals, aligning TRX counts with the executed DataRow results.

Changes:

• Adjusts TRX result counters when the first inner data-driven result marks its parent as a data-driven aggregate.
• Updates/adds unit coverage for data-driven result total counting.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs	Decrements the previously counted parent aggregate when processing the first inner data-driven result.
test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs	Updates existing expectations and adds a regression test for data-driven TRX summary counts.

[Jakub Jareš (nohwnd)]

🧠 Reviewed by Expert Code Reviewer

The fix is algorithmically correct and well-tested. Here's the analysis:

Fix Correctness

The undo-on-first-inner approach is sound:

1. Parent arrives → counters increment based on parent's outcome
2. First inner arrives → CreateTestResult sets inner.ResultType = InnerDataDrivenResultType and adds to parent.InnerResults (Count=1) → undo parent's contribution
3. Subsequent inners (Count > 1) → counted normally, no undo

Key invariant preserved: dataDrivenParent.Outcome at decrement time equals what was incremented when the parent was counted, because CreateTestResult only mutates ResultType, not Outcome.

Ordered Tests

Correctly unaffected — InnerDataDrivenResultType is only set on the UnitTestType path in CreateTestResult, never on the OrderedTestType path.

Thread Safety Note (pre-existing, not introduced here)

InnerResults is a plain List<ITestResult>. The new Count == 1 guard relies on sequential event delivery. If TestResultHandler were invoked concurrently, Count could be non-deterministic w.r.t. a concurrent Add. This is a pre-existing design assumption (the Interlocked calls on counters appear defensive), and the PR does not make it worse.

Tests

• Updated TestResultHandlerShouldAddHierarchicalResultsIfParentTestResultIsPresent: new expected count of 2 (not 3) is correct.
• New TestResultHandlerShouldNotDoubleCountDataDrivenTestResults: covers total/passed/failed with 2 DataRows (1 pass, 1 fail) — good regression coverage.

PR Description

Accurate and complete. No undescribed changes.

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

👋 This PR was created as a draft by the triage agent. It needs to be manually marked as Ready for Review — the safeoutputs toolset doesn't provide an undraft capability.

Please click "Ready for review" on this PR to trigger CI and move it forward.

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

Closing in favor of #15766 which has thread-safe first-inner detection and a test that exercises the pass/fail decrement path (parent Outcome = Passed).

[Jakub Jareš (nohwnd)]

🧠 Reviewed by Expert Code Reviewer

The fix is algorithmically correct. Here's the full dimension analysis for Microsoft.TestPlatform.Extensions.TrxLogger:

Algorithmic Correctness ✅

The undo-on-first-inner approach is sound. Walking through the invariants:

1. Parent arrives → testResult.Outcome (parent) → _totalTestCount++ and _failedTestCount++ / _passedTestCount++ based on outcome
2. First inner arrives → CreateTestResult sets inner.ResultType = InnerDataDrivenResultType, sets parent.ResultType = ParentDataDrivenResultType, and adds to parent.InnerResults (Count becomes 1). Critically, CreateTestResult does not mutate parentTestResult.Outcome — only ResultType. So the undo reads exactly the outcome that was counted.
3. Undo fires (Count == 1) → decrements _totalTestCount unconditionally, then decrements _failedTestCount or _passedTestCount matching the parent outcome that was originally incremented. ✓
4. Subsequent inners (Count > 1) → counted normally, no undo. ✓

TestResultOutcome (run-level ratchet) is intentionally not undone — it is a one-way fail-latch for the overall run outcome. The inner results re-establish the failure state, so the run-level outcome remains correct. ✓

Parallel Execution & Scheduling Safety ⚠️ (pre-existing, not introduced here)

dataDrivenParent.InnerResults is a List<ITestResult> accessed without locking. The Count == 1 guard relies on sequential event delivery. The Interlocked calls on counters are individually atomic but the read-increment-decrement sequence across two events is not atomic. This is a pre-existing design assumption that the PR does not worsen.

Ordered Tests — Correctly Unaffected ✅

InnerDataDrivenResultType is only assigned on the UnitTestType path in CreateTestResult (line 729). The OrderedTestType path (lines 714–719) never sets this, so ordered test counting is unaffected.

Tests ✅

• Updated TestResultHandlerShouldAddHierarchicalResultsIfParentTestResultIsPresent: new expected count of 2 (not 3) is correct.
• New TestResultHandlerShouldNotDoubleCountDataDrivenTestResults: covers parent + 2 DataRows (1 pass, 1 fail), asserts total=2, passed=1, failed=1. Good regression coverage.

PR Description Alignment ✅

Description accurately describes root cause, fix mechanism, and test changes. No undescribed changes in the diff.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠