Fix @failingTest not working correctly for out-of-band exceptions (#2216)

Author: DanTup

pkgs/test_reflective_loader/CHANGELOG.md

@@ -1,3 +1,10 @@
+## 0.5.0
+
+- Unawaited asynchronous exceptions in tests marked with `@FailingTest()` are
+  now handled correctly and will allow the test to pass.
+- Tests marked with `@FailingTest()` but passing no longer run until the
+  timeout.
+
 ## 0.4.0
 
 - Add support for one-time set up and teardown in test classes via static

pkgs/test_reflective_loader/lib/test_reflective_loader.dart

@@ -246,29 +246,32 @@ Future<Object?> _invokeSymbolIfExists(
 /// - The test fails by throwing an exception
 /// - The test returns a future which completes with an error.
 /// - An exception is thrown to the zone handler from a timer task.
-Future<Object?>? _runFailingTest(ClassMirror classMirror, Symbol symbol) {
-  var passed = false;
-  return runZonedGuarded(() {
+Future<void> _runFailingTest(ClassMirror classMirror, Symbol symbol) async {
+  _FailedTestResult? result;
+
+  await runZonedGuarded(() {
     // ignore: void_checks
     return Future.sync(() => _runTest(classMirror, symbol)).then<void>((_) {
-      passed = true;
-      test_package.fail('Test passed - expected to fail.');
+      // We can't throw async exceptions inside here because `runZoneGuarded`
+      // will never complete (see docs on `runZonedGuarded`), so we need to
+      // capture this state and throw later if there wasn't otherwise an
+      // exception.
+
+      // If we didn't already have a failure (eg. an unawaited exception) then
+      // this successful completion is an unexpected pass state.
+      result ??= _FailedTestResult.pass;
     }).catchError((Object e) {
-      // if passed, and we call fail(), rethrow this exception
-      if (passed) {
-        // ignore: only_throw_errors
-        throw e;
-      }
-      // otherwise, an exception is not a failure for _runFailingTest
+      // an awaited exception is always expected failure.
+      result = _FailedTestResult.expectedFail;
     });
   }, (e, st) {
-    // if passed, and we call fail(), rethrow this exception
-    if (passed) {
-      // ignore: only_throw_errors
-      throw e;
-    }
-    // otherwise, an exception is not a failure for _runFailingTest
+    result = _FailedTestResult.expectedFail;
   });
+
+  // We can safely throw exceptions back outside of the error zone.
+  if (result == _FailedTestResult.pass) {
+    throw test_package.TestFailure('Test passed - expected to fail.');
+  }
 }
 
 Future<void> _runTest(ClassMirror classMirror, Symbol symbol) async {
@@ -389,6 +392,15 @@ class _Test {
         timeout = null;
 }
 
+/// The result of a test that was expected to fail.
+enum _FailedTestResult {
+  /// The test (unexpectedly) passed.
+  pass,
+
+  /// The test failed as expected.
+  expectedFail,
+}
+
 extension on DeclarationMirror {
   test_package.TestLocation? get testLocation {
     if (location case var location?) {

pkgs/test_reflective_loader/pubspec.yaml

@@ -1,5 +1,5 @@
 name: test_reflective_loader
-version: 0.4.0
+version: 0.5.0
 description: Support for discovering tests and test suites using reflection.
 repository: https://github.com/dart-lang/tools/tree/main/pkgs/test_reflective_loader
 issue_tracker: https://github.com/dart-lang/tools/labels/package%3Atest_reflective_loader

pkgs/test_reflective_loader/test/test_reflective_loader_test.dart

@@ -71,10 +71,24 @@ class TestReflectiveLoaderTest {
   }
 
   @failingTest
-  Future test_fails_throws_async() {
+  Future<void> test_fails_throws_async() {
     return Future.error('foo');
   }
 
+  @failingTest
+  Future<void> test_fails_throws_async_unawaitedFuture() async {
+    var completer = Completer<void>();
+    // This exception occurs during the test run but isn't directly awaited so
+    // this method doesn't not itself complete with an error.
+    //
+    // This is a legit test failure and will be marked as such without
+    // the `@failingTest` annotation.
+    unawaited(Future<void>.value()
+        .then((_) => throw UnimplementedError('foo'))
+        .whenComplete(completer.complete));
+    await completer.future;
+  }
+
   @failingTest
   void test_fails_throws_sync() {
     throw StateError('foo');