Improve heuristic for picking a test run for purposes of measuring time - #85678
jasonmalinowski wants to merge 1 commit into
Conversation
|
Azure Pipelines: Successfully started running 2 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
🟢 Approval recommended
No unresolved review comments remain.
Review effort: Lite
Findings: None
What changed in this PR
Improves Azure DevOps test-run selection for more accurate timing estimates.
Changes:
- Selects the matching run with the highest
TotalTests. - Uses run ID as a deterministic tie-breaker.
- Updates related documentation.
| File | Summary |
|---|---|
src/Tools/RunTests/AzdoClient.cs |
Updates test-run selection logic and documentation. |
If some tests fail, the last run will only be a retry of tests that failed, so using that as a basis for test timing won't be very accurate.
7bb1153 to
53d1210
Compare
| return runs.LastOrDefault(r => string.Equals(r.Name, testRunName, StringComparison.OrdinalIgnoreCase)); | ||
| // Retries may contain only previously failing work items. Prefer the broadest timing history, | ||
| // even if some tests failed, and use the greatest run ID to break ties deterministically. | ||
| return runs |
There was a problem hiding this comment.
should we filter out tests that failed? For example a hang or something may impact the test run times that we wouldn't want to count
There was a problem hiding this comment.
I'd say no -- if a test takes time to fail, and we're trying to estimate how long it'll take to run, then we need to include failed tests too.
There was a problem hiding this comment.
Would it be better to return all runs matching testRunName and construct the timing history from the union of their successful results?
The largest run gives us broad coverage, but it may contain tests that failed, while later retry runs may contain successful results for those tests. When merging, we should process runs newest-first, include only Outcome == "Passed", and deduplicate by cleaned test method name so a result from an earlier attempt does not overwrite or double-count a newer successful result. In particular, aggregating within each run before merging would avoid counting SubResultsCount multiple times when the same theory appears in both the original run and a retry.
- var testRun = await GetTestRunAsync(...);
+ var testRuns = await GetTestRunsAsync(...);
- foreach (var testResult in testResults)
+ foreach (var testRun in testRuns.OrderByDescending(r => r.Id))
{
- ...
+ var resultsForRun = await GetResultsForRunAsync(testRun);
+ var successfulResultsForRun = BuildPerRunMap(
+ resultsForRun.Where(r =>
+ string.Equals(r.Outcome, "Passed", StringComparison.OrdinalIgnoreCase)));
+
+ // Runs are newest-first, so retry results win. TryAdd prevents the
+ // same test from being counted again from an earlier attempt.
+ foreach (var (testName, timing) in successfulResultsForRun)
+ {
+ testInfos.TryAdd(testName, timing);
+ }
}Opened #85679 based on this suggestion
|
Looks like @JoeRobich's approach in #85679 isn't too hard actually, so let's just do that. |
If some tests fail, the last run will only be a retry of tests that failed, so using that as a basis for test timing won't be very accurate.