[Pigeon]: version 0.1.0 (objc and testing features) #152

gaaclarke · 2020-05-08T23:25:03Z

cleaned up objc generation
added ability to generate mock handlers
added pigeon.dart

gaaclarke · 2020-05-11T17:07:42Z

packages/pigeon/mock_handler_tester/test/message.dart

@@ -0,0 +1,147 @@
+// Autogenerated from Pigeon (v0.1.0), do not edit directly.


This file gets regenerated when running run_tests.sh. I still don't have the time to invest in creating a mechanism to generate these during standard execution of the tests.

xster

General approach LGTM. The naming and docs need some more tweaking.

xster · 2020-05-11T23:58:22Z

packages/pigeon/lib/pigeon_lib.dart

@@ -30,7 +30,11 @@ const List<String> _validTypes = <String>[
 /// Metadata to mark an API which will be implemented on the host platform.


Can we make this a bit more explicit? Since this is our canonical code doc that people can cmd+click into when they see the annotation.

Something like

Annotates a platform implemented Pigeon API class. This class groups a collection of Dart↔platform interop methods. These methods are to be invoked by a Dart-side program and are to be received by a platform-side program (such as in Android or iOS) by an implementation class inheriting this class.

or some such?

xster · 2020-05-12T00:06:14Z

packages/pigeon/CHANGELOG.md

+
+* Added pigeon.dart.
+* Fixed some Obj-C linter problems.
+* Added the ability to generate a mock handler in Dart.


Here and below, we should be careful with the terminology to avoid confusion. What you're adding here isn't quite a mock which is a stand-in for a real object and that just registers calls. What this PR does doesn't replace the real object. It just adds a matched object that "works" and that's different from the real implementation. i.e. it's a fake.

There is merit to what you are saying but mock is the terminology used by channels, that's why I chose it (https://api.flutter.dev/flutter/services/MethodChannel/setMockMethodCallHandler.html)

xster · 2020-05-12T00:12:06Z

packages/pigeon/pigeons/message.dart

@@ -15,7 +15,7 @@ class SearchReply {
  String error;
 }

-@HostApi()
+@HostApi(mockDartHandler: 'MockApi')


Now this creates a second class which is exactly the same as the FlutterSearchApi class a bit further down in this file which will make people ask why there's 2 ways of doing the same thing. This file should have more docs.

Not exactly the same but close. I added a comment at the top.

xster · 2020-05-12T00:22:19Z

packages/pigeon/e2e_tests/test_objc/lib/dartle.dart

+  SearchReply search(SearchRequest arg);
+}
+
+void MockApiSetup(MockApi api) {


Let's not call it mock since it's not mocking Api and ApiSetup. If you're still using the real Api and using this as the receiving side, let's just call it ApiTestHandler in this example.

"mock handler" is already used terminology for the same concept elsewhere: https://api.flutter.dev/flutter/services/MethodChannel/setMockMethodCallHandler.html

Ah I see where you're going. That thing is a mock though. It's taking a production class and swapping it out for an alternate one which records and stubs interactions.

This doesn't replace a production class. It's used in parallel with the production class and is a separate "working" class that has a different behavior than the real one.

It'd be like we had client and server and we want to test client. We can have a mockClient which is what we all want but since we can't do it easily for existing tests, we're using client with a fakeServer. But if we do, we can't make the fakeServer be named mockClient. That would be really confusing.

We can call this TestHost[name of API] for instance. Since we're specifically doing the 'host' part of the class annotated by HostApi

I renamed this class to TestHostApi.

xster · 2020-05-12T00:22:47Z

packages/pigeon/e2e_tests/test_objc/lib/dartle.dart

+  SearchReply search(SearchRequest arg);
+}
+
+void MockApiSetup(MockApi api) {


While we're here, can we fix this and the normal static setup method to use camel case naming?

Changed it to MockApi.setup().

oh I like it. What about the production class?

xster · 2020-05-12T00:53:45Z

packages/pigeon/e2e_tests/test_objc/lib/dartle.dart

+  SearchReply search(SearchRequest arg);
+}
+
+void MockApiSetup(MockApi api) {


for this specific static method, it should be a verb rather than a noun. Perhaps flip the naming to put setup in front. e.g. setupApiTestHandler.

Changed it to MockApi.setup().

xster · 2020-05-12T00:56:22Z

packages/pigeon/lib/pigeon_lib.dart

-  const HostApi();
+  const HostApi({this.mockDartHandler});
+
+  /// The name of the interface to generate to receive Pigeon messages locally


Add some more details. Such as

This interface will be placed along side the [HostApi] class. You can implement this interface and invoke `setup[name of this test handler]` to receive calls from your real [HostApi] class in Dart instead of the host platform code as is the case typically. Generally, opt to use a mock of the real [HostApi], such as via mockito, in unit testing. But generating this Dart side test handler could sometimes be useful in integration testing. Defaults to `null` in which case no test handler will be generated.

xster · 2020-05-12T00:57:27Z

packages/pigeon/lib/pigeon_lib.dart

+
+  /// The name of the interface to generate to receive Pigeon messages locally
+  /// for testing.
+  final String mockDartHandler;


dartTestHandler

as noted above

I'd still call this dartHostTestHandler

- added ability to generate mock handlers - added pigeon.dart

gaaclarke · 2020-05-19T23:03:46Z

@xster I think I addressed all the comments. There may still be a question about mockDartHandler let me know if that's still the case and what you decided you'd like it to be. It will be a bit of a pain to change it and want to make sure to get it right.

xster

lgtm % the field name on the annotation

xster · 2020-05-20T00:11:21Z

packages/pigeon/e2e_tests/test_objc/lib/dartle.dart

-      final SearchReply output = api.search(input);
-      return output._toMap();
-    });
+  static void setup(FlutterSearchApi api) {


great! We should note the need to call this in the package's README

I'm a bit surprised though that it's in this abstract FlutterSearchApi class rather than being in Api.

xster · 2020-05-20T00:21:20Z

packages/pigeon/e2e_tests/test_objc/lib/dartle.dart

@@ -128,3 +128,20 @@ class Api {
    }
  }
 }
