Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[google_maps_flutter] Use an interface for test inspection #6116

Merged
merged 8 commits into from
Jul 23, 2022

Conversation

stuartmorgan-g
Copy link
Contributor

Instead of hard-coding knowledge of platform channel details in the integration test inspector, which makes it impossible to run with any other implementation, add an inspector interface to the platform interface, and use that for the tests.

Fixes flutter/flutter#55504
Precursor to federating mobile implementations for flutter/flutter#68498

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

/// A method-channel-based implementation of [GoogleMapsInspectorPlatform], for
/// use in tests in conjunction with [MethodChannelGoogleMapsFlutter].
// TODO(stuartmorgan): Move this into the platform implementations when
// federating the mobile implementations.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is here with a TODO instead of being moved to live with the default method channel implementation in the platform interface package because I plan to federate the mobile implementations shortly, and move them to in-package method channel implementations, at which point the default method channel inspector would be unused, but we'd be stuck with the extra copy indefinitely since removing it again would be a breaking change. Leaving it local for now lets us avoid that extra copy.

/// Populates [GoogleMapsFlutterInspectorPlatform.instance] to allow
/// inspecting the platform map state.
@visibleForTesting
void enableDebugInspection() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ditman Does this structure address flutter/flutter#80688 (comment)?

My thought was that by having the population controlled from here, you should be able to do whatever bridging is necessary between the private data you mentioned and the web implementation of the inspector. E.g., its constructor could take a reference to the "very private" data you mentioned, or if that's too much access, maybe to a provider object that can access pieces of it via lambas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be enough. The end goal is to "leak" this _googleMap object to whatever extends GoogleMapsInspectorPlatform we created for the web:

https://github.com/flutter/plugins/blob/main/packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_controller.dart#L86

Which is wrapped by a Controller object that is indexed by mapId here:

https://github.com/flutter/plugins/blob/main/packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_flutter_web.dart#L17

(Maybe this method can just enable a boolean that lets us user a _googleMap getter?)

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks for tackling it.

The only weirdness that I can think of when we eventually endorse the web implementation (with its own Inspector), is that there's certain values that are being passed in as configuration that don't make sense in the web.

So we either store whatever the user wanted and parrot it back, or we adapt the tests to face the truth of the web, where some of the inspected attributes will always be "false" :P

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to implement the web-version of the inspector ASAP next week!

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 23, 2022
@auto-submit auto-submit bot merged commit 398f8f7 into flutter:main Jul 23, 2022
yutaaraki-toydium pushed a commit to yutaaraki-toydium/plugins that referenced this pull request Aug 12, 2022
@charlieforward9
Copy link

The only weirdness that I can think of when we eventually endorse the web implementation

Is there an internal timeline to endorse the web implementation?

@stuartmorgan-g stuartmorgan-g deleted the maps-inpector-interface branch January 24, 2023 16:22
@stuartmorgan-g
Copy link
Contributor Author

@charlieforward9 Please follow the issue I pointed you to earlier rather than pinging old PRs. PR comments are only for discussion of the PR itself.

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: google_maps_flutter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate GoogleMapInspector to a platform-interface based API.
3 participants