[google_maps_flutter_android] Fix for testToggleInfoWindow persistently flaky #4768

mossmana · 2023-08-24T19:43:05Z

Pre-launch Checklist

flutter-dashboard · 2023-08-24T19:43:07Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

mossmana · 2023-08-24T20:58:37Z

I have a request out to @Hixie to get a test exemption for this PR.

Hixie · 2023-08-24T21:28:09Z

...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart

+    // https://github.com/flutter/flutter/issues/131783. It may be related
+    // to https://github.com/flutter/flutter/issues/54758 and should be
+    // re-evaluated when that issue is fixed.
+    await Future<void>.delayed(const Duration(seconds: 1));


this violates our style guide: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#never-check-if-a-port-is-available-before-using-it-never-add-timeouts-and-other-race-conditions

@Hixie I entirely agree, which is why I posted the following comment flutter/flutter#54758 (comment). It is unfortunately an existing pattern in this particular test and I would like to fix it the proper way, but I believe there's additional prerequisite work to be done first.

Discussion in the issue suggests that onCameraIdle may work as a hacky version of the actual "finished loading" callback; have you tried using that here, waiting on a future that completes the first time we get that callback?

I tried the suggestion and onCameraIdle is never called.

@stuartmorgan I documented what I tried in https://docs.google.com/document/d/1BPQ4OdlNkNEq5xUt8cp72F4qrZrOVTja4vIxqIvq3X4/edit?usp=sharing. Any other suggestions?

Hixie · 2023-08-24T21:28:36Z

test-exempt: is a test

gmackall · 2023-08-25T15:41:11Z

This probably got a comment from the bot because the test ends in ...tests.dart instead of ...test.dart.
https://github.com/flutter/cocoon/blob/58676a63bd29448f7cf2154dfe764c529f2e1cb3/app_dart/lib/src/request_handlers/github/webhook_subscription.dart#L432

stuartmorgan · 2023-08-25T16:55:39Z

Good find, we should rename this file. I just checked the repo, and it looks like it's the only one that made this mistake, so we can just add renaming it to this PR.

mossmana · 2023-08-28T20:29:21Z

Good find, we should rename this file. I just checked the repo, and it looks like it's the only one that made this mistake, so we can just add renaming it to this PR.

I'll try my best, but not 100% certain I can preserve the git history of this file (since it's a delete/add). Would that be a problem?

stuartmorgan · 2023-08-28T20:35:36Z

Good find, we should rename this file. I just checked the repo, and it looks like it's the only one that made this mistake, so we can just add renaming it to this PR.

I'll try my best, but not 100% certain I can preserve the git history of this file (since it's a delete/add). Would that be a problem?

If it doesn't preserve it that's fine, but I would be extremely surprised if it doesn't detect this as a rename given the small scope of the changes relative to the overall file. (git doesn't care how you actually do the move; move detection is heuristic in git.)

mossmana · 2023-08-29T17:37:06Z

Good find, we should rename this file. I just checked the repo, and it looks like it's the only one that made this mistake, so we can just add renaming it to this PR.

Tests fail with /var/folders/n1/6kw2lmfs0sg3k6lt7sv7g_6h010tld/T/flutter_tools.nN5iYa/flutter_test_listener.nAX29q/listener.dart:22:21: Error: Undefined name 'main'. await Future(test.main);. So, it looks like a _test file requires a main. In this particular case, I think google_maps_tests was imported into legacy_renderer_test and latest_renderer_test tests and is not able to be run standalone.

stuartmorgan · 2023-08-29T17:48:48Z

In this particular case, I think google_maps_tests was imported into legacy_renderer_test and latest_renderer_test tests and is not able to be run standalone.

