Repository navigation
Reland: Allow resetting test invariants in addTearDown - #193476
Conversation
github-actions
Bot
added
a: tests
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors TestWidgetsFlutterBinding to defer framework-level invariant verifications and binding state cleanups to postTest, allowing them to run after any addTearDown callbacks have executed. It also introduces a new test suite to verify these invariant behaviors. The review feedback correctly identifies several robustness issues in the new tests where global state modifications and resource allocations (such as Ticker and debugDefaultTargetPlatformOverride) are not guaranteed to be cleaned up if assertions fail, and suggests using addTearDown to ensure proper teardown.
dkwingsmt
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/193476, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
Bot
removed
the
autosubmit
|
Update: It is caused by three more failures in Google's codebase and I've opened CLs to close them respectively. Edit: As of Oct 1, 2 out of 3 CLs have been submitted. cl/989931886 remains, which I'm monitoring. |
dkwingsmt
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
Flutter's recent change flutter/flutter#193476 moved the test harness's check for pending frame callbacks. It now runs after the test's `addTearDown` callbacks, instead of right after the harness's final pump. Loading a font sends a `fontsChange` message, and text in the tree reacts by scheduling a frame callback. When a `testWidgets` body loads a font, the message is handled only after the final pump, so the callback never runs. The check used to miss such callbacks, but now it finds them. As a result, 178 tests in emoji_reaction_test and katex_test failed with "An animation is still running even after the widget tree was disposed". Loading the fonts in `setUpAll` avoids this, because that runs before any test has put text in the tree. Fonts persist across tests anyway. Co-Authored-By: Claude Sonnet 5.5 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
2 participants
This PR relands #192082 after fixing the error in integration tests as described in #193383.
I've manually verified that the integration test fails on #192082 and passes now.
Why it failed
This integration test runs
dev/automated_tests/flutter_test/ticker_test.dart, which leaves aTickerrunning. It expects that test to fail with an "An animation is still running even after the widget tree was disposed." error. After #192082,ticker_test.dartpassed instead.The leaked ticker is caught by
SchedulerBinding.debugAssertNoTransientCallbacks. Unlike most invariant checks, it doesn't throw; it reports the failure throughFlutterError.reportError. During a test,FlutterError.onErrorstores reported errors in_pendingExceptionDetails, and the test completion handler reports them whenrunTestfinishes. Changing the behavior of this method would be a breaking change to the public API in the framework, so instead the test framework should be changed to pick up this error.Before #192082, the check ran inside
_runTestBody, so the completion handler picked up the error. #192082 moved the check topostTest, which runs after the completion handler. Thetry/catchinpostTestonly catches thrown errors, andpostTestthen clears_pendingExceptionDetails, so the error was silently dropped and the test passed.What this reland changes
See the 2nd commit for the list of changes.
postTestalso picks up_pendingExceptionDetailsand reports it. If a check also threw, the thrown error is reported and the other one is printed to the console so it isn't lost.postTestnow passes the test description toreportTestExceptioninstead of'', so the output includesThe test description was: ...again. The integration test's expected output relies on that line._verify*Unsethelpers go back toFlutterError.reportError, which is what they used before Allow resetting test invariants inaddTearDown#192082. Now thatpostTesthandles reported errors, they don't need to throw, and they behave the same as before. Allow resetting test invariants inaddTearDown#192082 made this change with the hope to simplify and unify the code flow, but now that we have to pick up_pendingExceptionDetailsanyway, there's no reason to make this change.Tests
bindings_invariants_test.dart. It leaks aTickerand checks thatpostTestreports the error along with the test description.test_test.dart: flutter test should report a nice error when a Ticker is left runningpasses again.What the 3rd commit changes
The 3rd commit refactors the
bindings_invariants_test.dartto ensure that thepostTestis run (and invariants are reset) even when errors occur during the test body.Prior arts
This PR replaces #186604, an earlier attempt at this issue. While reviewing the implementation I found that this problem is much more complicated and would be easier for me to rewrite it. I appreciate the original author @/mvincentong for the inspiration.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.