[pigeon] Move test support logic out of the default Dart output. #246

Hixie · 2020-11-20T19:28:53Z

This will allow us to move the mock logic out of the core framework.

I also got distracted and changed some of the output to be cleaner while I was at it.

Hixie · 2020-11-20T19:29:25Z

flutter/plugins#3281 applies these changes to video_player.

gaaclarke

I'm going to pick this up tomorrow, I got to it late and it's taking longer than I expected. Generally looking good I want to run it a few times while I think about it. I think the encode and decode methods was a good compromise while I'm looking at it.

gaaclarke · 2020-12-01T01:41:23Z

packages/pigeon/lib/dart_generator.dart

-  final String nullTag = opt.isNullSafe ? '?' : '';
+  final String nullable = opt.isNullSafe ? '?' : '';
+  final String notNullable = opt.isNullSafe ? '!' : '';
+  bool first = true;


I find these variable names confusing. nullTag described it as a part of the language, nullable doesn't describe it as a character that has semantic meaning. I checked the Dart documentation to see what they call the ! operator and unfortunately I couldn't find a name. In other languages it's called unwrapping.

How about nullableTag and unwrapOperator or unwrapper?

oh i didn't realise "null tag" was the official name. Happy to change these to whatever you want.

done, ?=nulltag and !=unwrapOperator

gaaclarke · 2020-12-01T01:49:08Z

packages/pigeon/lib/dart_generator.dart

 } else if (replyMap['error'] != null) {
-\tfinal Map<dynamic, dynamic> error = replyMap['${Keys.error}'];
+\tfinal Map<Object$nullable, Object$nullable> error = replyMap['${Keys.error}'] as Map<Object$nullable, Object$nullable>;


Why did you change this? Because dynamic doesn't denote null-safety? Why was there a dynamic and an Object before NNBD? Are some things not Objects?

dynamic hides bugs, because it disables all type checking. By using Object? we can see where we're casting and we can make sure the methods (etc) we're using on those objects don't have typos.

(To answer your questions, dynamic is essentially the same as Object?, except that the language never checks what you call on objects that are marked dynamic, it's like you're in an untyped language. Everything is an Object?.)

gaaclarke

Okay, finished up the review. The only big issues I noticed is that there is a null check that has been removed from the decode method. That makes me nervous, do you think that is correct and that it is adequately tested already? Also there is an added case where channels can fail silently and bypass the provided API.

gaaclarke · 2020-12-01T17:54:39Z

packages/pigeon/lib/pigeon_lib.dart

+  /// calls from your real [HostApi] class in Dart instead of the host
+  /// platform code, as is typical.
+  ///
+  /// When using this, you must specify the `--out-test-dart` argument


You need to use snake case, not kebob case here.

gaaclarke · 2020-12-01T17:57:03Z

packages/pigeon/lib/pigeon_lib.dart

+          from: context.fromUri(
+            path.toUri(path.dirname(path.absolute(options.dartTestOut))),
+          ),
+        );


You do the same path juggling here twice, it could be more clear to pull some of that into a helper function.

If you mean the code two lines above this, it's subtly different in ways that makes it hard to usefully factor out. :-(
(I spent some time trying to find a way to factor it out cleanly and it ended up just being simpler to do it this way.)
That said if you have any suggestions on how to factor it out let me know.

Couldn't you just do this?

String posixify(String path) { final path.Context context = path.Context(style: path.Style.posix); return context.fromUri(path.toUri(path.absolute(path)); }

posixify(options.dartOut); posixify(path.dirname(options.dartTestOut);

The theory being that path.dirname(path.absolute(x)) == path.absolute(path.dirname(x))? Seems reasonable, let me give it a try.

gaaclarke · 2020-12-01T18:00:33Z

packages/pigeon/mock_handler_tester/test/message.dart

+    final Map<Object, Object> pigeonMap = message as Map<Object, Object>;
+    return SearchReply()


_fromMap used to accept null input, now decode doesn't. I see there appears to be one place where decode is called without a null check to the argument. Are you sure this is safe to remove?

I must have misread the code; it looked like it used to crash when the message was null, but I see in the output for video_player that there's a null check.

I'm happy to make it accept null again.

(Is there ever a valid reason to send null?)

Oh I remember why I made it not accept null -- it's not so much that the input is non-nullable, so much as that the output is non-nullable. With a null input I don't know what to return. I could return null but then I need to null check around every call site anyway, and it seemed cleaner to have the null check before the call than after the call.

Yea, that makes sense. I just checked the output with nullsafety and it looks like this:

static SearchReply _fromMap(Map<dynamic, dynamic> pigeonMap) { final SearchReply result = SearchReply(); result.result = pigeonMap['result']; result.error = pigeonMap['error']; return result; }

Looks like I already got rid of the null check, this generated code just hasn't been updated in a while.

gaaclarke · 2020-12-01T18:06:12Z

packages/pigeon/mock_handler_tester/test/message.dart

+        channel.setMessageHandler(null);
+      } else {
+        channel.setMessageHandler((Object message) async {
+          if (message == null) {


We shouldn't bypass calling into the api. Either we support null here and pass it through to the api, or we don't and we assert. Silently bypassing the API is just going to create hard to debug situations.

But then FlutterSearchApi.search has to take a nullable argument. Do we want that?

I think this is the main open question here. (bold so I can find this in the github page)

Ok sounds like from the previous comment that you're good leaving this as-is.

gaaclarke · 2020-12-01T18:09:21Z

packages/pigeon/mock_handler_tester/test/message.dart

-
-    final Map<dynamic, dynamic> replyMap = await channel.send(requestMap);
+    final Map<Object, Object> replyMap =
+        await channel.send(encoded) as Map<Object, Object>;


Are these casts incurring any runtime cost?

My understanding is that casts are cheaper than dynamic dispatch on dynamic these days, but I could be wrong.

(To put it another way: there were casts before, this just made them visible.)

(Specifically, before, we were assigning a dynamic (the return value of await channel.send(encoded)) to a variable of type Map<dynamic, dynamic>, which implies a cast. Now we are assigning an Object to a variable of type Map<Object, Object> with an explicit cast. I wouldn't be surprised if the assembly was actually byte for byte identical, though I haven't checked.)

gaaclarke · 2020-12-01T18:11:52Z

packages/pigeon/mock_handler_tester/test/message.dart

-        final SearchReply output = api.search(input);
-        return <dynamic, dynamic>{'result': output._toMap()};
-      });
+      return SearchReply.decode(replyMap['result']);


Here's the call whose behavior would change as a result of removing the null check in decode.

Ah, yes. In null-safe mode this has a ! in the new generated code. I can check it for a result key and throw if it's missing, if you like. Or do something else. Your call.

gaaclarke

I think this is good. I got a confused by the skip in checking in generated code. Thanks ian.

This will allow us to move the mock logic out of the core framework.

Hixie · 2020-12-10T21:29:37Z

Comments applied, tests pass! I'ma land it!

Deep location example: Migrate to null safety

Hixie mentioned this pull request Nov 20, 2020

Document how to use pigeon and update to the latest version. flutter/plugins#3281

Merged

Hixie marked this pull request as draft November 20, 2020 20:02

Hixie force-pushed the extract_tests branch 11 times, most recently from dd8a506 to 0bcb6a8 Compare November 24, 2020 06:11

Hixie marked this pull request as ready for review November 30, 2020 18:16

Hixie requested a review from gaaclarke December 1, 2020 00:45

gaaclarke reviewed Dec 1, 2020

View reviewed changes

gaaclarke changed the title ~~Move test support logic out of the default Dart output.~~ [pigeon] Move test support logic out of the default Dart output. Dec 1, 2020

gaaclarke requested changes Dec 1, 2020

View reviewed changes

gaaclarke approved these changes Dec 8, 2020

View reviewed changes

Hixie force-pushed the extract_tests branch from 0bcb6a8 to 313a13f Compare December 9, 2020 21:30

Move test support logic out of the default Dart output.

dcb439d

This will allow us to move the mock logic out of the core framework.

Hixie force-pushed the extract_tests branch from 313a13f to dcb439d Compare December 9, 2020 21:41

Hixie merged commit 1313c80 into flutter:master Dec 10, 2020

Hixie deleted the extract_tests branch December 10, 2020 21:29

austinstoker pushed a commit to austinstoker/packages that referenced this pull request Apr 29, 2022

Merge pull request flutter#246 from devj3ns/master

1761f81

Deep location example: Migrate to null safety

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pigeon] Move test support logic out of the default Dart output. #246

[pigeon] Move test support logic out of the default Dart output. #246

Hixie commented Nov 20, 2020

Hixie commented Nov 20, 2020

gaaclarke left a comment

gaaclarke Dec 1, 2020

Hixie Dec 1, 2020

Hixie Dec 1, 2020

gaaclarke Dec 1, 2020

Hixie Dec 1, 2020

Hixie Dec 1, 2020

gaaclarke left a comment •

edited

Loading

gaaclarke Dec 1, 2020

Hixie Dec 1, 2020

gaaclarke Dec 1, 2020

Hixie Dec 1, 2020

gaaclarke Dec 8, 2020

Hixie Dec 9, 2020

gaaclarke Dec 1, 2020

Hixie Dec 1, 2020

Hixie Dec 1, 2020

gaaclarke Dec 8, 2020 •

edited

Loading

gaaclarke Dec 1, 2020

Hixie Dec 1, 2020

Hixie Dec 1, 2020 •

edited

Loading

Hixie Dec 9, 2020

gaaclarke Dec 1, 2020

Hixie Dec 1, 2020

Hixie Dec 1, 2020

Hixie Dec 1, 2020 •

edited

Loading

gaaclarke Dec 1, 2020

Hixie Dec 1, 2020

gaaclarke left a comment

Hixie commented Dec 10, 2020

		final Map<Object, Object> pigeonMap = message as Map<Object, Object>;
		return SearchReply()

[pigeon] Move test support logic out of the default Dart output. #246

[pigeon] Move test support logic out of the default Dart output. #246

Conversation

Hixie commented Nov 20, 2020

Hixie commented Nov 20, 2020

gaaclarke 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 left a comment • 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

gaaclarke Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie Dec 1, 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

Hixie Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

Hixie commented Dec 10, 2020

gaaclarke left a comment •

edited

Loading

gaaclarke Dec 8, 2020 •

edited

Loading

Hixie Dec 1, 2020 •

edited

Loading

Hixie Dec 1, 2020 •

edited

Loading