-
Notifications
You must be signed in to change notification settings - Fork 243
feat(test): support sharding tests across CI runners #1707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
64326bd
5e1db47
a505315
f5b39fc
26357ed
ad2aa0e
80de3c0
1c52e95
7001ea9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,35 +33,96 @@ Future<void> run(HookContext context) async { | |
| final flutterSdkRegExp = RegExp(r'sdk:\s*flutter$', multiLine: true); | ||
| final isFlutter = flutterSdkRegExp.hasMatch(pubspecContents); | ||
|
|
||
| final identifierGenerator = DartIdentifierGenerator(); | ||
| final testIdentifierTable = <Map<String, String>>[]; | ||
| final shardIndex = context.vars['shard-index'] as int?; | ||
| final totalShards = context.vars['total-shards'] as int?; | ||
|
|
||
| // The CLI validates these before it gets here, but `mason make` prompts for | ||
| // them directly, so guard the round-robin below against values that would | ||
| // never terminate or index out of range. | ||
| if (shardIndex != null && | ||
| totalShards != null && | ||
| (totalShards < 1 || shardIndex < 1 || shardIndex > totalShards)) { | ||
| context.logger.err( | ||
| 'shard-index must be between 1 and total-shards, but got ' | ||
| 'shard-index $shardIndex and total-shards $totalShards', | ||
| ); | ||
| exitFn(1); | ||
| } | ||
|
|
||
| final tests = testDir | ||
| .listSync(recursive: true) | ||
| .where((entity) => entity.isTest); | ||
|
|
||
| final notOptimizedTests = await getNotOptimizedTests(tests, testDir.path); | ||
|
|
||
| for (final entity in tests) { | ||
| final relativePath = path | ||
| .relative(entity.path, from: testDir.path) | ||
| .replaceAll(r'\', '/'); | ||
| testIdentifierTable.add({ | ||
| 'path': relativePath, | ||
| 'identifier': identifierGenerator.next(), | ||
| }); | ||
| } | ||
|
|
||
| final optimizedTestsIdentifierTable = testIdentifierTable | ||
| .where((e) => !notOptimizedTests.contains(e['path'])) | ||
| final notOptimizedTests = (await getNotOptimizedTests( | ||
| tests, | ||
| testDir.path, | ||
| )).toSet(); | ||
|
|
||
| // Sorting guarantees a deterministic order across machines, which is what | ||
| // makes sharding reproducible: `Directory.listSync` order is filesystem | ||
| // dependent, so without this two runners could disagree on the partition | ||
| // and either skip or duplicate tests. | ||
| final testPaths = | ||
| tests | ||
| .map( | ||
| (entity) => path | ||
| .relative(entity.path, from: testDir.path) | ||
| .replaceAll(r'\', '/'), | ||
| ) | ||
| .toList() | ||
| ..sort(); | ||
|
|
||
| // Non optimized tests run as standalone files alongside the optimizer | ||
| // entrypoint, so they are sharded too, and in the same deal as the | ||
| // optimized ones: dealing out one list keeps every shard within one file | ||
| // of the others, whereas dealing out the two lists separately would hand | ||
| // the first shards a file from each. | ||
| final shardPaths = _shardOf( | ||
| testPaths, | ||
| shardIndex: shardIndex, | ||
| totalShards: totalShards, | ||
| ); | ||
| final optimizedTestPaths = shardPaths | ||
| .where((p) => !notOptimizedTests.contains(p)) | ||
| .toList(); | ||
| final shardedNotOptimizedTests = shardPaths | ||
| .where(notOptimizedTests.contains) | ||
| .toList(); | ||
|
|
||
| final identifierGenerator = DartIdentifierGenerator(); | ||
| final optimizedTestsIdentifierTable = [ | ||
| for (final relativePath in optimizedTestPaths) | ||
| {'path': relativePath, 'identifier': identifierGenerator.next()}, | ||
| ]; | ||
|
|
||
| context.vars = { | ||
| 'tests': optimizedTestsIdentifierTable, | ||
| 'isFlutter': isFlutter, | ||
| 'notOptimizedTests': notOptimizedTests, | ||
| 'notOptimizedTests': shardedNotOptimizedTests, | ||
| }; | ||
| } | ||
|
|
||
| /// Returns the subset of [paths] that belongs to the shard [shardIndex] out of | ||
| /// [totalShards]. | ||
| /// | ||
| /// Returns [paths] unchanged when sharding is not enabled (either value is | ||
| /// `null`). | ||
| /// | ||
| /// Files are dealt out round-robin (index modulo [totalShards]) over the | ||
| /// already sorted [paths], which keeps shards balanced in file count and makes | ||
| /// the partition stable for a given test suite. | ||
| List<String> _shardOf( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: the loop below has no bounds checks.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| List<String> paths, { | ||
| required int? shardIndex, | ||
| required int? totalShards, | ||
| }) { | ||
| if (shardIndex == null || totalShards == null) return paths; | ||
|
|
||
| return [ | ||
| for (var i = shardIndex - 1; i < paths.length; i += totalShards) paths[i], | ||
| ]; | ||
| } | ||
|
|
||
| extension on FileSystemEntity { | ||
| bool get isTest { | ||
| return this is File && path.basename(this.path).endsWith('_test.dart'); | ||
|
|
@@ -85,9 +146,10 @@ Future<List<String>> getNotOptimizedTests( | |
| } | ||
| } | ||
|
|
||
| /// Format to relative path | ||
| /// Format to relative path, normalizing separators so the paths compare | ||
| /// equal to the ones built in [run] on Windows too. | ||
| final relativePaths = testWithVeryGoodTest | ||
| .map((e) => path.relative(e, from: testDir)) | ||
| .map((e) => path.relative(e, from: testDir).replaceAll(r'\', '/')) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this normalizes to forward slashes, but the consumer at
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| .toList(); | ||
|
|
||
| return relativePaths; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Nit:
notOptimizedTestsis aList, so eachcontainsis a full scan, run once per path here and again on L73.That is quadratic in the number of test files, on every run of the command.
Build a
Setonce before bothwherecalls.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.
Done