Guard server telemetry against duplicate result updates per test UID#8192
Conversation
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Server-mode telemetry treated every result update as a distinct event, so duplicate final-outcome notifications for the same test UID inflated retry counters. This PR adds a short-circuit in AddOrUpdateTestNodeStateStatistics so only genuine outcome transitions (e.g. Failed → Passed) are counted as retries, and updates/extends the unit tests to cover duplicate Passed, duplicate Failed, and "transition then duplicate" scenarios.
Changes:
- Skip statistics update when the new outcome matches the existing
HasPassedvalue. - Adjust existing duplicate-passed test expectation (
TotalPassedRetries1 → 0) and rename the same-UID test to clarify it now exercises an outcome change. - Add new tests for duplicate failed events and for a duplicate event following an outcome change.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/PerRequestServerDataConsumerService.cs | Early-return when the incoming outcome equals the stored one to avoid double-counting retries. |
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/ServerMode/ServerDataConsumerServiceTests.cs | Updates expectations for duplicate-passed test, renames the outcome-change test, and adds duplicate-failed and transition-then-duplicate tests. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
| return; | ||
| } | ||
|
|
||
| if (existingStatistics.HasPassed == hasPassed) |
There was a problem hiding this comment.
I don't see why this is correct. Old code makes more sense to me.
If we receive two results, even if it's for the same test node, we should count it as two results.
It also doesn't make sense to gate the double counting under HasPassed == hasPassed.
If I have a folded test with two cases, imagine these two scenarios:
- First case passed, second case failed -> we count it twice.
- The two cases passed -> we count it once.
This doesn't make sense to me. I feel like the original issue is simply not correct and should just have been closed without making a change.
Bug Fix
What was the bug?
Server-mode telemetry assumed result updates were effectively unique per test UID. If the same final outcome notification was emitted more than once, retries could be overcounted and request telemetry would report inflated result totals.
How did you fix it?
Result deduplication
Failed -> Passed).Telemetry semantics preserved
Passed/Failedtotals still reflect the latest outcome per UID.Focused test coverage
Example of the core change:
This makes duplicate
Passed -> PassedorFailed -> Failedupdates no-ops while preserving retry counting for genuine state changes.Testing