-
Notifications
You must be signed in to change notification settings - Fork 228
Allow --shard-by=file #2655
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: master
Are you sure you want to change the base?
Allow --shard-by=file #2655
Changes from all commits
6d4361c
123f404
f5d7f19
bf785f1
fa12902
7de7f39
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 |
|---|---|---|
|
|
@@ -84,6 +84,8 @@ $_runtimeCompilers | |
| (defaults to "$_defaultConcurrency") | ||
| --total-shards The total number of invocations of the test runner being run. | ||
| --shard-index The index of this test runner invocation (of --total-shards). | ||
| --shard-by How to distribute tests across shards. | ||
| [test (default), file] | ||
|
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. Any expectations of other granularities? Is there any reason to not always use |
||
| --timeout The default test timeout. For example: 15s, 2x, none | ||
| (defaults to "30s") | ||
| --suite-load-timeout The timeout for loading a test suite. Loading the test suite includes compiling the test suite. For example: 15s, 2m, none | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -172,6 +172,147 @@ void main() { | |
| await test.shouldExit(79); | ||
|
Contributor
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. Should add some edge cases, 0 test suites, less test suites than shards, more test suites than shards, etc.
Contributor
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. I am also curious how this interacts with the |
||
| }); | ||
|
|
||
| test('shards by file', () async { | ||
| await d.file('1_test.dart', ''' | ||
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| test("test 1.1", () {}); | ||
| test("test 1.2", () {}); | ||
| } | ||
| ''').create(); | ||
|
|
||
| await d.file('2_test.dart', ''' | ||
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| test("test 2.1", () {}); | ||
| test("test 2.2", () {}); | ||
| } | ||
| ''').create(); | ||
|
|
||
| var test = await runTest([ | ||
| '.', | ||
| '--shard-index=0', | ||
| '--total-shards=2', | ||
| '--shard-by=file', | ||
| ]); | ||
| expect( | ||
| test.stdout, | ||
| containsInOrder([ | ||
| '+0: ./1_test.dart: test 1.1', | ||
| '+1: ./1_test.dart: test 1.2', | ||
| '+2: All tests passed!', | ||
| ]), | ||
| ); | ||
| expect(test.stdout, isNot(contains('./2_test.dart'))); | ||
| await test.shouldExit(0); | ||
|
|
||
| test = await runTest([ | ||
| '.', | ||
| '--shard-index=1', | ||
| '--total-shards=2', | ||
| '--shard-by=file', | ||
| ]); | ||
| expect( | ||
| test.stdout, | ||
| containsInOrder([ | ||
| '+0: ./2_test.dart: test 2.1', | ||
| '+1: ./2_test.dart: test 2.2', | ||
| '+2: All tests passed!', | ||
| ]), | ||
| ); | ||
| expect(test.stdout, isNot(contains('./1_test.dart'))); | ||
| await test.shouldExit(0); | ||
| }); | ||
|
|
||
| test('interaction of --shard-by=file with name filters', () async { | ||
| await d.file('1_test.dart', ''' | ||
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| test("match", () {}); | ||
| } | ||
| ''').create(); | ||
|
|
||
| await d.file('2_test.dart', ''' | ||
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| test("other", () {}); | ||
| } | ||
| ''').create(); | ||
|
|
||
| // Shard 1 gets 2_test.dart. But 2_test.dart has no tests matching 'match'. | ||
| // So shard 1 should report no tests ran and fail. | ||
| var test = await runTest([ | ||
| '.', | ||
| '--shard-index=1', | ||
| '--total-shards=2', | ||
| '--shard-by=file', | ||
| '--name=match', | ||
| ]); | ||
| expect(test.stdout, emitsThrough('No tests ran.')); | ||
| await test.shouldExit(79); | ||
|
|
||
| // Shard 0 gets 1_test.dart. It matches, so it runs. | ||
| test = await runTest([ | ||
| '.', | ||
| '--shard-index=0', | ||
| '--total-shards=2', | ||
| '--shard-by=file', | ||
| '--name=match', | ||
| ]); | ||
| expect( | ||
| test.stdout, | ||
| containsInOrder(['+0: ./1_test.dart: match', '+1: All tests passed!']), | ||
| ); | ||
| await test.shouldExit(0); | ||
| }); | ||
|
|
||
| test('interaction of --shard-by=test with name filters', () async { | ||
| await d.file('test.dart', ''' | ||
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| test("match 1", () {}); | ||
| test("match 2", () {}); | ||
| test("other 1", () {}); | ||
| test("other 2", () {}); | ||
| } | ||
| ''').create(); | ||
|
|
||
| // Only tests matching 'match' are sharded. 'match 1' and 'match 2' are active. | ||
| // Shard 0 runs 'match 1', Shard 1 runs 'match 2'. | ||
| var test = await runTest([ | ||
| 'test.dart', | ||
| '--shard-index=0', | ||
| '--total-shards=2', | ||
| '--shard-by=test', | ||
| '--name=match', | ||
| ]); | ||
| expect( | ||
| test.stdout, | ||
| containsInOrder(['+0: match 1', '+1: All tests passed!']), | ||
| ); | ||
| expect(test.stdout, isNot(contains('match 2'))); | ||
| await test.shouldExit(0); | ||
|
|
||
| test = await runTest([ | ||
| 'test.dart', | ||
| '--shard-index=1', | ||
| '--total-shards=2', | ||
| '--shard-by=test', | ||
| '--name=match', | ||
| ]); | ||
| expect( | ||
| test.stdout, | ||
| containsInOrder(['+0: match 2', '+1: All tests passed!']), | ||
| ); | ||
| expect(test.stdout, isNot(contains('match 1'))); | ||
| await test.shouldExit(0); | ||
| }); | ||
|
|
||
| group('reports an error if', () { | ||
| test('--shard-index is provided alone', () async { | ||
| var test = await runTest(['--shard-index=1']); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -232,6 +232,7 @@ Configuration configuration({ | |
| int? concurrency, | ||
| int? shardIndex, | ||
| int? totalShards, | ||
| String? shardBy, | ||
|
Contributor
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. Lets make this an enum |
||
| Map<String, Set<TestSelection>>? testSelections, | ||
| Iterable<String>? foldTraceExcept, | ||
| Iterable<String>? foldTraceOnly, | ||
|
|
@@ -286,6 +287,7 @@ Configuration configuration({ | |
| concurrency: concurrency, | ||
| shardIndex: shardIndex, | ||
| totalShards: totalShards, | ||
| shardBy: shardBy, | ||
| testSelections: testSelections, | ||
| foldTraceExcept: foldTraceExcept, | ||
| foldTraceOnly: foldTraceOnly, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import 'dart:io'; | |
|
|
||
| import 'package:async/async.dart'; | ||
| import 'package:boolean_selector/boolean_selector.dart'; | ||
| import 'package:path/path.dart' as p; | ||
| import 'package:stack_trace/stack_trace.dart'; | ||
| import 'package:test_api/backend.dart' | ||
| show PlatformSelector, Runtime, SuitePlatform; | ||
|
|
@@ -269,25 +270,76 @@ class Runner { | |
| /// Only tests that match [_config.patterns] will be included in the | ||
| /// suites once they're loaded. | ||
| Stream<LoadSuite> _loadSuites() { | ||
| return StreamGroup.merge( | ||
| _config.testSelections.entries.map((pathEntry) { | ||
| final Stream<LoadSuite> source; | ||
|
Contributor
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. I would break this out into separate functions instead of inlining it all. Or instead of making |
||
| if (_config.totalShards != null && _config.shardBy == 'file') { | ||
| final allFiles = <String>[]; | ||
| for (var pathEntry in _config.testSelections.entries) { | ||
|
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. (Can use patter assignment: for (var MapEntry(key: testPath, value: ...)) ...but I'm not sure the value is actually used, so it could just iterate keys?) |
||
| final testPath = pathEntry.key; | ||
| final testSelections = pathEntry.value; | ||
| final suiteConfig = _config.suiteDefaults.selectTests(testSelections); | ||
| if (Directory(testPath).existsSync()) { | ||
| return _loader.loadDir(testPath, suiteConfig); | ||
| allFiles.addAll( | ||
| Directory(testPath) | ||
| .listSync(recursive: true) | ||
| .whereType<File>() | ||
| .map((f) => f.path) | ||
| .where((path) => _config.filename.matches(p.basename(path))), | ||
| ); | ||
| } else if (File(testPath).existsSync()) { | ||
|
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. This test does nothig, you add the |
||
| return _loader.loadFile(testPath, suiteConfig); | ||
| allFiles.add(testPath); | ||
| } else { | ||
| return Stream.fromIterable([ | ||
| LoadSuite.forLoadException( | ||
| LoadException(testPath, 'Does not exist.'), | ||
| suiteConfig, | ||
| ), | ||
| ]); | ||
| allFiles.add(testPath); | ||
| } | ||
| }), | ||
| ).map((loadSuite) { | ||
| } | ||
|
|
||
| allFiles.sort(); | ||
|
|
||
| final shardSize = allFiles.length / _config.totalShards!; | ||
| final shardStart = (shardSize * _config.shardIndex!).round(); | ||
| final shardEnd = (shardSize * (_config.shardIndex! + 1)).round(); | ||
| final shardFiles = allFiles.sublist(shardStart, shardEnd); | ||
|
|
||
| source = StreamGroup.merge( | ||
| shardFiles.map((path) { | ||
| final key = _config.testSelections.keys.firstWhere( | ||
|
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. (Can use |
||
| (k) => path == k || p.isWithin(k, path), | ||
| orElse: () => _config.testSelections.keys.first, | ||
| ); | ||
| final testSelections = _config.testSelections[key]!; | ||
| final suiteConfig = _config.suiteDefaults.selectTests(testSelections); | ||
|
|
||
| if (!File(path).existsSync() && !Directory(path).existsSync()) { | ||
| return Stream.fromIterable([ | ||
| LoadSuite.forLoadException( | ||
| LoadException(path, 'Does not exist.'), | ||
| suiteConfig, | ||
| ), | ||
| ]); | ||
| } | ||
| return _loader.loadFile(path, suiteConfig); | ||
| }), | ||
| ); | ||
| } else { | ||
| source = StreamGroup.merge( | ||
| _config.testSelections.entries.map((pathEntry) { | ||
| final testPath = pathEntry.key; | ||
| final testSelections = pathEntry.value; | ||
| final suiteConfig = _config.suiteDefaults.selectTests(testSelections); | ||
| if (Directory(testPath).existsSync()) { | ||
| return _loader.loadDir(testPath, suiteConfig); | ||
| } else if (File(testPath).existsSync()) { | ||
| return _loader.loadFile(testPath, suiteConfig); | ||
| } else { | ||
| return Stream.fromIterable([ | ||
| LoadSuite.forLoadException( | ||
| LoadException(testPath, 'Does not exist.'), | ||
| suiteConfig, | ||
| ), | ||
| ]); | ||
| } | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| return source.map((loadSuite) { | ||
| return loadSuite.changeSuite((suite) { | ||
| _warnForUnknownTags(suite); | ||
|
|
||
|
|
@@ -482,15 +534,15 @@ class Runner { | |
| return 'the suite itself'; | ||
| } | ||
|
|
||
| /// If sharding is enabled, filters [suite] to only include the tests that | ||
| /// should be run in this shard. | ||
| /// If sharding by "test" is enabled, filters [suite] to only include the | ||
| /// tests that should be run in this shard. | ||
| /// | ||
| /// We just take a slice of the tests in each suite corresponding to the shard | ||
| /// index. This makes the tests pretty tests across shards, and since the | ||
| /// index. This makes the tests pretty even across shards, and since the | ||
| /// tests are continuous, makes us more likely to be able to re-use | ||
| /// `setUpAll()` logic. | ||
| RunnerSuite _shardSuite(RunnerSuite suite) { | ||
| if (_config.totalShards == null) return suite; | ||
| if (_config.totalShards == null || _config.shardBy == 'file') return suite; | ||
|
|
||
| var shardSize = suite.group.testCount / _config.totalShards!; | ||
| var shardIndex = _config.shardIndex!; | ||
|
|
||
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.
Why not shard after filtering?
I'm assuming there is a reason, but it would be nice to know what it is.
I guess each shard will individually do the same "find all tests" computation and sharding, but if it can do that normally, it should be able to do it sharding by file as well. As long as all the shards agree on the test-set, they should agree after filtering too.
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.
Some filters are runtime only, but we should be able to respect the top level
@TestOnannotation filters without loading the test.