Unverified Commit 0b226944 authored by Matan Lurey's avatar Matan Lurey Committed by GitHub

Do not hang on test failures of tests within `flutter_tools` (#141821)

Fixes https://github.com/flutter/flutter/issues/141823

Before this change, when a test would fail, the terminal would hang (by default for 30s) until killed by the test runner.

Basically, [`runZonedGuarded`](https://api.flutter.dev/flutter/dart-async/runZonedGuarded.html) _does_ document (though not clearly) that a returned future should not be awaited:

```txt
The zone will always be an error-zone ([Zone.errorZone](https://api.flutter.dev/flutter/dart-async/Zone/errorZone.html)), so returning a future created inside the zone, and waiting for it outside of the zone, will risk the future not being seen to complete.
```

For example, you can see other places in Dart and Flutter that we circumvent that problem:

- https://github.com/flutter/flutter/blob/5987563e4aecb34fca446ea804943bb8d27d8fcd/packages/flutter_tools/test/general.shard/base/async_guard_test.dart#L279-L306
- https://github.com/dart-lang/dartdoc/blob/b04c9c127fea5f3fdf600aa205f50d81d1c779c5/lib/src/dartdoc.dart#L258-L264
- https://github.com/flutter/engine/blob/d1afda52d254f5f1faf79e51fe430d912d6db3ee/lib/web_ui/dev/browser_process.dart#L20-L22

I'm open to suggestions on how to test this :)

/cc @natebosch @jakemac53 @lrhn if you have any color commentary for us.
parent 0dea82e6
......@@ -125,9 +125,16 @@ void testUsingContext(
Analytics: () => NoOpAnalytics(),
},
body: () {
return runZonedGuarded<Future<dynamic>>(() {
// To catch all errors thrown by the test, even uncaught async errors, we use a zone.
//
// Zones introduce their own event loop, so we do not await futures created inside
// the zone from outside the zone. Instead, we create a Completer outside the zone,
// and have the test complete it when the test ends (in success or failure), and we
// await that.
final Completer<void> completer = Completer<void>();
runZonedGuarded<Future<dynamic>>(() async {
try {
return context.run<dynamic>(
return await context.run<dynamic>(
// Apply the overrides to the test context in the zone since their
// instantiation may reference items already stored on the context.
overrides: overrides,
......@@ -145,14 +152,22 @@ void testUsingContext(
} catch (error) { // ignore: avoid_catches_without_on_clauses
_printBufferedErrors(context);
rethrow;
} finally {
if (!completer.isCompleted) {
completer.complete();
}
}
}, (Object error, StackTrace stackTrace) {
// When things fail, it's ok to print to the console!
print(error); // ignore: avoid_print
print(stackTrace); // ignore: avoid_print
_printBufferedErrors(context);
if (!completer.isCompleted) {
completer.completeError(error, stackTrace);
}
throw error; //ignore: only_throw_errors
});
return completer.future;
},
);
}, overrides: <Type, Generator>{
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment