-
Notifications
You must be signed in to change notification settings - Fork 76
Fix @failingTest not working correctly for out-of-band exceptions #2216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 Dart-Code/Dart-Code#5761
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly fixes an issue where @failingTest
did not handle out-of-band exceptions properly, which could lead to tests hanging. The introduction of a failed
flag effectively tracks whether any exception has occurred, preventing the test from being incorrectly marked as 'passed' and then failed. The new test case test_fails_throws_outOfBand
is a great addition that specifically targets and verifies the fix.
I've added a couple of suggestions to improve the code further:
- Refactoring duplicated error-handling logic in
_runFailingTest
to improve maintainability. - Aligning the new test function's signature with Effective Dart guidelines for better type safety, as per the repository's style guide.
pkgs/test_reflective_loader/test/test_reflective_loader_test.dart
Outdated
Show resolved
Hide resolved
The problem is real, waiting for these timeouts is very inconvenient. I'm not sure it requires any fancy out-of-band code. This code: @failingTest
void test_xxx() {} causes
The final statistics is correct. Looking at your picture again... Well, maybe we do have more than one issue here. |
pkgs/test_reflective_loader/test/test_reflective_loader_test.dart
Outdated
Show resolved
Hide resolved
If you mean the second failing test (the teardown one), that was just because that test doesn't work correctly if you use However, looks like you're right there is still an issue with it taking a long time in your repro - I will have a look at that. |
I forgot to mention in my comment above - I think the reason for the timeout was this: https://api.dart.dev/dart-async/runZonedGuarded.html
So I changed it to not throw inside the callback but instead throw it outside (although I'm not sure I completely understand why other exceptions thrown by tests didn't lead to the same condition). |
@scheglov thanks! I don't have any permissions here - are you the right person to approve the bots and/or merge, or does it need someone else? |
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with |
I can land it. |
@scheglov thanks! I've updated the version and changelog now. |
Revisions updated by `dart tools/rev_sdk_deps.dart`. tools (https://github.com/dart-lang/tools/compare/d0941a3..ce12670): ce126700 Mon Oct 20 21:50:45 2025 +0100 Danny Tuppeny Fix @failingTest not working correctly for out-of-band exceptions (dart-lang/tools#2216) 8bb94d88 Mon Oct 20 16:14:33 2025 +0200 Sebastian Stallenberger [oauth2] Add isClosed and exception handling for closed state to oaut… (dart-lang/tools#2089) webdev (https://github.com/dart-lang/webdev/compare/82b3855..b9c39c0): b9c39c00 Mon Oct 20 15:52:13 2025 -0400 Ben Konyi [ DWDS ] Refactor `AppInspector` to support both Chrome and Web Socket connections (dart-lang/webdev#2700) Change-Id: I1e989df7dbfcbc99afe7dfcd0920a8fb0be5e4db Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/456260 Commit-Queue: Konstantin Shcheglov <scheglov@google.com> Auto-Submit: Danny Tuppeny <danny@tuppeny.com> Reviewed-by: Devon Carew <devoncarew@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
The
_runFailingTest
method is documented with: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 reportsTest passed
when run with it.Fixes Dart-Code/Dart-Code#5761
Before:
After: