Repository navigation
Reland: Allow resetting test invariants in addTearDown - #193476
Conversation
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.
|
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. |
|
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. |
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]>
) This PR relands flutter#192082 after fixing the error in integration tests as described in flutter#193383. I've manually verified that the integration test fails on flutter#192082 and passes now. ### Why it failed This integration test runs `dev/automated_tests/flutter_test/ticker_test.dart`, which leaves a `Ticker` running. It expects that test to fail with an "An animation is still running even after the widget tree was disposed." error. After flutter#192082, `ticker_test.dart` passed instead. The leaked ticker is caught by `SchedulerBinding.debugAssertNoTransientCallbacks`. Unlike most invariant checks, it doesn't throw; it reports the failure through `FlutterError.reportError`. During a test, `FlutterError.onError` stores reported errors in `_pendingExceptionDetails`, and the test completion handler reports them when `runTest` finishes. 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 flutter#192082, the check ran inside `_runTestBody`, so the completion handler picked up the error. flutter#192082 moved the check to `postTest`, which runs after the completion handler. The `try/catch` in `postTest` only catches thrown errors, and `postTest` then clears `_pendingExceptionDetails`, so the error was silently dropped and the test passed. #### What this reland changes See the [2nd commit](flutter@c617498) for the list of changes. - After running the invariant checks, `postTest` also picks up `_pendingExceptionDetails` and 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. - `postTest` now passes the test description to `reportTestException` instead of `''`, so the output includes `The test description was: ...` again. The integration test's expected output relies on that line. - The four `_verify*Unset` helpers go back to `FlutterError.reportError`, which is what they used before flutter#192082. Now that `postTest` handles reported errors, they don't need to throw, and they behave the same as before. flutter#192082 made this change with the hope to simplify and unify the code flow, but now that we have to pick up `_pendingExceptionDetails` anyway, there's no reason to make this change. #### Tests - Added a test to `bindings_invariants_test.dart`. It leaks a `Ticker` and checks that `postTest` reports the error along with the test description. - `test_test.dart: flutter test should report a nice error when a Ticker is left running` passes again. ### What the 3rd commit changes The [3rd commit](flutter@6866134) refactors the `bindings_invariants_test.dart` to ensure that the `postTest` is run (and invariants are reset) even when errors occur during the test body. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant in-code documentation (doc comments with `///`). - [ ] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
* test: the tests pass on Flutter 3.50 Load the fonts in setUpAll instead of in a testWidgets body. A font loaded in the body sends its fontsChange message after the test has ended, and since Flutter 3.50 (flutter/flutter#193476) the transient callback check runs after it arrives. loadImpaktfullUiFonts now throws when it is called inside a testWidgets body. * build: the example app builds for Android on Flutter 3.50 Flutter 3.50 needs Gradle 9.3.1 or newer; the example was on 9.1.0. * feat: impaktfull_ui is built on material_ui and cupertino_ui for Flutter 3.50 Flutter 3.50 deprecates package:flutter/material.dart and package:flutter/cupertino.dart: Material and Cupertino moved into the packages material_ui and cupertino_ui. Every import of the package, its tests, the example app and the code templates of tool/cli now use them (the change of dart fix --code=migrate_design_widgets, applied as a plain import swap so no file is reformatted), and flutter analyze is clean again on Flutter 3.50. impaktfull_ui needs Flutter 3.47 or newer, the minimum of material_ui. The Dart SDK constraint stays where it was, so the language version and with it the formatting of the code do not change in this release. material_ui is a separate copy of Material, so a ThemeData passed to ImpaktfullUiApp comes from material_ui now. ImpaktfullUiApp puts MaterialUiCompatibilityBridge around its pages, so pages and packages that still use package:flutter/material.dart keep their theme and their material localizations while they migrate. The README explains the migration of an app. The golden helpers give the material_ui theme the obscured font of the alchemist theme, because alchemist 0.14 still builds its golden app with package:flutter/material.dart; without it every CI golden changed. test_fixes imports TabController from material_ui, the import dart fix now migrates. * build: the example app uses Android Gradle Plugin 9.1.1, the minimum of Flutter 3.50
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.