feat(test runner): improve sharding algorithm to better spread similar tests among shards #30962
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| /** | ||
| * Copyright (c) Microsoft Corporation. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import type { FullResult, TestCase, TestResult } from '../../types/testReporter'; | ||
| import type { LastRunInfo } from '../runner/runner'; | ||
| import { BaseReporter, resolveOutputFile } from './base'; | ||
|
|
||
| type LastRunOptions = { | ||
| outputFile?: string, | ||
| configDir: string, | ||
| _mode: 'list' | 'test' | 'merge', | ||
| }; | ||
|
|
||
| // The blob reporter adds a suffix to test ids to avoid collisions. But we need | ||
| // to remove that suffix to match the test ids from the last run. | ||
| function unsaltTestId(testId: string): string { | ||
| return testId.split('-').map(s => s.substring(0, 20)).join('-'); | ||
| } | ||
|
|
||
| class LastRunReporter extends BaseReporter { | ||
|
|
||
| private lastRun: LastRunInfo = { | ||
| failedTests: [], | ||
| status: 'passed', | ||
| testDurations: {}, | ||
| }; | ||
|
|
||
| private resolvedOutputFile: string | undefined; | ||
| private testId: (testId: string) => string; | ||
|
|
||
| constructor(options: LastRunOptions) { | ||
| super(); | ||
| this.resolvedOutputFile = resolveOutputFile('LASTRUN', { fileName: '.last-run.json', ...options })?.outputFile; | ||
| this.testId = options._mode === 'merge' ? unsaltTestId : (testId: string) => testId; | ||
| } | ||
|
|
||
| override printsToStdio() { | ||
| return !this.resolvedOutputFile; | ||
| } | ||
|
|
||
| override onTestEnd(test: TestCase, result: TestResult): void { | ||
| super.onTestEnd(test, result); | ||
| this.lastRun.testDurations![this.testId(test.id)] = result.duration; | ||
| if (result.status === 'failed') | ||
| this.lastRun.failedTests.push(this.testId(test.id)); | ||
| } | ||
|
|
||
| override async onEnd(result: FullResult) { | ||
| await super.onEnd(result); | ||
| this.lastRun.status = result.status; | ||
| await this.outputReport(this.lastRun, this.resolvedOutputFile); | ||
| } | ||
|
|
||
| async outputReport(lastRun: LastRunInfo, resolvedOutputFile: string | undefined) { | ||
| const reportString = JSON.stringify(lastRun, undefined, 2); | ||
| if (resolvedOutputFile) { | ||
| await fs.promises.mkdir(path.dirname(resolvedOutputFile), { recursive: true }); | ||
| await fs.promises.writeFile(resolvedOutputFile, reportString); | ||
| } else { | ||
| console.log(reportString); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export default LastRunReporter; | ||
|
|
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -151,6 +151,7 @@ export class Runner { | |||||
| export type LastRunInfo = { | ||||||
| status: FullResult['status']; | ||||||
| failedTests: string[]; | ||||||
| testDurations?: { [testId: string]: number }; | ||||||
|
There was a problem hiding this comment. We are a bit worried that this will force export type LastRunInfo = {
status: FullResult['status'],
projects: {
name: string,
files: {
path: string,
tests: {
title: string[],
duration: number,
status: TestStatus,
}[],
}[],
}[],
};Here There was a problem hiding this comment. Yes, more verbose is also better for future use-cases that will consume the last-run info. Having the titles and paths in there is more user friendly… 👍 However, removing the already existing There was a problem hiding this comment.
There are no backwards compatibility concerns as of today, since we do not expect That said, you can leave it as is, and we'll iterate on it. There was a problem hiding this comment. I just tried to write the LastRunInfo in the proposed format, but its not as straight forward as I thought it would be.
You mean, I don't need to proceed here to get this merged? 🙏 There was a problem hiding this comment.
In our CI setup, I'm using pipeline steps to restore the last-run.json from the last successful main build. So, in our case last-run info from previous playwright versions will be used, and it would be great if there is some level of backward compatibility... at least it should not fail when the format is incorrect. |
||||||
| }; | ||||||
|
|
||||||
| async function writeLastRunInfo(testRun: TestRun, status: FullResult['status']) { | ||||||
|
|
@@ -161,7 +162,12 @@ async function writeLastRunInfo(testRun: TestRun, status: FullResult['status']) | |||||
| await fs.promises.mkdir(outputDir, { recursive: true }); | ||||||
| const lastRunReportFile = path.join(outputDir, '.last-run.json'); | ||||||
| const failedTests = testRun.rootSuite?.allTests().filter(t => !t.ok()).map(t => t.id); | ||||||
| const testDurations = testRun.rootSuite?.allTests().reduce((map, t) => { | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| if (t.results.length) | ||||||
| map[t.id] = t.results.reduce((a, b) => a + b.duration, 0); | ||||||
| return map; | ||||||
| }, {} as { [testId: string]: number }); | ||||||
|
Comment on lines
+167
to
+171
There was a problem hiding this comment. I'm actually not sure it is the right way to sum all the durations… maybe it makes more sense to calc the average? Or only include durations from successful test runs… 🤔 There was a problem hiding this comment. I think this is ok for now, we'll iterate based on feedback. |
||||||
| const lastRunReport = JSON.stringify({ status, failedTests, testDurations }, undefined, 2); | ||||||
|
There was a problem hiding this comment. Since we are still writing the file here, why do we need a separate There was a problem hiding this comment. The Maybe it would be better to implicitly write the lastrun info when merging blob reports? There was a problem hiding this comment. I think so. From the user's perspective, lastrun just working out of the box would be easier. Whether it is a reporter internally or not is an implementation detail. There was a problem hiding this comment. How about always using the last run reporter? Then we would only need to have a single implementation…. There was a problem hiding this comment. That definitely works. We'll have to add it here when |
||||||
| await fs.promises.writeFile(lastRunReportFile, lastRunReport); | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1425,6 +1425,19 @@ interface TestConfig<TestArgs = {}, WorkerArgs = {}> { | |
| total: number; | ||
| }; | ||
|
|
||
| /** | ||
| * Defines the algorithm to be used for sharding. Defaults to `'partition'`. | ||
| * - `'partition'` - divide the set of test groups by number of shards. e.g. first half goes to shard 1/2 and | ||
| * seconds half to shard 2/2. | ||
| * - `'round-robin'` - spread test groups to shards in a round-robin way. e.g. loop over test groups and always | ||
| * assign to the shard that has the lowest number of tests. | ||
| * - `'duration-round-robin'` - use duration info from `.last-run.json` to spread test groups to shards in a | ||
| * round-robin way. e.g. loop over test groups and always assign to the shard that has the lowest duration of | ||
| * tests. new tests which were not present in the last run will use an average duration time. When no | ||
| * `.last-run.json` could be found the behavior is identical to `'round-robin'`. | ||
| */ | ||
| shardingMode?: "partition"|"round-robin"|"duration-round-robin"; | ||
|
|
||
| /** | ||
| * Shuffle the order of test groups with a seed. By default tests are run in the order they are discovered, which is | ||
| * mostly alphabetical. This could lead to an uneven distribution of slow and fast tests. Shuffling the order of tests | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is not a really useful strategy, because it is not stable. Whenever I add a test or move it between files, all my shards will be completely new. This introduces a lot of variability on CI, which from our experience is something to avoid. I wonder whether we can make this stable somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my current project
round-robinyields much better distribution thanpartition. Here is why, our tests are organised something like this (simplified)…Our framework tests are pretty exhaustive and take the most time. The logged-in tests require more time than logged-out tests due to additional bootstrap and teardown. By using the
partitionalgorithm the first shards require significantly more time as they have all the long-tests from either framework or logged-in suites. Withround-robinwe're already achieving a much better balanced overall runtime, even though the algorithm doesn't know anything about per test duration.I don't feel like the round-robin algorithm is "not stable". It's deterministic, given the same set of tests it always yields the same result. Also I think in CI there should be no assumption that one test goes after another, unless they are explicitly configured to execute sequentially within the same test group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's fair. If you would like to opt-in into
round-robinand it works for you, that's great. I still think it is less stable than I would prefer, but it doesn't hurt to have it as an option anyway.