Add strong mode-compliant 'typed' API #26

srawlins · 2016-06-16T15:21:47Z

Fixes #25

This solution is modeled after Java Mockito's implementation. Unfortunately, Java doesn't have named args, so this implementation is a little uglier with regards to named args.

The README and the tests show usage. What do you think?

Alternatives

@TedSander and I came up with the typed/*<horror>*/(matcher, {name}) API. But we're not married to it, if people can think of something cleaner. Maybe a separate typedNamed?

We could not conceive of a way for the API to understand when an argument is passed as a named argument. We can't rely on order (when(foo.fn(a: typed(...), b: typed(...)))), because Invocation's namedArguments is a Map, possibly with unstable order.

dskloetg · 2016-06-21T08:33:38Z

Does the object on which we define behavior need to be the same as the object that implements the behavior? Maybe mocks can be created in pairs: one object on which we specify and verfiy the behavior from the test and one object we pass to the object under test. Then the object on which the behavior is defined doesn't need to implement the exact same interface as the mocked type but can accept matchers without causing errors.

srawlins · 2016-06-21T20:20:58Z

OK this API is ready to merge, if we like it.

There is a lot of code, but it is actually nicely contained:

the typed method (the public API),
two private top-level objects, and a private top-level method,
a top-level resetMockitoState() method, only used in testing,
a new FakeInvocation class, where 95% of the work is done,
one tiny check in Mock.noSuchMethod, to see if the typed API is invoked.

srawlins · 2016-06-21T20:23:36Z

@dskloetg : I think using two different objects would be a large break from the mockito API. Something that Mockito gives us for free is method signatures. For example, the analyzer complains about the following:

class Cat {
  bool eatFood(List<String> foods) => true;
}

class MockCat extends Mock implements Cat {}

void fn() {
  var cat = new MockCat();
  when(cat.eatFood()).thenReturn(true);
}

Do you have a mockup of the API you had in mind?

dskloetg · 2016-06-22T10:13:54Z

README.md

+
+Unfortunately, the use of the arg matchers in mock method calls (like `cat.eatFood(any)`)
+violates the [Strong mode] type system. Specifically, if the method signature of a mocked
+method has a parameter with a parameterized type (like `List<int>`), then passing `any` or


Wouldn't this also happen if the parameter has a simple type?
"Unsound implicit cast from dynamic to String"

I don't think so. This is why it's not such a prominent problem. Just a little prominent:

$ cat lib/dogs.dart import 'mockito.dart'; class Dog { bool eatFood(String food) => true; bool eatFood2(List<String> foods) => true; } class MockDog extends Mock implements Dog {} void fn() { var dog = new MockDog(); when(dog.eatFood(any)).thenReturn(true); when(dog.eatFood2(any)).thenReturn(true); } $ dartanalyzer --strong lib/dogs.dart Analyzing [lib/dogs.dart]... [warning] Unsound implicit cast from dynamic to List<String> (/Users/srawlins/code/dart-mockito/lib/dogs.dart, line 13, col 21) 1 warning found.

I'd be curious to know why one is considered unsound but the other isn't.

Any implicit or explicit cast might fail in DDC. Similarly, any implicit or explicit cast might fail in regular Dart checked mode. In general, for simple types (like String), if the cast passed in checked mode, it will pass in DDC. For complex types (like List<String>), DDC may throw a type error where checked mode wouldn't have. This is because the checked mode check is unsound for composite types like List<String> (a List<dynamic> will be allowed through), and so DDC strengthens the cast. Since these are places where code that worked in checked mode may fail in DDC, we emit a special warnings for these casts that might fail in DDC where the would not have in checked mode.

dskloetg · 2016-06-23T08:13:50Z

lib/mockito.dart