+


Not for this PR but I don' think we should do it soon. We should doc annotate the output classes, especially since we decided that the person writing the pigeon file might not be the person consuming these Dart/Java/ObjC classes and the latter could use some help in knowing what to do with these.

xster · 2020-05-20T01:07:17Z

packages/pigeon/lib/pigeon_lib.dart

-  const HostApi();
+  const HostApi({this.mockDartHandler});
+
+  /// The name of an interface generated next to the [HostApi] class.  Implement


nit: here and elsewhere, there's a bunch of double spaces.

xster · 2020-05-20T01:07:20Z

packages/pigeon/lib/pigeon_lib.dart

+
+  /// The name of the interface to generate to receive Pigeon messages locally
+  /// for testing.
+  final String mockDartHandler;


I'd still call this dartHostTestHandler

xster · 2020-05-20T01:08:09Z

packages/pigeon/mock_handler_tester/README.md

@@ -0,0 +1,3 @@
+# mock_handler_tester
+
+A test bed for testing the code generated by `mockDartHandler`.


if you rename it, keep the name consistent here and below

gaaclarke · 2020-05-27T18:14:14Z

Landing before build-ipa's task finish. They seem to be fubar, saying stuff about upgrading xcode but showing no progress. Those tests previously passed before my renaming of the field.

gaaclarke force-pushed the pigeon-0.1.0 branch 3 times, most recently from 7c1f29d to 5f74659 Compare May 11, 2020 17:06

gaaclarke commented May 11, 2020

View reviewed changes

gaaclarke marked this pull request as ready for review May 11, 2020 17:07

