From 6c1906ae95ec7052746f6ac5a5e20b88aee7a0ef Mon Sep 17 00:00:00 2001 From: Amir Hardon Date: Wed, 23 Oct 2019 11:12:37 -0700 Subject: [PATCH 1/3] Forbid `... implements UrlLauncherPlatform` --- .../CHANGELOG.md | 7 +++ .../lib/url_launcher_platform_interface.dart | 45 +++++++++++++++---- .../pubspec.yaml | 3 +- .../method_channel_url_launcher_test.dart | 43 ++++++++++++++---- 4 files changed, 80 insertions(+), 18 deletions(-) create mode 100644 packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md diff --git a/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md new file mode 100644 index 000000000000..a849e404cd7a --- /dev/null +++ b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md @@ -0,0 +1,7 @@ +## 1.0.1 + +* Enforce that UrlLauncherPlatform isn't implemented with `implements`. + +## 1.0.0 + +* Initial release. diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 8e1d090eef2b..a77fd7decda4 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -4,19 +4,26 @@ import 'dart:async'; -import 'package:meta/meta.dart' show required; +import 'package:meta/meta.dart' show required, visibleForTesting; import 'method_channel_url_launcher.dart'; /// The interface that implementations of url_launcher must implement. /// -/// Platform implementations that live in a separate package should extend this -/// class rather than implement it as `url_launcher` does not consider newly -/// added methods to be breaking changes. Extending this class (using `extends`) -/// ensures that the subclass will get the default implementation, while -/// platform implementations that `implements` this interface will be broken by -/// newly added [UrlLauncherPlatform] methods. +/// Platform implementations should extend this class rather than implement it as `url_launcher` +/// does not consider newly added methods to be breaking changes. Extending this class +/// (using `extends`) ensures that the subclass will get the default implementation, while +/// platform implementations that `implements` this interface will be broken by newly added +/// [UrlLauncherPlatform] methods. abstract class UrlLauncherPlatform { + /// Only mock implementations should set this to true. + /// + /// Mockito mocks are implementing this class with `implements` which is forbidden for anything + /// other than mocks (see class docs). This property provides a backdoor for mockito mocks to + /// skip the verification that the class isn't implemented with `implements`. + @visibleForTesting + bool get isMock => false; + /// The default instance of [UrlLauncherPlatform] to use. /// /// Platform-specific plugins should override this with their own @@ -24,7 +31,21 @@ abstract class UrlLauncherPlatform { /// register themselves. /// /// Defaults to [MethodChannelUrlLauncher]. - static UrlLauncherPlatform instance = MethodChannelUrlLauncher(); + static UrlLauncherPlatform _instance = MethodChannelUrlLauncher(); + + static UrlLauncherPlatform get instance => _instance; + + static set instance(UrlLauncherPlatform instance) { + if (!instance.isMock) { + try { + instance._verifyProvidesDefaultImplementations(); + } on NoSuchMethodError catch (_) { + throw AssertionError( + 'Platform interfaces must not be implemented with `implements`'); + } + } + _instance = instance; + } /// Returns `true` if this platform is able to launch [url]. Future canLaunch(String url) { @@ -51,4 +72,12 @@ abstract class UrlLauncherPlatform { Future closeWebView() { throw UnimplementedError('closeWebView() has not been implemented.'); } + + // This method makes sure that UrlLauncher isn't implemented with `implements`. + // + // See class doc for more details on why implementing this class is forbidden. + // + // This private method is called by the instance setter, which fails if the class is + // implemented with `implements`. + void _verifyProvidesDefaultImplementations() {} } diff --git a/packages/url_launcher/url_launcher_platform_interface/pubspec.yaml b/packages/url_launcher/url_launcher_platform_interface/pubspec.yaml index 99b260efe943..cbdfed122ca7 100644 --- a/packages/url_launcher/url_launcher_platform_interface/pubspec.yaml +++ b/packages/url_launcher/url_launcher_platform_interface/pubspec.yaml @@ -4,7 +4,7 @@ author: Flutter Team homepage: https://github.com/flutter/plugins/tree/master/packages/url_launcher/url_launcher_platform_interface # NOTE: We strongly prefer non-breaking changes, even at the expense of a # less-clean API. See https://flutter.dev/go/platform-interface-breaking-changes -version: 1.0.0 +version: 1.0.1 dependencies: flutter: @@ -14,6 +14,7 @@ dependencies: dev_dependencies: flutter_test: sdk: flutter + mockito: ^4.1.1 environment: sdk: ">=2.0.0-dev.28.0 <3.0.0" diff --git a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart index f409441903f3..7422f8dacd9e 100644 --- a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart +++ b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart @@ -2,15 +2,40 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:mockito/mockito.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; -import 'package:url_launcher_platform_interface/method_channel_url_launcher.dart'; -import 'package:url_launcher_platform_interface/url_launcher_platform_interface.dart'; + +import '../lib/method_channel_url_launcher.dart'; +import '../lib/url_launcher_platform_interface.dart'; void main() { - group('$MethodChannelUrlLauncher', () { - TestWidgetsFlutterBinding.ensureInitialized(); + TestWidgetsFlutterBinding.ensureInitialized(); + + group('$UrlLauncherPlatform', () { + test('$MethodChannelUrlLauncher() is the default instance', () { + expect(UrlLauncherPlatform.instance, + isInstanceOf()); + }); + + test('Cannot be implemented with `implements', () { + expect(() { + UrlLauncherPlatform.instance = ImplementsUrlLauncherPlatform(); + }, throwsA(isInstanceOf())); + }); + + test('Can be mocked with `implements', () { + final ImplementsUrlLauncherPlatform mock = ImplementsUrlLauncherPlatform(); + when(mock.isMock).thenReturn(true); + UrlLauncherPlatform.instance = mock; + }); + test('Can be exteneded', () { + UrlLauncherPlatform.instance = ExtendsUrlLauncherPlatform(); + }); + }); + + group('$MethodChannelUrlLauncher', () { const MethodChannel channel = MethodChannel('plugins.flutter.io/url_launcher'); final List log = []; @@ -24,11 +49,6 @@ void main() { log.clear(); }); - test('is the default $UrlLauncherPlatform instance', () { - expect(UrlLauncherPlatform.instance, - isInstanceOf()); - }); - test('canLaunch', () async { await launcher.canLaunch('http://example.com/'); expect( @@ -258,3 +278,8 @@ void main() { }); }); } + +class ImplementsUrlLauncherPlatform extends Mock + implements UrlLauncherPlatform {} + +class ExtendsUrlLauncherPlatform extends UrlLauncherPlatform {} From 0f726d79136c8cdf47959fad428a09955089c70b Mon Sep 17 00:00:00 2001 From: Amir Hardon Date: Wed, 23 Oct 2019 12:39:21 -0700 Subject: [PATCH 2/3] review fixes --- .../lib/url_launcher_platform_interface.dart | 2 ++ .../test/method_channel_url_launcher_test.dart | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index a77fd7decda4..a17aa0626126 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -35,6 +35,8 @@ abstract class UrlLauncherPlatform { static UrlLauncherPlatform get instance => _instance; + // TODO(amirh): Extract common platform interface logic. + // https://github.com/flutter/flutter/issues/43368 static set instance(UrlLauncherPlatform instance) { if (!instance.isMock) { try { diff --git a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart index 7422f8dacd9e..6dfe37eb36a6 100644 --- a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart +++ b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart @@ -18,19 +18,19 @@ void main() { isInstanceOf()); }); - test('Cannot be implemented with `implements', () { + test('Cannot be implemented with `implements`', () { expect(() { UrlLauncherPlatform.instance = ImplementsUrlLauncherPlatform(); }, throwsA(isInstanceOf())); }); - test('Can be mocked with `implements', () { + test('Can be mocked with `implements`', () { final ImplementsUrlLauncherPlatform mock = ImplementsUrlLauncherPlatform(); when(mock.isMock).thenReturn(true); UrlLauncherPlatform.instance = mock; }); - test('Can be exteneded', () { + test('Can be extended', () { UrlLauncherPlatform.instance = ExtendsUrlLauncherPlatform(); }); }); From 31706e13e14e544e8aacd520d22115daf93d7c84 Mon Sep 17 00:00:00 2001 From: Amir Hardon Date: Wed, 23 Oct 2019 12:39:45 -0700 Subject: [PATCH 3/3] format --- .../test/method_channel_url_launcher_test.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart index 6dfe37eb36a6..b98812b99839 100644 --- a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart +++ b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart @@ -25,7 +25,8 @@ void main() { }); test('Can be mocked with `implements`', () { - final ImplementsUrlLauncherPlatform mock = ImplementsUrlLauncherPlatform(); + final ImplementsUrlLauncherPlatform mock = + ImplementsUrlLauncherPlatform(); when(mock.isMock).thenReturn(true); UrlLauncherPlatform.instance = mock; });