Uh oh!
There was an error while loading. Please reload this page.
feat(test-runner): add --merge-strategy flag to merge-reports - #42287
feat(test-runner): add --merge-strategy flag to merge-reports#42287areebmohammed wants to merge 2 commits into
Conversation
Adds `--merge-strategy <separate|overwrite|as-retry>` to `merge-reports` so tests that appear in multiple blobs with the same id (e.g. a full run merged with a --last-failed rerun) can be reconciled instead of always showing up as duplicate entries. Default stays "separate" for backward compatibility. Fixes: microsoft#33094
areebmohammed
commented
Aug 17, 2026
@microsoft-github-policy-service agree |
| workerIndex: number; | ||
| parallelIndex: number; | ||
| startTime: number; | ||
| // Set by report merging to discard previously-merged results for this test id |
There was a problem hiding this comment.
remove the comment
| // Set by report merging to discard previously-merged results for this test id | ||
| // right before this result is added, without affecting every onTestBegin globally | ||
| // (see clearPreviousResultsWhenTestBegins, which applies to all of them). | ||
| discardPreviousResults?: boolean; |
There was a problem hiding this comment.
I don't think we need to repeat it on each event.
There was a problem hiding this comment.
Right — dropped the once-per-blob "already discarded" tracking entirely, see the MergeStrategyPatcher change below.
| const globalTestIdSet = new Set<string>(); | ||
| // Shared across all blobs so that "as-retry" keeps numbering retries | ||
| // consecutively for a given test id, instead of each blob restarting at 0. | ||
| const retryOffsets = new Map<string, number>(); |
There was a problem hiding this comment.
can we do away without the map?
There was a problem hiding this comment.
Replaced with a single shared counter map, per your suggestion below.
| if (this._mergeStrategy === 'as-retry') { | ||
| if (isCollision) | ||
| this._offsetRetry(params.testId, params.result); | ||
| this._trackRetryIndex(params.testId, params.result.retry); |
There was a problem hiding this comment.
IdsPatcher is and odd place for code like this.
There was a problem hiding this comment.
Agreed, pulled it out into a new MergeStrategyPatcher class that runs right after IdsPatcher in the chain. IdsPatcher no longer knows anything about merge strategy beyond id mapping.
| private _globalTestIdSet: Set<string>; | ||
| private _mergeStrategy: MergeStrategy; | ||
| private _retryOffsets: Map<string, number>; | ||
| // Test ids that collided with an earlier blob (populated while patching this blob's |
There was a problem hiding this comment.
We shouldn't need such long comments, let's drop. If we cannot make sense of the code without the comments, it's a good signal that the code has to be simplified (and it feels so).
There was a problem hiding this comment.
Agreed — _collidingTestIds and the comment both went away with the rest of the collision-tracking state.
| this._discardedResultsForTestIds.add(params.testId); | ||
| } | ||
| if (this._mergeStrategy === 'as-retry') { | ||
| if (isCollision) |
There was a problem hiding this comment.
For as-retry, the merged retry index is just the number of results merged so far for that test id. A single shared Map<testId, number> replaces _retryOffsets + _retryOffsetForTestId + _offsetRetry + _trackRetryIndex.
There was a problem hiding this comment.
Done - one map, incremented per merged result, in MergeStrategyPatcher
| case 'onTestBegin': { | ||
| params.testId = this._mapTestId(params.testId); | ||
| const isCollision = this._collidingTestIds.has(params.testId); | ||
| if (isCollision && this._mergeStrategy === 'overwrite' && !this._discardedResultsForTestIds.has(params.testId)) { |
There was a problem hiding this comment.
For overwrite, discardPreviousResults = (result.retry === 0) is enough — clearing an empty result list is a no-op, so there's no need to track which ids collided or which were already discarded. _collidingTestIds and _discardedResultsForTestIds both go away.
There was a problem hiding this comment.
Done — implemented as MergeStrategyPatcher's overwrite branch, and _collidingTestIds/_discardedResultsForTestIds are both removed.
Replaces IdsPatcher's per-test collision tracking (_collidingTestIds, _discardedResultsForTestIds, _retryOffsetForTestId, _retryOffsets) with a standalone MergeStrategyPatcher backed by a single shared counter map. "overwrite" now discards on result.retry === 0 (a no-op when there's no prior result to discard); "as-retry" renumbers retries by counting merged results per test id. Neither needs to know which ids collided. Addresses review feedback on PR microsoft#42287.
Test results for "MCP"1 failed 8141 passed, 1361 skipped Merge workflow run. |
Test results for "tests 1"1 failed 21 flaky51306 passed, 1235 skipped Merge workflow run. |
Summary
--merge-strategy <separate|overwrite|as-retry>tomerge-reports, so a test that appears in multiple blobs with the same id (e.g. a full run merged with a--last-failedrerun) can be reconciled instead of always showing up as a duplicate entry.separate(default) keeps existing behavior.overwritekeeps only the chronologically later result.as-retryrenumbers the later result to continue the original's retry sequence, so it renders like a normal Playwright retry.Fixes#33094