Even more awaits v2#184552
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds missing await keywords to several asynchronous method calls within the Flutter framework and its tests, including dispose, handleRawKeyMessage, and route handling methods. The reviewer suggests using await instead of unawaited for reassembleApplication calls in test files to ensure the reassembly process is fully completed before the test continues, which would also permit the removal of the dart:async imports.
| ); | ||
| debugDisableShadows = false; | ||
| tester.binding.reassembleApplication(); | ||
| unawaited(tester.binding.reassembleApplication()); |
There was a problem hiding this comment.
Consider using await here instead of unawaited to ensure the reassemble process completes before the test continues. This is consistent with the intent of this PR and improves test reliability.
await tester.binding.reassembleApplication();References
- The Flutter style guide emphasizes optimizing for readability. Using
awaitin tests is generally more readable and predictable thanunawaitedfollowed bypumpwhen dealing with futures that have side effects. (link)
There was a problem hiding this comment.
Using await causes test failures.
| ); | ||
| debugDisableShadows = false; | ||
| tester.binding.reassembleApplication(); | ||
| unawaited(tester.binding.reassembleApplication()); |
There was a problem hiding this comment.
Consider using await here instead of unawaited to ensure the reassemble process completes before the test continues.
await tester.binding.reassembleApplication();References
- The Flutter style guide emphasizes optimizing for readability. Using
awaitin tests is generally more readable and predictable thanunawaitedfollowed bypumpwhen dealing with futures that have side effects. (link)
There was a problem hiding this comment.
Using await causes test failures.
| // We need the runTest to setup the fake async in the test binding. | ||
| await binding.runTest(() async { | ||
| binding.reassembleApplication(); | ||
| unawaited(binding.reassembleApplication()); |
There was a problem hiding this comment.
Consider using await here instead of unawaited. Awaiting reassembleApplication ensures that all asynchronous work triggered by the reassemble is finished before calling pump.
await binding.reassembleApplication();References
- The Flutter style guide emphasizes optimizing for readability. Using
awaitin tests is generally more readable and predictable thanunawaitedfollowed bypumpwhen dealing with futures that have side effects. (link)
| final ControllerT controller = await createNewViewController(id); | ||
| if (!mounted) { | ||
| controller.dispose(); | ||
| await controller.dispose(); |
There was a problem hiding this comment.
This might not be desirable because _createNewUiKitView's returned future is not used. I suggest process its error within _createNewUiKitView. Maybe wrap the full body within a big try and report error at the end. (Then maybe unawaited it?)
| await tester.pumpWidget(const TestWidgetsApp(home: Text('Hello World'))); | ||
| await tester.pump(); | ||
| tester.binding.reassembleApplication(); | ||
| unawaited(tester.binding.reassembleApplication()); |
There was a problem hiding this comment.
It seems that all usages of reassembleApplication must be unawaited in unit tests. If you're confident that this is the correct pattern, I think we should add a new method to the tester binding that performs unawaited(tester.binding.reassembleApplication()) and document its usage.
There was a problem hiding this comment.
In some cases using unawaited causes test failures so I ignored instead. Should I just mark this function as awaitNotRequired?
There was a problem hiding this comment.
I am confused too. It seems to me that unawaited is equivalent to awaitNotRequired. Do you have an idea?
There was a problem hiding this comment.
Actually the test failures weren't caused by the unawaited but by importing dart:async. I don't understand why.
It seems to me that unawaited is equivalent to awaitNotRequired. Do you have an idea?
I think if all the callsites of a function should be unawaited it's best to just apply the annotation. But for reassembleApplication there are some callsites where it is awaited?
| ); | ||
| debugDisableShadows = false; | ||
| tester.binding.reassembleApplication(); | ||
| unawaited(tester.binding.reassembleApplication()); |
There was a problem hiding this comment.
Using await causes test failures.
| ); | ||
| debugDisableShadows = false; | ||
| tester.binding.reassembleApplication(); | ||
| unawaited(tester.binding.reassembleApplication()); |
There was a problem hiding this comment.
Using await causes test failures.
| final TestWidgetsFlutterBinding binding = TestWidgetsFlutterBinding.ensureInitialized(); | ||
| // We need the runTest to setup the fake async in the test binding. | ||
| await binding.runTest(() async { | ||
| // ignore: unawaited_futures |
There was a problem hiding this comment.
unawaited here causes failures so I replaced with ignore.
|
auto label is removed for flutter/flutter/184552, Failed to enqueue flutter/flutter/184552 with HTTP 400: GraphQL mutate failed. |
|
auto label is removed for flutter/flutter/184552, Failed to enqueue flutter/flutter/184552 with HTTP 400: GraphQL mutate failed. |
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
|
auto label is removed for flutter/flutter/184552, Failed to enqueue flutter/flutter/184552 with HTTP 400: GraphQL mutate failed. |
|
Sorry about the opaque error message here. I pulled from our logs and here is the error: The next version of cocoon will have these messages inline in the failure message that autosubmit posts when it fails to enqueue the PR. |
flutter/flutter@05e0ae0...81c87ea 2026-04-09 katelovett@google.com Remove last material dependency from cupertino tests (flutter/flutter#184781) 2026-04-09 engine-flutter-autoroll@skia.org Roll Skia from 7c46cb639dba to 4d0f5389e131 (7 revisions) (flutter/flutter#184812) 2026-04-09 goderbauer@google.com Make `windowing_test` follow repo analyzer rules (flutter/flutter#184752) 2026-04-09 116356835+AbdeMohlbi@users.noreply.github.com Improve documentation of `frictionFactor` function (flutter/flutter#184509) 2026-04-09 engine-flutter-autoroll@skia.org Roll Skia from d2b0bd12576a to 7c46cb639dba (1 revision) (flutter/flutter#184796) 2026-04-09 engine-flutter-autoroll@skia.org Roll Fuchsia GN SDK from JLBh4Z9PKsjIJcqDU... to SEfYx3xgueX3aFAY3... (flutter/flutter#184797) 2026-04-09 katelovett@google.com Fixed freeze flow (flutter/flutter#184793) 2026-04-09 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#184795) 2026-04-09 engine-flutter-autoroll@skia.org Roll Skia from e9ed4fc9f154 to d2b0bd12576a (36 revisions) (flutter/flutter#184791) 2026-04-08 43054281+camsim99@users.noreply.github.com [Android] Allow sensitive content to gracefully fail when unregistering host before registering (flutter/flutter#184789) 2026-04-08 34465683+rkishan516@users.noreply.github.com Refactor: remove material from autocomplete_test, scrollable_restoration_test, semantics_tester_generate_test_semantics_expression_for_current_semantics_tree_test (flutter/flutter#184615) 2026-04-08 jmccandless@google.com Warn about the use of TestSemantics (flutter/flutter#184369) 2026-04-08 katelovett@google.com Change freeze flow to pull_request_target (flutter/flutter#184785) 2026-04-08 1063596+reidbaker@users.noreply.github.com Update to the beta dart version for 3.44 branch cut. (flutter/flutter#184770) 2026-04-08 737941+loic-sharma@users.noreply.github.com [Dot shorthands] Migrate examples/api/test (flutter/flutter#183966) 2026-04-08 rmacnak@google.com [fuchsia] Give AOT runners the ability to copy FFI callback thunks. (flutter/flutter#184696) 2026-04-08 victorsanniay@gmail.com Add await or ignore lint to invokeMethod callsites (flutter/flutter#182870) 2026-04-08 robert.ancell@canonical.com Correctly handle failure to read /proc/self/exe link (flutter/flutter#184700) 2026-04-08 engine-flutter-autoroll@skia.org Roll Skia from e264d870a380 to e9ed4fc9f154 (11 revisions) (flutter/flutter#184713) 2026-04-08 engine-flutter-autoroll@skia.org Roll Packages from 5299279 to 0e0a032 (5 revisions) (flutter/flutter#184720) 2026-04-08 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#184772) 2026-04-08 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1rcChbOv4nSTVkUxs... to pDXMXRIjEHTw7B0sk... (flutter/flutter#184722) 2026-04-08 73785960+xfce0@users.noreply.github.com Remove navigator_utils cross-imports from cupertino tests (flutter/flutter#184282) 2026-04-08 victorsanniay@gmail.com Even more awaits v2 (flutter/flutter#184552) 2026-04-08 15619084+vashworth@users.noreply.github.com Allow personal skills to be gitignored (flutter/flutter#184727) 2026-04-08 dacoharkes@google.com [ci] mac_arm64 build_test re-enable shard 1 presubmit (flutter/flutter#184751) 2026-04-08 katelovett@google.com Fix repo check on code freeze (flutter/flutter#184771) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Part of flutter#181513 --------- Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com> Co-authored-by: Kate Lovett <katelovett@google.com>
…r#11477) flutter/flutter@05e0ae0...81c87ea 2026-04-09 katelovett@google.com Remove last material dependency from cupertino tests (flutter/flutter#184781) 2026-04-09 engine-flutter-autoroll@skia.org Roll Skia from 7c46cb639dba to 4d0f5389e131 (7 revisions) (flutter/flutter#184812) 2026-04-09 goderbauer@google.com Make `windowing_test` follow repo analyzer rules (flutter/flutter#184752) 2026-04-09 116356835+AbdeMohlbi@users.noreply.github.com Improve documentation of `frictionFactor` function (flutter/flutter#184509) 2026-04-09 engine-flutter-autoroll@skia.org Roll Skia from d2b0bd12576a to 7c46cb639dba (1 revision) (flutter/flutter#184796) 2026-04-09 engine-flutter-autoroll@skia.org Roll Fuchsia GN SDK from JLBh4Z9PKsjIJcqDU... to SEfYx3xgueX3aFAY3... (flutter/flutter#184797) 2026-04-09 katelovett@google.com Fixed freeze flow (flutter/flutter#184793) 2026-04-09 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#184795) 2026-04-09 engine-flutter-autoroll@skia.org Roll Skia from e9ed4fc9f154 to d2b0bd12576a (36 revisions) (flutter/flutter#184791) 2026-04-08 43054281+camsim99@users.noreply.github.com [Android] Allow sensitive content to gracefully fail when unregistering host before registering (flutter/flutter#184789) 2026-04-08 34465683+rkishan516@users.noreply.github.com Refactor: remove material from autocomplete_test, scrollable_restoration_test, semantics_tester_generate_test_semantics_expression_for_current_semantics_tree_test (flutter/flutter#184615) 2026-04-08 jmccandless@google.com Warn about the use of TestSemantics (flutter/flutter#184369) 2026-04-08 katelovett@google.com Change freeze flow to pull_request_target (flutter/flutter#184785) 2026-04-08 1063596+reidbaker@users.noreply.github.com Update to the beta dart version for 3.44 branch cut. (flutter/flutter#184770) 2026-04-08 737941+loic-sharma@users.noreply.github.com [Dot shorthands] Migrate examples/api/test (flutter/flutter#183966) 2026-04-08 rmacnak@google.com [fuchsia] Give AOT runners the ability to copy FFI callback thunks. (flutter/flutter#184696) 2026-04-08 victorsanniay@gmail.com Add await or ignore lint to invokeMethod callsites (flutter/flutter#182870) 2026-04-08 robert.ancell@canonical.com Correctly handle failure to read /proc/self/exe link (flutter/flutter#184700) 2026-04-08 engine-flutter-autoroll@skia.org Roll Skia from e264d870a380 to e9ed4fc9f154 (11 revisions) (flutter/flutter#184713) 2026-04-08 engine-flutter-autoroll@skia.org Roll Packages from 5299279 to 0e0a032 (5 revisions) (flutter/flutter#184720) 2026-04-08 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#184772) 2026-04-08 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1rcChbOv4nSTVkUxs... to pDXMXRIjEHTw7B0sk... (flutter/flutter#184722) 2026-04-08 73785960+xfce0@users.noreply.github.com Remove navigator_utils cross-imports from cupertino tests (flutter/flutter#184282) 2026-04-08 victorsanniay@gmail.com Even more awaits v2 (flutter/flutter#184552) 2026-04-08 15619084+vashworth@users.noreply.github.com Allow personal skills to be gitignored (flutter/flutter#184727) 2026-04-08 dacoharkes@google.com [ci] mac_arm64 build_test re-enable shard 1 presubmit (flutter/flutter#184751) 2026-04-08 katelovett@google.com Fix repo check on code freeze (flutter/flutter#184771) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Part of #181513