+// Return a new [Invocation], reconstituted from [invocation], [_typedArgs],
+// and [_typedNamedArgs].
+Invocation _reconstituteInvocation(Invocation invocation) {
+  var newInvocation = new FakeInvocation(invocation);


just return without temp variable?

Andersmholmgren · 2016-06-23T08:14:59Z

Just curious why an a new typed function is needed rather than making all the existing functions generic.

Eg

argThat/*<list<string>>*/(..)

etc

dskloetg · 2016-06-23T08:17:25Z

I'm on vacation so I won't get back to this for 2.5 weeks. You can treat my remaining comments as FYI.

srawlins · 2016-07-07T15:57:36Z

@Andersmholmgren It's mostly just backwards-compatibility issues. If we changed any and argThat to return null instead of returning _ArgMatcher objects, then there would be a huge breaking change, where you could no longer use, e.g. when(m.fn(any, null, any)). I think there were more incompatibilities.

srawlins · 2016-07-07T15:57:49Z

@TedSander This is ready to review/merge!

dskloetg · 2016-07-11T12:13:06Z

So we don't want to change argThat and any for compatibility, but wrapping is also a bit ugly.
What about introducing multiple new functions that replace argThat and any (and others) that do what their wrapped counterparts would do?

leafpetersen · 2016-07-11T22:08:47Z

README.md

+
+```dart
+when(cat.walk(
+    typed/*<List<String>>*/(any),


Not sure whether this approach is still under consideration, but I don't think you should need to pass these type arguments explicitly - they are in a position that should be inferrable.

It is. Sam is just checking to make sure it will work with DDC. Do you see any reason it won't @leafpetersen

Removing those comments would be a nice improvement.

If I'm understanding the overall approach here, I think this won't work without some compiler help. The way that DDC generates code means that you can only get to noSuchMethod by going through dynamic. So

dynamic cat = new MockCat(); cat.eatFood([]);

would work, but

var cat = new MockCat(); cat.eatFood([]); }

won't, because we will infer that cat has type MockCat, and not generate the dynamic call. It's arguably a bug that we still allow noSuchMethod to suppress the warnings that you should get by not fully implementing the Cat interface.

I think this might be a place where some language and/or compiler help might be appropriate.

Thanks Leaf exactly what we wanted to know. Some language or compiler help would be appreciated. Even if there are no changes to the language having recommendations from the DDC team on how we should solve this would be appreciated. I think Dart should have a solution to stubbing/mocking for tests.

Filed a bug on the fact that we just silently accept this here:

dart-lang/sdk#26863

Supporting this sort of thing is something I think we should do. I'd like to be sure we understand and cover all (or as many as possible) of the use cases though.

srawlins · 2016-08-02T13:50:20Z

I can confirm that as of dart-archive/dev_compiler@e5c2aac, the typed API in this pull request functions well with DDC. It's ready to roll.

dskloetg · 2016-08-02T14:23:40Z

What do you think of my last suggestion from 22 days ago?

srawlins · 2016-08-02T15:09:32Z

@dskloetg I'd have to make the following matching functions:

Old	New
get any	typedAny({String named})
argThat(Matcher matcher)	typedArgThat(Matcher matcher, {String named})
get captureAny	typedCaptureAny({String named})
captureThat	typedCaptureThat(Matcher matcher, {String named})

This converts two getters to functions, only so they can be used with named args 😦 . I think this could have some real consequences, as passing any calls the any getter, but passing typedAny will now send the typedAny closure, without calling it. This mistake would also be hard (impossible?) to catch, as a closure could be a legitimate argument to a function.

I think the single typed function is a simpler API, and its only two characters longer (a ( and a )).

TedSander · 2016-08-03T00:06:22Z

lib/mockito.dart

+
+/// An Invocation implementation that allows all attributes to be passed into
+/// the constructor.
+class FakeInvocation extends Invocation {


I'm not really in love with this name, and the comment doesn't seem to really apply anymore. To a user they can't pass in all the attributes. Would TypedInvocation be better, or InvocationForTypedArguments? Fake doesn't really tell me much here.

Done. _InvocationForTypedArguments.

TedSander · 2016-08-03T00:13:39Z

lib/mockito.dart

+        // `when(obj.fn(typed(any, name: 'a')))`.
+        throw new ArgumentError(
+            'A typed argument was declared with name $name, but was not passed '
+            'as an argument named $name.');


I think the example would be useful in the error.
Maybe:
Bad: when(obj.fn(typed(any, name: 'a')))
Good: when(obj.fn(a: typed(any, name: 'a')))

TedSander · 2016-08-03T00:28:59Z

We should also update the minor version. Maybe while we are at it we should take it to 1.0.0

This code has been stable, and the dart team has asked us to reflect that fact.

srawlins · 2016-08-03T01:50:13Z

OK tests are good; new DDC tests in tool/. I'm fine for bumping to 1.0.0. Should I go for it?

TedSander · 2016-08-03T02:03:07Z

lib/src/mock.dart

@@ -55,6 +60,139 @@ class Mock {
  String toString() => _givenName != null ? _givenName : runtimeType.toString();
 }

+// Return a new [Invocation], reconstituted from [invocation], [_typedArgs],
+// and [_typedNamedArgs].
+Invocation _reconstituteInvocation(Invocation invocation) =>


This seems like a little overkill, why not just inline the new?

TedSander · 2016-08-03T02:11:03Z

Update the version number to 1.0.0??

srawlins · 2016-08-03T22:56:37Z

Done. Version bump included.

TedSander · 2016-08-03T23:10:19Z

Thanks Sam. Excited that we can now write DDC tests!

Add strong mode-compliant 'typed' API

srawlins force-pushed the typed-api branch from 9595e27 to 935aea1 Compare June 16, 2016 15:28

srawlins force-pushed the typed-api branch 2 times, most recently from e6b4976 to 93396e4 Compare June 21, 2016 20:16

dskloetg reviewed Jun 22, 2016
View reviewed changes

srawlins force-pushed the typed-api branch from 93396e4 to 0d48651 Compare June 23, 2016 02:45

dskloetg reviewed Jun 23, 2016
View reviewed changes

leafpetersen reviewed Jul 11, 2016
View reviewed changes

TedSander reviewed Aug 3, 2016
View reviewed changes

Add strong mode-compliant 'typed' API

e32f79a

srawlins force-pushed the typed-api branch from 75972a7 to c44aa8b Compare August 3, 2016 01:49

srawlins force-pushed the typed-api branch from c44aa8b to 5c7605b Compare August 3, 2016 01:51

TedSander reviewed Aug 3, 2016
View reviewed changes

Address feedback, remove type annotations

beb8b26

srawlins force-pushed the typed-api branch from 5c7605b to beb8b26 Compare August 3, 2016 22:56

TedSander merged commit 625f0f4 into dart-lang:master Aug 3, 2016

srawlins deleted the typed-api branch August 4, 2016 14:43

srawlins mentioned this pull request Aug 4, 2016

Mockito's "any" incompatible with strong mode analyzer dart-lang/sdk#26036

Closed

mosuem pushed a commit to dart-lang/test that referenced this pull request Oct 17, 2024

Merge pull request dart-lang/mockito#26 from srawlins/typed-api

d371443

Add strong mode-compliant 'typed' API

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add strong mode-compliant 'typed' API #26

Add strong mode-compliant 'typed' API #26

srawlins commented Jun 16, 2016 •

edited

Loading

dskloetg commented Jun 21, 2016

srawlins commented Jun 21, 2016

srawlins commented Jun 21, 2016

dskloetg Jun 22, 2016

srawlins Jun 23, 2016

dskloetg Jun 23, 2016

leafpetersen Jul 11, 2016

dskloetg Jun 23, 2016

srawlins Jul 7, 2016

Andersmholmgren commented Jun 23, 2016

dskloetg commented Jun 23, 2016

srawlins commented Jul 7, 2016

srawlins commented Jul 7, 2016

dskloetg commented Jul 11, 2016

leafpetersen Jul 11, 2016

TedSander Jul 11, 2016

leafpetersen Jul 11, 2016

TedSander Jul 11, 2016

leafpetersen Jul 11, 2016

srawlins commented Aug 2, 2016

dskloetg commented Aug 2, 2016

srawlins commented Aug 2, 2016

TedSander Aug 3, 2016

srawlins Aug 3, 2016

TedSander Aug 3, 2016 •

edited

Loading

srawlins Aug 3, 2016

TedSander commented Aug 3, 2016

srawlins commented Aug 3, 2016

TedSander Aug 3, 2016

srawlins Aug 3, 2016

TedSander commented Aug 3, 2016

srawlins commented Aug 3, 2016

TedSander commented Aug 3, 2016

Add strong mode-compliant 'typed' API #26

Add strong mode-compliant 'typed' API #26

Conversation

srawlins commented Jun 16, 2016 • edited Loading

Alternatives

dskloetg commented Jun 21, 2016

srawlins commented Jun 21, 2016

srawlins commented Jun 21, 2016

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

Andersmholmgren commented Jun 23, 2016

dskloetg commented Jun 23, 2016

srawlins commented Jul 7, 2016

srawlins commented Jul 7, 2016

dskloetg commented Jul 11, 2016

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

srawlins commented Aug 2, 2016

dskloetg commented Aug 2, 2016

srawlins commented Aug 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedSander Aug 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedSander commented Aug 3, 2016

srawlins commented Aug 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedSander commented Aug 3, 2016

srawlins commented Aug 3, 2016

TedSander commented Aug 3, 2016

srawlins commented Jun 16, 2016 •

edited

Loading

TedSander Aug 3, 2016 •

edited

Loading