From 48af2c2594759aff3d574f4f0d22c85aa46eb61a Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Mon, 20 Oct 2025 18:45:18 +0100 Subject: [PATCH 1/5] Fix @failingTest not working correctly for out-of-band exceptions The `_runFailingTest` method is documented with: ``` - An exception is thrown to the zone handler from a timer task. ``` However, if the test code runs without exceptions, it would still call `test_package.fail('Test passed - expected to fail.');` (and cause the test to not complete until the timeout). This means you could have a test (such as the one added here) that fails when run without `@failingTest` but hands and reports `Test passed` when run with it. Fixes https://github.com/Dart-Code/Dart-Code/issues/5761 --- .../lib/test_reflective_loader.dart | 12 ++++++++++-- .../test/test_reflective_loader_test.dart | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart index 8ea837dee..903e57eac 100644 --- a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart +++ b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart @@ -248,17 +248,24 @@ Future _invokeSymbolIfExists( /// - An exception is thrown to the zone handler from a timer task. Future? _runFailingTest(ClassMirror classMirror, Symbol symbol) { var passed = false; + var failed = false; return runZonedGuarded(() { // ignore: void_checks return Future.sync(() => _runTest(classMirror, symbol)).then((_) { - passed = true; - test_package.fail('Test passed - expected to fail.'); + // Only consider a passed test (and therefore something we should fail) if + // this completed without another failure (such as an out-of-band + // exception) occurring during the run. + if (!failed) { + passed = true; + test_package.fail('Test passed - expected to fail.'); + } }).catchError((Object e) { // if passed, and we call fail(), rethrow this exception if (passed) { // ignore: only_throw_errors throw e; } + failed = true; // otherwise, an exception is not a failure for _runFailingTest }); }, (e, st) { @@ -267,6 +274,7 @@ Future? _runFailingTest(ClassMirror classMirror, Symbol symbol) { // ignore: only_throw_errors throw e; } + failed = true; // otherwise, an exception is not a failure for _runFailingTest }); } diff --git a/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart b/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart index ba0319305..e3f302aab 100644 --- a/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart +++ b/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart @@ -75,6 +75,20 @@ class TestReflectiveLoaderTest { return Future.error('foo'); } + @failingTest + Future test_fails_throws_outOfBand() async { + var completer = Completer(); + // 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.value() + .then((_) => throw UnimplementedError('foo')) + .whenComplete(completer.complete)); + await completer.future; + } + @failingTest void test_fails_throws_sync() { throw StateError('foo'); From add5cb13d9c5d269398a0e68dead43fb755b03d3 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Mon, 20 Oct 2025 18:52:42 +0100 Subject: [PATCH 2/5] Make return types explicit --- .../test/test_reflective_loader_test.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart b/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart index e3f302aab..ea487fa93 100644 --- a/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart +++ b/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart @@ -71,12 +71,12 @@ class TestReflectiveLoaderTest { } @failingTest - Future test_fails_throws_async() { + Future test_fails_throws_async() { return Future.error('foo'); } @failingTest - Future test_fails_throws_outOfBand() async { + Future test_fails_throws_outOfBand() async { var completer = Completer(); // This exception occurs during the test run but isn't directly awaited so // this method doesn't not itself complete with an error. From 7282c09268bd842be82f6fbee73fa124df77d04f Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Mon, 20 Oct 2025 19:48:18 +0100 Subject: [PATCH 3/5] Fix timing out of unexpected passing tests --- .../lib/test_reflective_loader.dart | 56 +++++++++++-------- .../test/test_reflective_loader_test.dart | 2 +- 2 files changed, 35 insertions(+), 23 deletions(-) diff --git a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart index 903e57eac..37cdf387f 100644 --- a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart +++ b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart @@ -246,37 +246,40 @@ Future _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? _runFailingTest(ClassMirror classMirror, Symbol symbol) { - var passed = false; - var failed = false; - return runZonedGuarded(() { +Future _runFailingTest(ClassMirror classMirror, Symbol symbol) async { + _FailedTestResult? result; + + await runZonedGuarded(() { // ignore: void_checks return Future.sync(() => _runTest(classMirror, symbol)).then((_) { - // Only consider a passed test (and therefore something we should fail) if - // this completed without another failure (such as an out-of-band - // exception) occurring during the run. - if (!failed) { - 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; - } - failed = true; - // 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) { + // if an unawaited exception occurs after we had already completed and + // passed then this is an error case and we should throw. + if (result == _FailedTestResult.pass) { // ignore: only_throw_errors throw e; + } else { + // Otherwise, since it occurred during the run it is an expected failure. + result = _FailedTestResult.expectedFail; } - failed = true; - // otherwise, an exception is not a failure for _runFailingTest }); + + // 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 _runTest(ClassMirror classMirror, Symbol symbol) async { @@ -397,6 +400,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?) { diff --git a/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart b/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart index ea487fa93..5b4d6e067 100644 --- a/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart +++ b/pkgs/test_reflective_loader/test/test_reflective_loader_test.dart @@ -76,7 +76,7 @@ class TestReflectiveLoaderTest { } @failingTest - Future test_fails_throws_outOfBand() async { + Future test_fails_throws_async_unawaitedFuture() async { var completer = Completer(); // This exception occurs during the test run but isn't directly awaited so // this method doesn't not itself complete with an error. From 588bc67d6387ee2647f537d04b579d562fcbee3a Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Mon, 20 Oct 2025 21:03:57 +0100 Subject: [PATCH 4/5] Simplify --- .../lib/test_reflective_loader.dart | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart index 37cdf387f..8c8b6abdc 100644 --- a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart +++ b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart @@ -265,15 +265,7 @@ Future _runFailingTest(ClassMirror classMirror, Symbol symbol) async { result = _FailedTestResult.expectedFail; }); }, (e, st) { - // if an unawaited exception occurs after we had already completed and - // passed then this is an error case and we should throw. - if (result == _FailedTestResult.pass) { - // ignore: only_throw_errors - throw e; - } else { - // Otherwise, since it occurred during the run it is an expected failure. - result = _FailedTestResult.expectedFail; - } + result = _FailedTestResult.expectedFail; }); // We can safely throw exceptions back outside of the error zone. From f6e5b68a9694f9ede08bfb88fecbb324c915cd4a Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Mon, 20 Oct 2025 21:40:43 +0100 Subject: [PATCH 5/5] Bump version + update changelog --- pkgs/test_reflective_loader/CHANGELOG.md | 7 +++++++ pkgs/test_reflective_loader/pubspec.yaml | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/pkgs/test_reflective_loader/CHANGELOG.md b/pkgs/test_reflective_loader/CHANGELOG.md index 27a14c6f8..682011349 100644 --- a/pkgs/test_reflective_loader/CHANGELOG.md +++ b/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 diff --git a/pkgs/test_reflective_loader/pubspec.yaml b/pkgs/test_reflective_loader/pubspec.yaml index efdec89d1..4a4a0a36a 100644 --- a/pkgs/test_reflective_loader/pubspec.yaml +++ b/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