feat(test runner): shuffle order of tests with sharding seed #30817

muhqu · 2024-05-15T07:50:46Z

Adds a new feature to the test runner which allows to specify a seed value to randomize the test group order before sharding.

Problem

For example, if the first half of your tests are slower than the rest of your tests and you are using 4 shards to run your tests across multiple machines, it means that shard 1 and 2 will take significantly more time to complete than shard 3 and 4.

Solution

By randomizing the order of test groups before sharding in a deterministic way we can achieve a better distribution of slow and fast tests across all shards.

Example

Below are some runtime stats from a project I've been working on, which shows the potential benefit of this change.
Previously our distributed test job required around 30 minutes to complete due to individual shards requiring between 9-29 minutes. Now, with the seeded shuffling we're down to 20 minutes with individual shards requiring between 15-19 minutes.

muhqu · 2024-05-15T07:57:27Z

@microsoft-github-policy-service agree company="Adobe"

muhqu · 2024-05-15T12:55:37Z

retest

docs/src/test-api/class-fullconfig.md

docs/src/test-api/class-testconfig.md

john-griffin · 2024-05-15T14:06:04Z

This looks great, thank you!

pavelfeldman

Looks good overall!

docs/src/test-api/class-fullconfig.md

docs/src/test-api/class-testconfig.md

packages/playwright/src/common/config.ts

pavelfeldman · 2024-05-21T19:22:18Z

packages/playwright/src/common/ipc.ts

@@ -32,6 +32,7 @@ export type ConfigCLIOverrides = {
  reporter?: ReporterDescription[];
  additionalReporters?: ReporterDescription[];
  shard?: { current: number, total: number };
+  shardingSeed?: string;


revert: no need to pass it to the workers.

shardingSeed is required on ConfigCLIOverrides to support --sharding-seed cli option... It would be limiting to only support this via config file. Or am I missing something?

…OR if not via cli option, it would be great to support it via environment variable e.g.export PW_SHARDING_SEED=...

Ah, you are right, we need it here since we are setting it in the command line.

packages/playwright/src/isomorphic/teleReceiver.ts

packages/playwright/src/runner/loadUtils.ts

pavelfeldman · 2024-05-21T19:26:26Z

packages/playwright/types/test.d.ts

+   * ```
+   *
+   */
+  shardingSeed?: string;


These will disappear.

Looks like it did not. Is that a problem?

We have user-defined config TestConfig and resolved FullConfig. Should be in TestConfig and not in FullConfig. So you seem to be fine.

github-actions · 2024-05-22T08:45:28Z

Test results for "tests 1"

1 flaky

⚠️ [firefox-page] › page/page-request-continue.spec.ts:481:3 › continue should not change multipart/form-data body

27030 passed, 610 skipped
✔️✔️✔️

Merge workflow run.

muhqu · 2024-05-22T09:10:11Z

I think there is some more room for improvement regarding the sharding logic.

Currently the way sharding works something like this…

         [  1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12]
Shard 1:  ^---------^                                      : [  1, 2, 3 ]
Shard 2:              ^---------^                          : [  4, 5, 6 ]
Shard 3:                          ^---------^              : [  7, 8, 9 ]
Shard 4:                                      ^---------^  : [ 10,11,12 ]

I think it would be better to change the distribution logic to something like this…

         [  1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12]
Shard 1:    ^               ^               ^              : [  1, 5, 9 ]
Shard 2:        ^               ^               ^          : [  2, 6,10 ]
Shard 3:            ^               ^               ^      : [  3, 7,11 ]
Shard 4:                ^               ^               ^  : [  4, 8,12 ]

And when we are able to sort the test groups by duration (e.g. given the results of a previous test run), we could achieve better distribution than this shuffle logic in this PR. However, that can be explored in a different PR…

pavelfeldman · 2024-05-22T23:19:10Z

packages/playwright/src/common/ipc.ts

@@ -32,6 +32,7 @@ export type ConfigCLIOverrides = {
  reporter?: ReporterDescription[];
  additionalReporters?: ReporterDescription[];
  shard?: { current: number, total: number };
+  shardingSeed?: string;


Ah, you are right, we need it here since we are setting it in the command line.

pavelfeldman · 2024-05-22T23:21:56Z

packages/playwright/types/test.d.ts

+   * ```
+   *
+   */
+  shardingSeed?: string;


We have user-defined config TestConfig and resolved FullConfig. Should be in TestConfig and not in FullConfig. So you seem to be fine.

feat(test runner): shuffle order of tests with sharding seed

d4e236e