[google_maps] Add initial google_maps tests #1409

iskakaushik · 2019-03-27T17:21:42Z

Also fixes a bug where we were not doing returning success when we update the markers.

- Also add a controller for querying things - TODO: figureout a way to inject google maps api key

collinjackson · 2019-03-27T22:28:59Z

packages/google_maps_flutter/lib/src/controller.dart

@@ -129,4 +129,9 @@ class GoogleMapController {
      'cameraUpdate': cameraUpdate._toJson(),
    });
  }
+
+  // TODO(iskakaushik): dart doc.
+  MethodChannel getMethodChannel() {


Can we rename _channel to channel and use @visibleForTesting so we aren't exposing this API to developers?

collinjackson · 2019-03-28T21:10:37Z

packages/google_maps_flutter/example/test_driver/google_maps.dart

+    mapTestState.toggleCompass();
+
+    // This delay exists for platform channel propagation.
+    await Future<void>.delayed(Duration(seconds: 1), () {});


Maybe we should make toggleCompass a Future<void> so we can await it in tests and get reliable timing?

The problem here is that we ideally want for a rebuild event to occur before the next assert. using testWidget wrapper we would have pumped a new widget, but it's not easy to do this here. Will fix this in the future.

We can probably do something like:

Future<void> pumpFrame() async { final Completer<void> completer = Completer<void>(); WidgetsBinding.instance.addPostFrameCallback((Duration duration) { completer.complete(null); }); SchedulerBinding.instance.scheduleFrame(); return completer; }

We can even have our own pumpWidget for these driver tests that calls runApp and then pumpFrame.
@collinjackson do you think we should keep such a utility in a common place? it will definitely be useful for both the maps and webview plugins (or alternatively if we can get the WidgetTester working on these tests)

I agree that we should move it to a common place and I'm happy to take the lead on that.

collinjackson · 2019-03-28T21:12:33Z

...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

+          result.success(null);
+          break;
+        }
+      case "map#stateSnapshot":


IMO it would be more consistent with other plugins to have a separate async MethodCall name for each property that you want to read off of the googleMap and expose these properties as a public API of the plugin. This gives developers a suite of useful API for getting for properties of the map that can also be used in integration testing.

collinjackson · 2019-03-28T21:15:54Z

packages/google_maps_flutter/example/test_driver/google_maps_test_controller.dart

