[fix] Fix DataDriven test results being double-counted in TRX logger totals#15766
[fix] Fix DataDriven test results being double-counted in TRX logger totals#15766nohwnd wants to merge 3 commits into
Conversation
When a DataDriven (data-row) test is run, MSTest emits a parent container result followed by an inner result per data row. TrxLogger was counting all of them, so a test with N data rows incremented TotalTestCount by N+1 instead of N. Fix: when the first inner DataDriven result is processed (identified by a UnitTest-typed parent element), undo the count that was previously recorded for the parent container result so that only the actual executions are tallied. Fixes #15643 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes incorrect TRX ResultSummary counters for MSTest DataDriven tests where the parent (container) result was being counted in addition to each data-row result, inflating TotalTestCount / PassedTestCount / FailedTestCount.
Changes:
- Adjust
TrxLogger.TestResultHandlercounters to subtract the parent container’s contribution when the first inner DataDriven result is processed. - Update an existing hierarchical-results unit test assertion to reflect the corrected total count.
- Add a new regression test covering the DataDriven parent + inner results counting scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs | Adds counter “undo” logic intended to prevent counting the DataDriven parent container as a separate test execution. |
| test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs | Updates an assertion for hierarchical DataDriven results and adds a regression test for the double-counting scenario. |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
The fix correctly undoes the over-count of a DataDriven parent container result on the first inner result arrival, and the regression test is well-structured and covers the basic scenario.
One threading concern noted inline: the InnerResults.Count == 1 check is not atomic with the Interlocked.Decrement calls, which can produce incorrect counts if two inner results for the same parent are processed concurrently. The existing use of Interlocked operations in TestResultHandler suggests concurrent invocation is expected, so this warrants a fix before merge.
🧠 Reviewed by Expert Code Reviewer
🧠 Reviewed by Expert Code Reviewer 🧠
Use Interlocked.Increment on a dedicated _innerResultCount in TestResultAggregation so the 'first inner result' check is atomic. The previous InnerResults.Count == 1 check was not thread-safe: two concurrent data-row results could both read Count == 1 before either decrement fired, causing a double-decrement. The new AddInnerResult method locks the list for thread-safe Add and uses Interlocked.Increment to get the new count atomically. The returned count is propagated via an out parameter so the caller gates the parent-count undo on the Interlocked return value == 1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
The threading concern flagged in the prior review has been correctly resolved.
AddInnerResult fix analysis:
lock (this)protects theList<ITestResult>mutation (correct —List<T>is not thread-safe)Interlocked.Incrementoutside the lock returns the new counter value atomically, guaranteeing exactly one thread receivesinnerCount == 1regardless of concurrent callsisFirstDataDrivenInnerResult = innerCount == 1is therefore race-safe ✅
Increment/decrement symmetry:
The increment logic (lines 307–316) uses testResult.Outcome at parent-processing time. The new decrement logic uses parentTestResult.Outcome. These refer to the same TRX object (stored in _results[executionId] when the parent was processed and retrieved as _results[parentExecutionId] when the inner result arrives), so the symmetry is correct ✅
No new issues introduced. The fix is well-scoped and the regression test covers the key scenario.
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
Runs DataDrivenTestProject (3 DataRow + 1 simple test) with TRX logger and verifies the Counters element reflects 4 actual executions, not 5 (which would indicate the parent container is double-counted). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 This is an automated fix generated by GitHub Copilot.
Closes #15643
Root Cause
When a DataDriven (data-row) test is executed, MSTest emits:
ExecutionId, noParentExecId)ParentExecIdpointing to the parent)TrxLogger.TestResultHandlerwas incrementingTotalTestCount(andPassedTestCount/FailedTestCount) for every result it received — including the parent container. For a test with N data rows, this meant a count of N+1 instead of the correct N.Fix
In
TestResultHandler, after creating the test result, we detect when the first inner DataDriven result is processed (parent element is aUnitTestTypeand the aggregation now has exactly one inner result). At that point we undo the count that was previously recorded for the parent container, since the parent is just a grouping construct and not an actual test execution.The fix is scoped only to
UnitTestTypeparents (i.e., DataDriven tests). Ordered tests are unaffected — their parent and each child are genuinely independent test cases.Changes
src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs— decrement parent counts when first inner DataDriven result arrivestest/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs— correct existing assertion from3→2, add new regression testTestResultHandlerShouldNotDoubleCountParentDataDrivenTestInTotalTestCountTesting
All TrxLogger unit tests pass.