Oops, I missed that. Sorry for the waste of time there :(

So the bot was right to flag it (since it wouldn't automatically be run), and doing a manual exception was the right solution after all.

stuartmorgan

LGTM with one comment.

The delay is definitely not ideal, but this seems better than the status quo of having no coverage, and we have a TODO for improving it once the necessary feature exists to do so.

stuartmorgan · 2023-09-07T19:43:13Z

...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart

-            tester,
-            () => controller.isMarkerInfoWindowShown(marker.markerId),
-            (bool visible) => visible) ??
-        false;


I don't think we should remove this; the delay you added above is for the map to load, before telling the map to show the marker window; if the comment on this code is true that would still be a potential source of flake, so keeping the wait-for-match is safer (and should be harmless if not).

…pdates

…rsistently flaky (flutter/packages#4768)

flutter/packages@ef0c65e...e04ba88 2023-09-12 [email protected] [tool] Add a package inclusion filter (flutter/packages#4904) 2023-09-12 [email protected] [flutter_markdown] Fix changelog regarding minimum supported SDK version (flutter/packages#4851) 2023-09-12 [email protected] [ios_platform_images] Add integration tests (flutter/packages#4899) 2023-09-12 [email protected] [image_picker] Copy exif tags in categories II and III (flutter/packages#4738) 2023-09-11 [email protected] [google_maps_flutter_android] Fix for testToggleInfoWindow persistently flaky (flutter/packages#4768) 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-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

* main: (83 commits) go_router_builder: support the latest pkg:analyzer (flutter#4921) Replace collection type lints with more general lint (flutter#4912) Roll Flutter from 219efce to 4e7a07a (30 revisions) (flutter#4910) [camerax] Implement `startVideoCapturing` and `onVideoRecordedEvent` (flutter#4815) [tool] Add a package inclusion filter (flutter#4904) [flutter_markdown] Fix changelog regarding minimum supported SDK version (flutter#4851) [ios_platform_images] Add integration tests (flutter#4899) [image_picker] Copy exif tags in categories II and III (flutter#4738) [google_maps_flutter_android] Fix for testToggleInfoWindow persistently flaky (flutter#4768) Roll Flutter from 7c28e8e to 219efce (16 revisions) (flutter#4901) [url_launcher] migrating iOS tests from objc to swift (flutter#4758) Roll Flutter from da676f7 to 7c28e8e (20 revisions) (flutter#4879) Bump actions/upload-artifact from 3.1.2 to 3.1.3 (flutter#4863) Roll Flutter from aea4552 to da676f7 (28 revisions) (flutter#4874) [webview_flutter_android] Added the functionality to fullscreen html5 video (flutter#3879) [tool] Add Android dependency (gradle) option to update dependencies command (flutter#4757) [camerax] Implement resolution configuration (flutter#3799) Manual roll Flutter from 685ce14 to aea4552 (64 revisions) (flutter#4870) [rfw, ci] Regenerate goldens, manually roll flutter#4835 (flutter#4862) Bump actions/checkout from 3.6.0 to 4.0.0 (flutter#4845) ...

mossmana requested a review from reidbaker as a code owner August 24, 2023 19:43

github-actions bot added p: google_maps_flutter platform-android labels Aug 24, 2023

mossmana linked an issue Aug 24, 2023 that may be closed by this pull request

[google_maps_flutter_android] testToggleInfoWindow persistently flaky flutter/flutter#131783

Closed

mossmana changed the title ~~Added delay to address timing issue with info window display status u…~~ Fix for [google_maps_flutter_android] testToggleInfoWindow persistently flaky Aug 24, 2023

Hixie reviewed Aug 24, 2023

View reviewed changes

mossmana requested a review from stuartmorgan August 24, 2023 21:59

mossmana force-pushed the issue131783 branch 2 times, most recently from 6105b17 to 2db9088 Compare August 28, 2023 20:22

mossmana force-pushed the issue131783 branch 2 times, most recently from e9d72c0 to 7d0c5ab Compare August 29, 2023 17:27

mossmana force-pushed the issue131783 branch from 7d0c5ab to cf405dd Compare August 29, 2023 20:02

mossmana changed the title ~~Fix for [google_maps_flutter_android] testToggleInfoWindow persistently flaky~~ [google_maps_flutter_android] Fix for testToggleInfoWindow persistently flaky Aug 31, 2023

mossmana force-pushed the issue131783 branch from cf405dd to e8bc47c Compare September 5, 2023 18:05

stuartmorgan approved these changes Sep 7, 2023

View reviewed changes

mossmana added 4 commits September 11, 2023 15:43

Added delay to address timing issue with info window display status u…

38b41f9

…pdates

Attempting to fix TODO style

e8aced7

Fixed formatting issue

7222352

reverted removal of waitForValueMatchingPredicate

b9a655c

mossmana force-pushed the issue131783 branch from f756c1f to b9a655c Compare September 11, 2023 22:43

mossmana added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 11, 2023

auto-submit bot merged commit 8d553e3 into flutter:main Sep 11, 2023

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Packages from ef0c65ea4ccd to 4512e4de5f8b (2 revisions) flutter/flutter#134540

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023

8d553e3 [google_maps_flutter_android] Fix for testToggleInfoWindow pe…

7a761d1

…rsistently flaky (flutter/packages#4768)

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Packages from ef0c65ea4ccd to e04ba886a85e (5 revisions) flutter/flutter#134546

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023

8d553e3 [google_maps_flutter_android] Fix for testToggleInfoWindow pe…

7473bf1

…rsistently flaky (flutter/packages#4768)

reidbaker mentioned this pull request Sep 15, 2023

Update annotations deps to 1.7 #4935

Merged

11 tasks

bparrishMines mentioned this pull request Nov 6, 2023

Roll Flutter from 29b25165cab8 to f5a983535131 (101 revisions) #5341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter_android] Fix for testToggleInfoWindow persistently flaky #4768

[google_maps_flutter_android] Fix for testToggleInfoWindow persistently flaky #4768

mossmana commented Aug 24, 2023

flutter-dashboard bot commented Aug 24, 2023

mossmana commented Aug 24, 2023

Hixie Aug 24, 2023

mossmana Aug 24, 2023

stuartmorgan Aug 25, 2023

mossmana Aug 29, 2023

mossmana Aug 30, 2023

Hixie commented Aug 24, 2023

gmackall commented Aug 25, 2023

stuartmorgan commented Aug 25, 2023

mossmana commented Aug 28, 2023

stuartmorgan commented Aug 28, 2023

mossmana commented Aug 29, 2023

stuartmorgan commented Aug 29, 2023

stuartmorgan left a comment

stuartmorgan Sep 7, 2023

[google_maps_flutter_android] Fix for testToggleInfoWindow persistently flaky #4768

[google_maps_flutter_android] Fix for testToggleInfoWindow persistently flaky #4768

Conversation

mossmana commented Aug 24, 2023

Pre-launch Checklist

flutter-dashboard bot commented Aug 24, 2023

mossmana commented Aug 24, 2023

Hixie Aug 24, 2023

Choose a reason for hiding this comment

mossmana Aug 24, 2023

Choose a reason for hiding this comment

stuartmorgan Aug 25, 2023

Choose a reason for hiding this comment

mossmana Aug 29, 2023

Choose a reason for hiding this comment

mossmana Aug 30, 2023

Choose a reason for hiding this comment

Hixie commented Aug 24, 2023

gmackall commented Aug 25, 2023

stuartmorgan commented Aug 25, 2023

mossmana commented Aug 28, 2023

stuartmorgan commented Aug 28, 2023

mossmana commented Aug 29, 2023

stuartmorgan commented Aug 29, 2023

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan Sep 7, 2023

Choose a reason for hiding this comment