Don't ignore outstanding work on failure (#1815)
Fixes #1512
The test invoker tracks the work done by the test, both the callback
bodies and the other work registered with the framework, and in the
typical case waits for it to complete before the test is considered
done.
Previously, any error would immediately ignore all outstanding work. In
the case of an unhandled async error, the test may still be using
resources that will be cleaned up by the tear down.
Some tests that previously would have failed quickly will now result in
a timeout. This should be uncommon, and will only impact tests that
fail anyway.
- Stop ignoring all outstanding work on any error.
- Explicitly ignore outstanding work for a timeout.
- Fix some cases that run tests that do fail and then timeout. These
tests had been manually incrementing the outstanding callback count
and relying on the known failure to prevent a timeout. Decrement the
outstanding callback count after the errors have been delivered.
- More reliably decrement the outstanding work counter in a few
situations where an exception from a test callback could cause it to
be missed so it had to be cleaned up by the error handler.
- Use a try/finally around callbacks which introduce their own
outstanding work tracking zones.
- Use a try/catch/rethrow around the body for load suites (the
platform work including the user `main` method). Using a try/finally
can cause the runner to be held open waiting for `main` to complete
after a signal should have stopped tests.diff --git a/pkgs/test/CHANGELOG.md b/pkgs/test/CHANGELOG.md
index 3aa280c..60fb868 100644
--- a/pkgs/test/CHANGELOG.md
+++ b/pkgs/test/CHANGELOG.md
@@ -1,3 +1,8 @@
+## 1.22.2-dev
+
+* Don't run `tearDown` until the test body and outstanding work is complete,
+ even if the test has already failed.
+
## 1.22.1
* Add documentation for the `--ignore-timeouts` argument.
diff --git a/pkgs/test/pubspec.yaml b/pkgs/test/pubspec.yaml
index c1798b8..df2e49f 100644
--- a/pkgs/test/pubspec.yaml
+++ b/pkgs/test/pubspec.yaml
@@ -1,5 +1,5 @@
name: test
-version: 1.22.1
+version: 1.22.2-dev
description: >-
A full featured library for writing and running Dart tests across platforms.
repository: https://github.com/dart-lang/test/tree/master/pkgs/test
@@ -32,8 +32,8 @@
webkit_inspection_protocol: ^1.0.0
yaml: ^3.0.0
# Use an exact version until the test_api and test_core package are stable.
- test_api: 0.4.17
- test_core: 0.4.21
+ test_api: 0.4.18
+ test_core: 0.4.22
dev_dependencies:
fake_async: ^1.0.0
diff --git a/pkgs/test_api/CHANGELOG.md b/pkgs/test_api/CHANGELOG.md
index 458b0fc..d1ffa88 100644
--- a/pkgs/test_api/CHANGELOG.md
+++ b/pkgs/test_api/CHANGELOG.md
@@ -1,3 +1,8 @@
+## 0.4.18-dev
+
+* Don't run `tearDown` until the test body and outstanding work is complete,
+ even if the test has already failed.
+
## 0.4.17
* Deprecate `throwsNullThrownError`, use `throwsA(isA<TypeError>())` instead.
diff --git a/pkgs/test_api/lib/src/backend/invoker.dart b/pkgs/test_api/lib/src/backend/invoker.dart
index 54a439c..582d9f9 100644
--- a/pkgs/test_api/lib/src/backend/invoker.dart
+++ b/pkgs/test_api/lib/src/backend/invoker.dart
@@ -254,8 +254,11 @@
runZoned(() async {
zone = Zone.current;
_outstandingCallbackZones.add(zone!);
- await fn();
- counter.decrement();
+ try {
+ await fn();
+ } finally {
+ counter.decrement();
+ }
}, zoneValues: {_counterKey: counter});
return counter.onZero.whenComplete(() {
@@ -286,6 +289,7 @@
_timeoutTimer = Zone.root.createTimer(timeout, () {
_outstandingCallbackZones.last.run(() {
_handleError(Zone.current, TimeoutException(message(), timeout));
+ _outstandingCallbacks.complete();
});
});
}
@@ -347,7 +351,6 @@
}
_controller.addError(error, stackTrace!);
- zone.run(() => _outstandingCallbacks.complete());
if (_printsOnFailure.isNotEmpty) {
_print(_printsOnFailure.join('\n\n'));
diff --git a/pkgs/test_api/pubspec.yaml b/pkgs/test_api/pubspec.yaml
index 764fa6a..5c47c5b 100644
--- a/pkgs/test_api/pubspec.yaml
+++ b/pkgs/test_api/pubspec.yaml
@@ -1,5 +1,5 @@
name: test_api
-version: 0.4.17
+version: 0.4.18-dev
description: >-
The user facing API for structuring Dart tests and checking expectations.
repository: https://github.com/dart-lang/test/tree/master/pkgs/test_api
diff --git a/pkgs/test_api/test/backend/declarer_test.dart b/pkgs/test_api/test/backend/declarer_test.dart
index e8bf0a9..0fe848a 100644
--- a/pkgs/test_api/test/backend/declarer_test.dart
+++ b/pkgs/test_api/test/backend/declarer_test.dart
@@ -170,7 +170,8 @@
'description 1',
expectAsync0(() {
Invoker.current!.addOutstandingCallback();
- Future(() => throw TestFailure('oh no'));
+ Future(() => throw TestFailure('oh no'))
+ .whenComplete(Invoker.current!.removeOutstandingCallback);
}, max: 1));
});
@@ -217,6 +218,27 @@
expect(outstandingCallbackRemovedBeforeTeardown, isTrue);
});
+ test("isn't run until test body completes after out-of-band error",
+ () async {
+ var hasTestFinished = false;
+ var hasTestFinishedBeforeTeardown = false;
+ var tests = declare(() {
+ tearDown(() {
+ hasTestFinishedBeforeTeardown = hasTestFinished;
+ });
+
+ test('description', () {
+ Future.error('oh no');
+ return pumpEventQueue().then((_) {
+ hasTestFinished = true;
+ });
+ });
+ });
+
+ await _runTest(tests.single as Test, shouldFail: true);
+ expect(hasTestFinishedBeforeTeardown, isTrue);
+ });
+
test("doesn't complete until there are no outstanding callbacks", () async {
var outstandingCallbackRemoved = false;
var tests = declare(() {
diff --git a/pkgs/test_api/test/backend/invoker_test.dart b/pkgs/test_api/test/backend/invoker_test.dart
index 6aa4774..490bb02 100644
--- a/pkgs/test_api/test/backend/invoker_test.dart
+++ b/pkgs/test_api/test/backend/invoker_test.dart
@@ -141,7 +141,8 @@
() {
var liveTest = _localTest(() {
Invoker.current!.addOutstandingCallback();
- Future(() => registerException(TestFailure('oh no')));
+ Future(() => registerException(TestFailure('oh no')))
+ .whenComplete(Invoker.current!.removeOutstandingCallback);
}).load(suite);
expectSingleFailure(liveTest);
@@ -152,7 +153,8 @@
() {
var liveTest = _localTest(() {
Invoker.current!.addOutstandingCallback();
- Future(() => throw TestFailure('oh no'));
+ Future(() => throw TestFailure('oh no'))
+ .whenComplete(Invoker.current!.removeOutstandingCallback);
}).load(suite);
expectSingleFailure(liveTest);
@@ -198,7 +200,8 @@
Future(() => throw TestFailure('one'));
Future(() => throw TestFailure('two'));
Future(() => throw TestFailure('three'));
- Future(() => throw TestFailure('four'));
+ Future(() => throw TestFailure('four'))
+ .whenComplete(Invoker.current!.removeOutstandingCallback);
}).load(suite);
expectStates(liveTest, [
@@ -273,7 +276,8 @@
() {
var liveTest = _localTest(() {
Invoker.current!.addOutstandingCallback();
- Future(() => registerException('oh no'));
+ Future(() => registerException('oh no'))
+ .whenComplete(Invoker.current!.removeOutstandingCallback);
}).load(suite);
expectSingleError(liveTest);
@@ -284,7 +288,8 @@
() {
var liveTest = _localTest(() {
Invoker.current!.addOutstandingCallback();
- Future(() => throw 'oh no');
+ Future(() => throw 'oh no')
+ .whenComplete(Invoker.current!.removeOutstandingCallback);
}).load(suite);
expectSingleError(liveTest);
@@ -327,7 +332,8 @@
Future(() => throw 'one');
Future(() => throw 'two');
Future(() => throw 'three');
- Future(() => throw 'four');
+ Future(() => throw 'four')
+ .whenComplete(Invoker.current!.removeOutstandingCallback);
}).load(suite);
expectStates(liveTest, [
diff --git a/pkgs/test_core/CHANGELOG.md b/pkgs/test_core/CHANGELOG.md
index 2c654d2..eaad8cf 100644
--- a/pkgs/test_core/CHANGELOG.md
+++ b/pkgs/test_core/CHANGELOG.md
@@ -1,3 +1,8 @@
+# 0.4.22-dev
+
+* Don't run `tearDown` until the test body and outstanding work is complete,
+ even if the test has already failed.
+
# 0.4.21
* Move `includeTags` and `excludeTags` from `SuiteConfiguration` to
diff --git a/pkgs/test_core/lib/src/runner/load_suite.dart b/pkgs/test_core/lib/src/runner/load_suite.dart
index 66f0e7f..2c64fc0 100644
--- a/pkgs/test_core/lib/src/runner/load_suite.dart
+++ b/pkgs/test_core/lib/src/runner/load_suite.dart
@@ -94,7 +94,13 @@
invoker!.addOutstandingCallback();
unawaited(() async {
- var suite = await body();
+ RunnerSuite? suite;
+ try {
+ suite = await body();
+ } catch (_) {
+ invoker.removeOutstandingCallback();
+ rethrow;
+ }
if (completer.isCompleted) {
// If the load test has already been closed, close the suite it
// generated.
diff --git a/pkgs/test_core/pubspec.yaml b/pkgs/test_core/pubspec.yaml
index 8efd87b..f7407f1 100644
--- a/pkgs/test_core/pubspec.yaml
+++ b/pkgs/test_core/pubspec.yaml
@@ -1,5 +1,5 @@
name: test_core
-version: 0.4.21
+version: 0.4.22-dev
description: A basic library for writing tests and running them on the VM.
repository: https://github.com/dart-lang/test/tree/master/pkgs/test_core
@@ -30,7 +30,7 @@
# matcher is tightly constrained by test_api
matcher: any
# Use an exact version until the test_api package is stable.
- test_api: 0.4.17
+ test_api: 0.4.18
dev_dependencies:
lints: '>=1.0.0 <3.0.0'
