Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .cirrus.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ task:
matrix:
PLUGIN_SHARDING: "--shardIndex 0 --shardCount 2"
PLUGIN_SHARDING: "--shardIndex 1 --shardCount 2"
MAPS_API_KEY: ENCRYPTED[8b59d83bf4e55b0360b7183a2011241f4e22ba27e31cf06037f7e6e59fa7f9022d82ee604547f8a27d1ceba325cc2588]
create_device_script:
echo no | avdmanager -v create avd -n test -k "system-images;android-21;default;armeabi-v7a"
start_emulator_background_script:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,12 @@ 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated the PR description.

break;
}
case "map#isCompassEnabled":
{
result.success(googleMap.getUiSettings().isCompassEnabled());
break;
}
default:
Expand Down
4 changes: 4 additions & 0 deletions packages/google_maps_flutter/example/android/app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ android {
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
}

defaultConfig {
manifestPlaceholders = [mapsApiKey: "$System.env.MAPS_API_KEY"]
}

buildTypes {
release {
// TODO: Add your own signing config for the release build.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

<activity
android:name=".MainActivity"
android:launchMode="singleTop"
Expand Down
6 changes: 3 additions & 3 deletions packages/google_maps_flutter/example/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ dependencies:
cupertino_icons: ^0.1.0

dev_dependencies:
flutter_test:
sdk: flutter

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

depend on a specific version with caret notation


# For information on the generic Dart part of this file, see the
# following page: https://www.dartlang.org/tools/pub/pubspec
Expand Down
87 changes: 87 additions & 0 deletions packages/google_maps_flutter/example/test_driver/google_maps.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright 2019, the Chromium project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:async';

import 'package:flutter/widgets.dart';
import 'package:flutter_driver/driver_extension.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:google_maps_flutter/google_maps_flutter.dart';

import 'google_maps_test_controller.dart';

const CameraPosition _kInitialCameraPosition =
CameraPosition(target: LatLng(0, 0));

class GoogleMapTest extends StatefulWidget {
GoogleMapTest(
this.mapState,
this._controllerCompleter,
);

final _GoogleMapTestState mapState;
final Completer<GoogleMapController> _controllerCompleter;

@override
_GoogleMapTestState createState() => mapState;
}

class _GoogleMapTestState extends State<GoogleMapTest> {
bool _compassEnabled = false;

@override
Widget build(BuildContext context) {
return Directionality(
textDirection: TextDirection.ltr,
child: GoogleMap(
initialCameraPosition: _kInitialCameraPosition,
compassEnabled: _compassEnabled,
onMapCreated: (GoogleMapController controller) {
widget._controllerCompleter.complete(controller);
},
),
);
}

void toggleCompass() {
setState(() {
_compassEnabled = !_compassEnabled;
});
}
}

void main() {
final Completer<String> allTestsCompleter = Completer<String>();
enableFlutterDriverExtension(handler: (_) => allTestsCompleter.future);

tearDownAll(() => allTestsCompleter.complete(null));

_GoogleMapTestState mapTestState;
GoogleMapController controller;
GoogleMapTestController testController;

setUp(() async {
mapTestState = _GoogleMapTestState();
final Completer<GoogleMapController> controllerCompleter =
Completer<GoogleMapController>();
runApp(GoogleMapTest(mapTestState, controllerCompleter));
controller = await controllerCompleter.future;
testController = GoogleMapTestController(controller.getMethodChannel());
});

test('testCompassToggle', () async {
bool compassEnabled;

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

mapTestState.toggleCompass();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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


// This delay exists as we are waiting for a new build to occur.
await Future<void>.delayed(Duration(seconds: 1), () {});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


compassEnabled = await testController.isCompassEnabled();
expect(compassEnabled, true);
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2019, the Chromium project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:async';

import 'package:flutter_driver/flutter_driver.dart';

Future<void> main() async {
final FlutterDriver driver = await FlutterDriver.connect();
await driver.requestData(null, timeout: const Duration(minutes: 1));
driver.close();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2019, the Chromium project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'package:flutter/services.dart';

class GoogleMapTestController {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

GoogleMapTestController(this._channel);

final MethodChannel _channel;

Future<bool> isCompassEnabled() async {
return await _channel.invokeMethod<bool>('map#isCompassEnabled');
}
}
23 changes: 14 additions & 9 deletions packages/google_maps_flutter/lib/src/controller.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ part of google_maps_flutter;
/// Controller for a single GoogleMap instance running on the host platform.
class GoogleMapController {
GoogleMapController._(
MethodChannel channel,
this.channel,
CameraPosition initialCameraPosition,
this._googleMapState,
) : assert(channel != null),
_channel = channel {
_channel.setMethodCallHandler(_handleMethodCall);
) : assert(channel != null) {
channel.setMethodCallHandler(_handleMethodCall);
}

static Future<GoogleMapController> init(
Expand All @@ -34,7 +33,8 @@ class GoogleMapController {
);
}

final MethodChannel _channel;
@visibleForTesting
final MethodChannel channel;

final _GoogleMapState _googleMapState;

Expand Down Expand Up @@ -79,7 +79,7 @@ class GoogleMapController {
// TODO(amirh): remove this on when the invokeMethod update makes it to stable Flutter.
// https://github.com/flutter/flutter/issues/26431
// ignore: strong_mode_implicit_dynamic_method
await _channel.invokeMethod(
await channel.invokeMethod(
'map#update',
<String, dynamic>{
'options': optionsUpdate,
Expand All @@ -98,7 +98,7 @@ class GoogleMapController {
// TODO(amirh): remove this on when the invokeMethod update makes it to stable Flutter.
// https://github.com/flutter/flutter/issues/26431
// ignore: strong_mode_implicit_dynamic_method
await _channel.invokeMethod(
await channel.invokeMethod(
'markers#update',
markerUpdates._toMap(),
);
Expand All @@ -112,7 +112,7 @@ class GoogleMapController {
// TODO(amirh): remove this on when the invokeMethod update makes it to stable Flutter.
// https://github.com/flutter/flutter/issues/26431
// ignore: strong_mode_implicit_dynamic_method
await _channel.invokeMethod('camera#animate', <String, dynamic>{
await channel.invokeMethod('camera#animate', <String, dynamic>{
'cameraUpdate': cameraUpdate._toJson(),
});
}
Expand All @@ -125,8 +125,13 @@ class GoogleMapController {
// TODO(amirh): remove this on when the invokeMethod update makes it to stable Flutter.
// https://github.com/flutter/flutter/issues/26431
// ignore: strong_mode_implicit_dynamic_method
await _channel.invokeMethod('camera#move', <String, dynamic>{
await channel.invokeMethod('camera#move', <String, dynamic>{
'cameraUpdate': cameraUpdate._toJson(),
});
}

// TODO(iskakaushik): dart doc.
MethodChannel getMethodChannel() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

return channel;
}
}
6 changes: 6 additions & 0 deletions packages/google_maps_flutter/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ dev_dependencies:
flutter_test:
sdk: flutter

# TODO(iskakaushik): The following dependencies can be removed once
# https://github.com/dart-lang/pub/issues/2101 is resolved.
flutter_driver:
sdk: flutter
test: any

flutter:
plugin:
androidPackage: io.flutter.plugins.googlemaps
Expand Down