[google_maps] Prepares packages to endorse web. #4064

ditman · 2023-05-23T01:29:10Z

Prepares the google_maps packages to endorse the Web platform:

[google_maps_flutter] Changes example/integration_test to run in the web.
- Splits the only file there in 3 for them to be slightly more manageable.
- (Does not require publishing)
[google_maps_flutter_platform_interface] Adds a test coming from the core plugin to the unit tests of this package.
- (Does not require publishing)
[google_maps_flutter_web] Changes to add an "inspector" object, and to conform with the tests in the core package.
- Implements a GoogleMapsInspectorPlatform to allow integration tests to inspect parts of the internal state of a gmap.
- Fires a MapStyleException when an invalid JSON is used in setMapStyle (was FormatException previously), to conform with the expected behavior in the core plugin tests.
- (Requires publishing)

Issues

Part of: [google_maps_flutter_web] Endorse Google Maps plugin for Web flutter#80688

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ditman · 2023-05-23T01:38:21Z

I thought of renaming the files midway through, and the commits ended up being a little bit sloppy.

This is what an average "fix" of the core test looks like: 31d51ca

Maps on the web need a little bit more scaffolding to render than on mobile, apparently.

ditman · 2023-05-23T01:48:55Z

Testing example/integration_test/tiles_inspector_test.dart...
Running command: "/tmp/cirrus-ci-build/packages/google_maps_flutter/google_maps_flutter/example/android/gradlew app:assembleDebug -Pverbose=true -Ptarget=/tmp/cirrus-ci-build/packages/google_maps_flutter/google_maps_flutter/example/integration_test/tiles_inspector_test.dart" in /tmp/cirrus-ci-build/packages/google_maps_flutter/google_maps_flutter/example/android

This mean that for each file that we have in integration_test, we create a new FTL run, so I might need to re-unify the test file (ggghghghhhaaagh)

ditman · 2023-05-24T01:18:23Z

All that would be needed to endorse the web package after landing this PR is something like this:

23f2c5e...ditman:flutter-packages:maps_endorse_web

stuartmorgan-g · 2023-05-24T15:06:27Z

All that would be needed to endorse the web package after landing this PR is something like this:

23f2c5e...ditman:flutter-packages:maps_endorse_web

We'll want to remember to update the web package's README to say it's endorsed when we do that. IIRC the tooling will let us do it in the same PR since it's not changing Dart code.

stuartmorgan-g · 2023-05-24T11:48:20Z

...s/google_maps_flutter/google_maps_flutter/example/integration_test/tiles_inspector_test.dart

I'm assuming there are no changes as part of the move, so these "new" files don't need review?

The only changes there were added to make the map render in the web. The scaffolding around the map widgets in this test wasn't enough for the web implementation of the map to actually render.

packages/google_maps_flutter/google_maps_flutter_web/CHANGELOG.md

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_flutter_web.dart

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_inspector_web.dart

ditman · 2023-05-25T03:14:09Z

Thanks for the review @stuartmorgan!

I've used the "ConfigurationProvider" trick you suggested so I don't have to punch so many holes into the currently existing objects. This has greatly simplified the actual implementation bits!
I've also versioned the _web change as major because of the change in the Exception thrown by the setConfiguration method.

I'm assuming there are no changes as part of the move, so these "new" files don't need review?

Actually, I did some changes so the tests could run on the web. Highlights:

Stop using dart:io Platform and use defaultTargetPlatform, like so.
Change the scaffolding of the Map widget for it to render on the web, like in this commit.
- (The minimal code currently in the integration tests doesn't work on the web)
Used isWeb to opt-in to some of the tests like here, or opt-out when a feature is not supported, like here.

No expect calls/checks were tweaked in this PR, however.

I also moved this test from the core plugin to the platform interface package (it didn't make much sense in the core plugin.)

ditman · 2023-05-25T03:17:26Z

QQ @stuartmorgan. Each integration_test/*_test.dart file in the core package triggers an FTL run when running for android. Given how FTL some times flakes on our CI, should I un-split the test files so there's only one FTL run when testing this package to minimize the number of consecutive FTL runs?

stuartmorgan-g · 2023-05-27T17:45:12Z

QQ @stuartmorgan. Each integration_test/*_test.dart file in the core package triggers an FTL run when running for android. Given how FTL some times flakes on our CI, should I un-split the test files so there's only one FTL run when testing this package to minimize the number of consecutive FTL runs?

Oh right, I forgot about that. I'm not worried about increasing flake since the tooling currently reruns a failing FTL run, so this would potentially help in that regard, but it does look like this just makes the test way slower overall, so we should recombine them for now. Maybe we can revisit once we have emulator tests, where I don't think we'll have to do that.

stuartmorgan-g

LGTM modulo recombining the tests.

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_controller.dart

stuartmorgan-g · 2023-05-27T17:47:45Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_controller.dart

  // Caching this allows us to re-create the map faithfully when needed.
  MapConfiguration _lastMapConfiguration = const MapConfiguration();
  List<gmaps.MapTypeStyle> _lastStyles = const <gmaps.MapTypeStyle>[];

+  /// Configuration accessor for integration tests


Also, nit: missing period.

Should I push this? dart-archive/linter#3820 :P

ditman · 2023-05-30T22:11:12Z

Will recombine the tests and land this after CI is happy. Thanks!

(Skip on the web)

* Make _map private again. * Pass a configurationProvider function to the inspector so it can look directly at the latest configuration.

ditman · 2023-05-31T00:27:31Z

I think the repo_checks is tripping by the lack of CHANGELOG in the gogle_maps_flutter package, but all the changes happened in google_maps/example/integration_test.

(Added override: no changelog needed label (FYI @stuartmorgan))

stuartmorgan-g · 2023-05-31T02:24:28Z

It's the run_test.sh script; that's not a standard thing so the tooling doesn't know to exclude it.

Having a generically named script in the main package that's actually web-only isn't ideal; I didn't notice that before. We should rename and comment-document it in the second PR.

…ages#4064)

flutter/packages@c85111c...95bb793 2023-05-31 ditman@gmail.com [google_maps] Prepares packages to endorse web. (flutter/packages#4064) 2023-05-30 ditman@gmail.com [google_sign_in] Clarifies canAccessScopes docs. (flutter/packages#4092) 2023-05-30 35989475+MeandNi@users.noreply.github.com [flutter_markdown] Unable to use MarkdownElementBuilder to act those tags without children. (flutter/packages#3952) 2023-05-30 49699333+dependabot[bot]@users.noreply.github.com [local_auth]: Bump androidx.fragment:fragment from 1.5.5 to 1.5.6 in /packages/local_auth/local_auth_android/android (flutter/packages#3553) 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 flutter-ecosystem@google.com,rmistry@google.com 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

ditman · 2023-06-01T18:23:15Z

We should rename and comment-document it in the second PR.

I have a bunch of those scripts in several other packages, I'll rename them all, so they're all called run_web_integration_test.sh or similar.

(Maybe it's time to use the standard script runner though)

ditman · 2023-06-01T21:47:41Z

It's the run_test.sh script; that's not a standard thing so the tooling doesn't know to exclude it.

I ended up removing the bespoke scripts, here: #4129 (now they live in my ~/bin)

(If anybody ever other than me needs them, we can figure out a way to bring them back more properly).

ditman requested a review from stuartmorgan-g as a code owner May 23, 2023 01:29

github-actions bot added p: google_maps_flutter platform-web labels May 23, 2023

This comment was marked as resolved.

# to view

ditman changed the title ~~[google_maps] Prepare packages to endorse web.~~ [google_maps] Prepares packages to endorse web. May 24, 2023

stuartmorgan-g requested changes May 24, 2023

View reviewed changes

ditman requested a review from stuartmorgan-g May 25, 2023 02:56

ditman force-pushed the maps_web_inspection_channel branch from b2f84b5 to 128f46d Compare May 26, 2023 21:47

stuartmorgan-g approved these changes May 27, 2023

View reviewed changes

ditman added 13 commits May 30, 2023 17:13

[google_maps] Split integration_test in 3 smaller files.

ac818b0

Add inspector to tiles_test.dart.

924153d

Rename tests

c0eeaa0

Fix maps_inspector_test.dart (only map rendering)

1c56de2

Make maps_controller_test run on web.

df24182

[maps_platform] Move mipmap unit test from core plugin to here.

c3b1a79

dart format .

f45f3bd

[maps_web] Expose map and options getters.

e0b0586

[maps_web] Add a Google Maps Inspector (and connect it).

958b142

[maps_web] Fire MapStyleException when receiving invalid style JSON.

33fab46

[maps_web] Update CHANGELOG and pubspec.

e21a047

[maps_platform] Remove unnecessary imports.

20749f4

Last attempt for test.

a170ed6

ditman added 10 commits May 30, 2023 17:13

Remove deprecated API use.

e41c042

Revert test so it passes in Android

618ae0c

(Skip on the web)

Address some PR comments:

33d5ca4

* Make _map private again. * Pass a configurationProvider function to the inspector so it can look directly at the latest configuration.

Version _web update as MAJOR.

ebdf7e6

Update mocks.

1d1cfe6

Updates http dev_dependency.

8436be4

Update mockito mocks.

6a8da78

Recombine integration_tests in a single one.

ceb4e19

Clarify configuration accessor on a GoogleMapsController.

10160c0

Add run_test.sh for web convenience.

9cde458

ditman force-pushed the maps_web_inspection_channel branch from 128f46d to 9cde458 Compare May 31, 2023 00:14

ditman added the override: no changelog needed label May 31, 2023

ditman added the autosubmit label May 31, 2023

auto-submit bot merged commit 95bb793 into flutter:main May 31, 2023

ditman deleted the maps_web_inspection_channel branch May 31, 2023 01:34

engine-flutter-autoroll mentioned this pull request May 31, 2023

Roll Packages from c85111c7b880 to 95bb7930e12b (4 revisions) flutter/flutter#127958

Merged

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

95bb793 [google_maps] Prepares packages to endorse web. (flutter/pack…

990f147

…ages#4064)

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

ditman mentioned this pull request Feb 24, 2024

[google_maps_flutter] Add style to widget #6192

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps] Prepares packages to endorse web. #4064

[google_maps] Prepares packages to endorse web. #4064

ditman commented May 23, 2023 •

edited

Loading

ditman commented May 23, 2023 •

edited

Loading

ditman commented May 23, 2023 •

edited

Loading

This comment was marked as resolved.

ditman commented May 24, 2023

stuartmorgan-g commented May 24, 2023

stuartmorgan-g May 24, 2023

ditman May 24, 2023

ditman commented May 25, 2023

ditman commented May 25, 2023 •

edited

Loading

stuartmorgan-g commented May 27, 2023

stuartmorgan-g left a comment

stuartmorgan-g May 27, 2023

ditman May 30, 2023

ditman commented May 30, 2023

ditman commented May 31, 2023 •

edited

Loading

stuartmorgan-g commented May 31, 2023

ditman commented Jun 1, 2023

ditman commented Jun 1, 2023

[google_maps] Prepares packages to endorse web. #4064

[google_maps] Prepares packages to endorse web. #4064

Conversation

ditman commented May 23, 2023 • edited Loading

Issues

Pre-launch Checklist

ditman commented May 23, 2023 • edited Loading

ditman commented May 23, 2023 • edited Loading

This comment was marked as resolved.

ditman commented May 24, 2023

stuartmorgan-g commented May 24, 2023

stuartmorgan-g May 24, 2023

Choose a reason for hiding this comment

ditman May 24, 2023

Choose a reason for hiding this comment

ditman commented May 25, 2023

ditman commented May 25, 2023 • edited Loading

stuartmorgan-g commented May 27, 2023

stuartmorgan-g left a comment

Choose a reason for hiding this comment

stuartmorgan-g May 27, 2023

Choose a reason for hiding this comment

ditman May 30, 2023

Choose a reason for hiding this comment

ditman commented May 30, 2023

ditman commented May 31, 2023 • edited Loading

stuartmorgan-g commented May 31, 2023

ditman commented Jun 1, 2023

ditman commented Jun 1, 2023

ditman commented May 23, 2023 •

edited

Loading

ditman commented May 23, 2023 •

edited

Loading

ditman commented May 23, 2023 •

edited

Loading

ditman commented May 25, 2023 •

edited

Loading

ditman commented May 31, 2023 •

edited

Loading