Shared loading and running pool (#1161)
Change the behavior of the concurrency setting so that loading and running
don't have separate pools.
* The loading and running of a test are now done with the same resource, and
the concurrency setting uniformly affects each. With `-j1` only a single
test will ever be loaded at a time.
* Previously the loading pool was 2x larger than the actual concurrency
setting which could cause flaky tests due to tests being loaded while
other tests were running, even with `-j1`.diff --git a/pkgs/test/test/runner/browser/compact_reporter_test.dart b/pkgs/test/test/runner/browser/compact_reporter_test.dart
index 513229d..4a65f92 100644
--- a/pkgs/test/test/runner/browser/compact_reporter_test.dart
+++ b/pkgs/test/test/runner/browser/compact_reporter_test.dart
@@ -26,7 +26,7 @@
['-p', 'chrome', '-p', 'vm', '-j', '1', 'test.dart'],
reporter: 'compact');
- expect(test.stdout, containsInOrder(['[VM]', '[Chrome]']));
+ expect(test.stdout, containsInOrder(['[Chrome]', '[VM]']));
await test.shouldExit(0);
}, tags: 'chrome');
}
diff --git a/pkgs/test/test/runner/configuration/platform_test.dart b/pkgs/test/test/runner/configuration/platform_test.dart
index 79a9fe0..d9bcede 100644
--- a/pkgs/test/test/runner/configuration/platform_test.dart
+++ b/pkgs/test/test/runner/configuration/platform_test.dart
@@ -69,8 +69,8 @@
expect(
test.stdout,
containsInOrder([
- '-1: [VM] test [E]',
- '-2: [Chrome] test [E]',
+ '-1: [Chrome] test [E]',
+ '-2: [VM] test [E]',
'-2: Some tests failed.'
]));
await test.shouldExit(1);
diff --git a/pkgs/test/test/runner/engine_test.dart b/pkgs/test/test/runner/engine_test.dart
index 08c3b00..0546228 100644
--- a/pkgs/test/test/runner/engine_test.dart
+++ b/pkgs/test/test/runner/engine_test.dart
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
import 'dart:async';
+import 'dart:math';
import 'package:pedantic/pedantic.dart';
import 'package:test/test.dart';
@@ -275,4 +276,58 @@
return engine.run();
});
});
+
+ group('concurrency', () {
+ test('is shared between runner and load suites', () async {
+ for (var concurrency = 1; concurrency < 5; concurrency++) {
+ var testsLoaded = 0;
+ var maxLoadConcurrency = 0;
+ var testsRunning = 0;
+ var maxTestConcurrency = 0;
+ var testCount = concurrency * 2;
+
+ Future<void> updateAndCheckConcurrency(
+ {bool isLoadSuite = false}) async {
+ if (isLoadSuite) {
+ testsLoaded++;
+ maxLoadConcurrency = max(maxLoadConcurrency, testsLoaded);
+ expect(testsLoaded, lessThanOrEqualTo(concurrency));
+ } else {
+ testsRunning++;
+ maxTestConcurrency = max(maxTestConcurrency, testsRunning);
+ expect(testsRunning, lessThanOrEqualTo(concurrency));
+ }
+ // Simulate the test/loading taking some amount of time so that
+ // we actually reach max concurrency.
+ await Future.delayed(Duration(milliseconds: 100));
+ if (!isLoadSuite) {
+ testsRunning--;
+ testsLoaded--;
+ }
+ }
+
+ var tests = declare(() {
+ for (var i = 0; i < testCount; i++) {
+ test('test ${i + 1}', () async {
+ await updateAndCheckConcurrency();
+ });
+ }
+ });
+ var engine = Engine.withSuites([
+ for (var i = 0; i < testCount; i++)
+ loadSuite('group $i', () async {
+ await updateAndCheckConcurrency(isLoadSuite: true);
+ return runnerSuite(Group.root([tests[i]]));
+ }),
+ ], concurrency: concurrency);
+
+ await engine.run();
+ expect(engine.liveTests.length, testCount);
+
+ // We should reach but not exceed max concurrency
+ expect(maxTestConcurrency, concurrency);
+ expect(maxLoadConcurrency, concurrency);
+ }
+ });
+ });
}
diff --git a/pkgs/test/test/utils.dart b/pkgs/test/test/utils.dart
index b2a288e..09b608d 100644
--- a/pkgs/test/test/utils.dart
+++ b/pkgs/test/test/utils.dart
@@ -19,6 +19,7 @@
import 'package:test_core/src/runner/suite.dart';
import 'package:test_core/src/runner/engine.dart';
import 'package:test_core/src/runner/plugin/environment.dart';
+import 'package:test_core/src/runner/load_suite.dart';
import 'package:test_core/src/runner/runner_suite.dart';
import 'package:test/test.dart';
@@ -201,3 +202,7 @@
/// Returns a [RunnerSuite] with a default environment and configuration.
RunnerSuite runnerSuite(Group root) => RunnerSuite(
const PluginEnvironment(), SuiteConfiguration.empty, root, suitePlatform);
+
+/// Returns a [LoadSuite] with a default configuration.
+LoadSuite loadSuite(String name, FutureOr<RunnerSuite> Function() body) =>
+ LoadSuite(name, SuiteConfiguration.empty, suitePlatform, body);
diff --git a/pkgs/test_core/CHANGELOG.md b/pkgs/test_core/CHANGELOG.md
index b36455a..bfc510c 100644
--- a/pkgs/test_core/CHANGELOG.md
+++ b/pkgs/test_core/CHANGELOG.md
@@ -9,6 +9,14 @@
* Support retrying of entire test suites when they fail to load.
* Fix the `compiling` message in precompiled mode so it says `loading` instead,
which is more accurate.
+* Change the behavior of the concurrency setting so that loading and running
+ don't have separate pools.
+ * The loading and running of a test are now done with the same resource, and
+ the concurrency setting uniformly affects each. With `-j1` only a single
+ test will ever be loaded at a time.
+ * Previously the loading pool was 2x larger than the actual concurrency
+ setting which could cause flaky tests due to tests being loaded while
+ other tests were running, even with `-j1`.
## 0.2.18
diff --git a/pkgs/test_core/lib/src/runner/engine.dart b/pkgs/test_core/lib/src/runner/engine.dart
index d944e50..c532c11 100644
--- a/pkgs/test_core/lib/src/runner/engine.dart
+++ b/pkgs/test_core/lib/src/runner/engine.dart
@@ -73,12 +73,6 @@
/// A pool that limits the number of test suites running concurrently.
final Pool _runPool;
- /// A pool that limits the number of test suites loaded concurrently.
- ///
- /// Once this reaches its limit, loading any additional test suites will cause
- /// previous suites to be unloaded in the order they completed.
- final Pool _loadPool;
-
/// A completer that will complete when [this] is unpaused.
///
/// If [this] isn't paused, [_pauseCompleter] is `null`.
@@ -96,8 +90,7 @@
/// This will be `null` if [close] was called before all the tests finished
/// running.
Future<bool> get success async {
- await Future.wait(<Future>[_group.future, _loadPool.done],
- eagerError: true);
+ await Future.wait(<Future>[_group.future, _runPool.done], eagerError: true);
if (_closedBeforeDone) return null;
return liveTests.every((liveTest) =>
liveTest.state.result.isPassing &&
@@ -215,12 +208,10 @@
// dart:io is unavailable.
/// Creates an [Engine] that will run all tests provided via [suiteSink].
///
- /// [concurrency] controls how many suites are run at once, and defaults to 1.
- /// [maxSuites] controls how many suites are *loaded* at once, and defaults to
- /// four times [concurrency].
- Engine({int concurrency, int maxSuites, String coverage})
+ /// [concurrency] controls how many suites are loaded and ran at once, and
+ /// defaults to 1.
+ Engine({int concurrency, String coverage})
: _runPool = Pool(concurrency ?? 1),
- _loadPool = Pool(maxSuites ?? (concurrency ?? 1) * 2),
_coverage = coverage {
_group.future.then((_) {
_onTestStartedGroup.close();
@@ -265,35 +256,32 @@
_onSuiteAddedController.add(suite);
_group.add(() async {
- var loadResource = await _loadPool.request();
-
+ var resource = await _runPool.request();
LiveSuiteController controller;
- if (suite is LoadSuite) {
- await _onUnpaused;
- controller = await _addLoadSuite(suite);
- if (controller == null) {
- loadResource.release();
- return;
+ try {
+ if (suite is LoadSuite) {
+ await _onUnpaused;
+ controller = await _addLoadSuite(suite);
+ if (controller == null) return;
+ } else {
+ controller = LiveSuiteController(suite);
}
- } else {
- controller = LiveSuiteController(suite);
- }
- _addLiveSuite(controller.liveSuite);
+ _addLiveSuite(controller.liveSuite);
- await _runPool.withResource(() async {
if (_closed) return;
await _runGroup(controller, controller.liveSuite.suite.group, []);
controller.noMoreLiveTests();
if (_coverage != null) await writeCoverage(_coverage, controller);
- loadResource.allowRelease(() => controller.close());
- });
+ } finally {
+ resource.allowRelease(() => controller?.close());
+ }
}());
}, onDone: () {
_subscriptions.remove(subscription);
_onSuiteAddedController.close();
_group.close();
- _loadPool.close();
+ _runPool.close();
});
_subscriptions.add(subscription);
@@ -558,11 +546,11 @@
var allLiveTests = liveTests.toSet()..addAll(_activeLoadTests);
var futures = allLiveTests.map((liveTest) => liveTest.close()).toList();
- // Closing the load pool will close the test suites as soon as their tests
+ // Closing the run pool will close the test suites as soon as their tests
// are done. For browser suites this is effectively immediate since their
// tests shut down as soon as they're closed, but for VM suites we may need
// to wait for tearDowns or tearDownAlls to run.
- futures.add(_loadPool.close());
+ futures.add(_runPool.close());
await Future.wait(futures, eagerError: true);
}
}