gaaclarke force-pushed the pigeon-0.1.0 branch from 5f74659 to 1c43bf9 Compare May 11, 2020 17:09

gaaclarke requested a review from xster May 11, 2020 17:09

gaaclarke force-pushed the pigeon-0.1.0 branch from 1c43bf9 to de28ee2 Compare May 11, 2020 17:10

xster reviewed May 12, 2020

View reviewed changes

gaaclarke mentioned this pull request May 13, 2020

[video_player] fixed detach from engine logic flutter/plugins#2759

Merged

13 tasks

- cleaned up objc generation

9a3793c

- added ability to generate mock handlers - added pigeon.dart

gaaclarke force-pushed the pigeon-0.1.0 branch from de28ee2 to 9a3793c Compare May 13, 2020 21:52

gaaclarke added 2 commits May 13, 2020 15:09

update docstrings for @HostApi and @FlutterApi

240f1f5

added comment to message.dart

680338c

gaaclarke force-pushed the pigeon-0.1.0 branch from 5ea12ca to 680338c Compare May 13, 2020 22:18

gaaclarke added 2 commits May 13, 2020 15:35

switched the setup function for FlutterApi to be a static function

8276e70

updated docstring for mockDartHandler

0cd0e9d

gaaclarke requested a review from xster May 15, 2020 21:10

renamed MockApi to TestHostApi

5f89206

xster reviewed May 20, 2020

View reviewed changes

renamed mockDartHandler to testHostDartHandler

00e7f7d

gaaclarke merged commit 18b52ae into flutter:master May 27, 2020

stuartmorgan pushed a commit to stuartmorgan/packages that referenced this pull request Apr 30, 2021

Rebuild example android and ios dirs (flutter#152)

50cc373

stuartmorgan pushed a commit that referenced this pull request Oct 31, 2024

fix bounds of saveLayer (#152)

c6b9871

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pigeon]: version 0.1.0 (objc and testing features) #152

[Pigeon]: version 0.1.0 (objc and testing features) #152

gaaclarke commented May 8, 2020 •

edited

Loading

gaaclarke May 11, 2020

xster left a comment

xster May 11, 2020

gaaclarke May 13, 2020

xster May 12, 2020

gaaclarke May 13, 2020

xster May 12, 2020

gaaclarke May 13, 2020

xster May 12, 2020

gaaclarke May 13, 2020

xster May 16, 2020 •

edited

Loading

gaaclarke May 19, 2020

xster May 12, 2020

gaaclarke May 13, 2020

xster May 16, 2020

gaaclarke May 18, 2020

xster May 12, 2020

gaaclarke May 13, 2020

xster May 16, 2020

xster May 12, 2020

gaaclarke May 13, 2020

xster May 12, 2020

gaaclarke May 13, 2020

xster May 20, 2020

gaaclarke commented May 19, 2020

xster left a comment

xster May 20, 2020

xster May 20, 2020

xster May 20, 2020

xster May 20, 2020

xster May 20, 2020

xster May 20, 2020

gaaclarke commented May 27, 2020

		@@ -0,0 +1,147 @@
		// Autogenerated from Pigeon (v0.1.0), do not edit directly.

		@@ -30,7 +30,11 @@ const List<String> _validTypes = <String>[
		/// Metadata to mark an API which will be implemented on the host platform.

@@ @@ -128,3 +128,20 @@ class Api { @@
                   }
                 }
               }

		@@ -0,0 +1,3 @@
		# mock_handler_tester

		A test bed for testing the code generated by `mockDartHandler`.

[Pigeon]: version 0.1.0 (objc and testing features) #152

[Pigeon]: version 0.1.0 (objc and testing features) #152

Conversation

gaaclarke commented May 8, 2020 • edited Loading

Choose a reason for hiding this comment

xster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xster May 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented May 19, 2020

xster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented May 27, 2020

gaaclarke commented May 8, 2020 •

edited

Loading

xster May 16, 2020 •

edited

Loading