Retry only failed dist tests in CI - #2509
George Adams (gdams) wants to merge 2 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The retry pattern lacks the required dist-test prefix, and Windows devscript invocation does not safely escape multi-package patterns.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This pull request narrows CI retries to failed dist-test packages or variants while preserving per-attempt JSON and JUnit artifacts.
Changes:
- Chains each attempt’s raw output into subsequent retries.
- Builds escaped package and variant filters for standard and devscript builders.
- Adds parser/output coverage and missing-failure handling.
| File | Summary |
|---|---|
eng/pipeline/stages/run-stage.yml |
Passes each attempt’s raw output to the next retry. |
eng/_util/cmd/run-builder/run-builder.go |
Adds retry filtering to both builder paths. |
eng/_util/cmd/build/build.go |
Forwards test filters to devscript tests. |
eng/_util/buildutil/testjson.go |
Parses package-level failures into retry patterns. |
eng/_util/buildutil/testjson_test.go |
Tests filtering, escaping, deduplication, and failure handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Quim Muntal (@qmuntal) any concern that we'll start to accrue real test bugs that fail when the tests are run in a specific order but don't happen again when only a subset of tests are re-run? (Considering we don't currently have any dashboard or tracking for flaky tests.) |
|
Hmm, I had the impression that this PR was retrying whole packages rather than individual tests. But looking at the test cases, I'm wrong. Better just do the former, else we will definitely be under-testing on the second attempt. |
Davis Goodin (dagood)
left a comment
There was a problem hiding this comment.
This seems too simple. I don't think it accounts for dist test, and Copilot points this out:
// Test GOEXPERIMENT=nojsonv2.
if !strings.Contains(goexperiment, "nojsonv2") {
t.registerTest("GOEXPERIMENT=nojsonv2 go test encoding/json/...", &goTest{
variant: "nojsonv2",
env: []string{"GOEXPERIMENT=" + goexperiments("nojsonv2")},
pkg: "encoding/json/...",
})
}Copilot says this PR would construct ^(encoding/json:nojsonv2)$ to attempt to retry it, but this would result in zero tests matching, causing the run to succeed even though the test wasn't run.
|
I propose we don't try this until we have automatic tracking of flaky tests, as a backstop for various types of problems that could come up. |

Summary
Validation
git diff --checkpassed.Retry granularity is a dist-test package/variant, not an individual Go test function within that package.