Revert "Guard server telemetry against duplicate result updates per test UID (#8192)"#8218
Conversation
There was a problem hiding this comment.
Pull request overview
Reverts #8192, which introduced an outcome-based deduplication guard in server-mode telemetry. That guard produced asymmetric retry counting depending on outcome shape; this revert restores the prior, symmetric behavior where every subsequent update for the same UID counts as a retry.
Changes:
- Remove the
HasPassed == hasPassedearly-return guard inAddOrUpdateTestNodeStateStatistics. - Revert the duplicate-passed test expectation back to counting as a retry, and rename the outcome-change test back to
PopulateTestNodeStatistics_WithEventsForSameUid. - Drop the two tests added by #8192 (duplicate-failed and duplicate-after-outcome-change).
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/PerRequestServerDataConsumerService.cs | Removes the dedup early-return so duplicate updates again increment retry counters. |
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/ServerMode/ServerDataConsumerServiceTests.cs | Restores prior test expectations and removes tests added with the reverted guard. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary
The revert is clean and correct. The root cause of the revert is valid: the guard if (existingStatistics.HasPassed == hasPassed) return; was asymmetric in the sense that pass+pass counted 0 retries while fail+pass+pass (after the outcome flip) would count only 1 retry — effectively swallowing the second duplicate event in the latter case but not having any equivalent deduplication for the failure case prior to the flip. Restoring symmetric behaviour (every subsequent event increments the corresponding retry counter) is the right call.
Production code — no issues. The removal of the 5-line guard is minimal and correct.
Test coverage — one minor gap: the symmetric counterpart to WithDuplicatePassedEvents (i.e., fail+fail → TotalFailedRetries=1) is now untested. The old test for that case was rightfully removed (it expected the now-wrong value of 0), but no replacement documents the new expected behaviour. See inline comment for a suggested test.
Generated by Expert Code Review (on open) for issue #8218 · ● 3.8M
|
I agree with the revert here 👍 (I don't think the PR description is correct though - there is no retry involved here IMO) |
Summary
Reverts #8192.
Why
After the merge, @Youssef1313 pointed out (#8192 (comment)) that the deduplication guard introduces an asymmetric retry-counting semantic. Under #8192's guard, the retry count became:
The "retry" count then depends on outcome shape rather than on how many results were actually reported — particularly noticeable for "folded" tests where multiple sub-cases share one
TestNode.Uid. The pre-#8192 behavior (every subsequent update for the same UID counts as a retry) is at least symmetric and consistent.This revert restores that prior behavior. If retry/dedup semantics need to change, that should be a separate, design-led PR rather than the post-hoc guard from #8192.
Validation
ServerDataConsumerServiceTestsruns green (8/8).