+    final List<dynamic> stateSnapshot =
+        await _channel.invokeMethod('map#stateSnapshot');
+    return GoogleMapStateSnapshot(
+      compassEnabled: stateSnapshot[0],


I think it's clearer to use named arguments rather than indexed arguments for plugin method calls. But, you might not need this class if you agree with my other feedback above.

amirh · 2019-03-27T19:45:16Z

...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

@@ -199,6 +199,13 @@ public void onMethodCall(MethodCall call, MethodChannel.Result result) {
          markersController.changeMarkers((List<Object>) markersToChange);
          Object markerIdsToRemove = call.argument("markerIdsToRemove");
          markersController.removeMarkers((List<Object>) markerIdsToRemove);
+          result.success(null);


Is this a bugfix? if so be clear about it in the PR description, changelog, etc (probably worth considering splitting this to a separate PR)

updated the PR description.

amirh · 2019-03-27T19:48:49Z

packages/google_maps_flutter/example/android/app/src/main/AndroidManifest.xml

@@ -13,7 +13,7 @@
            android:value="@integer/google_play_services_version" />
        <meta-data
            android:name="com.google.android.geo.API_KEY"
-            android:value="YOUR KEY HERE" />
+            android:value="${mapsApiKey}" />


Add a comment saying this should be replaced(for those running locally)

amirh · 2019-03-29T00:54:51Z

packages/google_maps_flutter/example/test_driver/google_maps.dart

+    mapTestState.toggleCompass();
+
+    // This delay exists for platform channel propagation.
+    await Future<void>.delayed(Duration(seconds: 1), () {});


We can probably do something like:

Future<void> pumpFrame() async { final Completer<void> completer = Completer<void>(); WidgetsBinding.instance.addPostFrameCallback((Duration duration) { completer.complete(null); }); SchedulerBinding.instance.scheduleFrame(); return completer; }

We can even have our own pumpWidget for these driver tests that calls runApp and then pumpFrame.
@collinjackson do you think we should keep such a utility in a common place? it will definitely be useful for both the maps and webview plugins (or alternatively if we can get the WidgetTester working on these tests)

amirh · 2019-03-29T01:01:43Z

packages/google_maps_flutter/example/test_driver/google_maps.dart

+    compassEnabled = await testController.isCompassEnabled();
+    expect(compassEnabled, false);
+
+    mapTestState.toggleCompass();


Looking just at this test case (e.g if I were coming here to debug a test failure), it's kind of hard to understand what's going on.

I tend to favor keeping test cases more verbose, and having repetition across test cases such that when you read the test case you know what it is doing without having to learn the common facilities shared across the test cases in this file.

In this case I'd prefer to see something like:

pumpWidget(Directionality( textDirection: TextDirection.ltr, child: GoogleMap( initialCameraPosition: _kInitialCameraPosition, compassEnabled: false, onMapCreated: (GoogleMapController controller) { widget._controllerCompleter.complete(controller); }, ), )); compassEnabled = await testController.isCompassEnabled(); expect(compassEnabled, false); pumpWidget(Directionality( textDirection: TextDirection.ltr, child: GoogleMap( initialCameraPosition: _kInitialCameraPosition, compassEnabled: true, onMapCreated: (GoogleMapController controller) { widget._controllerCompleter.complete(controller); }, ), )); compassEnabled = await testController.isCompassEnabled(); expect(compassEnabled, true);

More verbose, but it's clear what the test is doing (also I believe this is the typical style of widget tests in the framework)

Created test_widgets.dart that has pumpWidget, we can potentially move it to a shared location, but fits nicely into the framework pattern.

amirh · 2019-03-29T17:19:38Z

packages/google_maps_flutter/example/ios/Runner/AppDelegate.m

  // Provide the GoogleMaps API key.
-  [GMSServices provideAPIKey:@"YOUR KEY HERE"];
+  NSString* mapsApiKey = [[NSProcessInfo processInfo] environment][@"MAPS_API_KEY"];


This might be confusing for users, what do you think about still using the key "YOUR KEY HERE" if the env variable is not set?

I'm thinking of making it so that in the future for local runs we require the api key to be set via env var. That way we do not need to edit these files, and flutter run will automatically pick them up. But for short term, i'm cool with making this be "YOUR KEY HERE".

amirh · 2019-03-29T17:20:10Z

packages/google_maps_flutter/example/pubspec.yaml

  google_maps_flutter:
    path: ../
+  flutter_driver:
+    sdk: flutter
+  test: any


depend on a specific version with caret notation

amirh · 2019-03-29T17:30:46Z

packages/google_maps_flutter/example/test_driver/google_maps_test_controller.dart

+
+import 'package:flutter/services.dart';
+
+class GoogleMapTestController {


nit: Controller feels a little weird here, as this is not controlling the map but rather allows inspecting it(TestController sounds to me like it's controlling the test). Maybe call it something like GoogleMapInspectors?

amirh

LGTM modulo nit

amirh · 2019-03-29T17:47:05Z

packages/google_maps_flutter/example/test_driver/google_map_inspector.dart

@@ -4,8 +4,8 @@

 import 'package:flutter/services.dart';

-class GoogleMapTestController {
-  GoogleMapTestController(this._channel);
+class GoogleMapInspector {


nit: add a doc comment explaining the role of this class.

I'm confused about why this is its own class. Why not put methods on the GoogleMapController?

Add initial google_maps tests

f219dc6

- Also add a controller for querying things - TODO: figureout a way to inject google maps api key

iskakaushik requested a review from amirh as a code owner March 27, 2019 17:21

googlebot added the cla: yes label Mar 27, 2019

iskakaushik changed the title ~~Add initial google_maps tests~~ [google_maps] Add initial google_maps tests Mar 27, 2019

iskakaushik requested a review from collinjackson March 27, 2019 18:06

Kaushik Iska added 2 commits March 27, 2019 11:36

inject apikey via env var

7da145e

Put a maps api token in cirrus, encrypted

9919710

collinjackson reviewed Mar 28, 2019

View reviewed changes

Kaushik Iska added 2 commits March 28, 2019 15:26

Factor out things to their own methods

5873d0b

IOS impl

b28925e

amirh reviewed Mar 29, 2019

View reviewed changes

Kaushik Iska added 3 commits March 28, 2019 20:41

address comments

4e32075

Add env vars to right places

52e305e

update enc val

759299a

amirh reviewed Mar 29, 2019

View reviewed changes

Kaushik Iska added 3 commits March 29, 2019 10:37

fix test version

6e07157

rename to GoogleMapInspector

e73d48f

check env var and add key

097d2ac

amirh mentioned this pull request Mar 29, 2019

[webview_flutter] Use a pumpWidget utility in e2e tests #1420

Merged

13 tasks

amirh approved these changes Mar 29, 2019

View reviewed changes

Add doc

60798e4

iskakaushik merged commit abb7734 into flutter:master Mar 29, 2019

iskakaushik deleted the gmaps-test-3 branch March 29, 2019 20:22

collinjackson mentioned this pull request Mar 29, 2019

WidgetTester should be available in integration tests flutter/flutter#30214

Closed

julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020

[google_maps] Add initial google_maps tests (flutter#1409)

3cbb2d3


		import 'package:flutter/services.dart';

		class GoogleMapTestController {

[google_maps] Add initial google_maps tests #1409

[google_maps] Add initial google_maps tests #1409

Uh oh!

Conversation

iskakaushik commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

collinjackson Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

collinjackson Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iskakaushik commented Mar 27, 2019 •

edited

Loading

collinjackson Mar 28, 2019 •

edited

Loading

collinjackson Mar 28, 2019 •

edited

Loading