Handle new noSuchMethod mechanics

[srawlins]

In Dart 2, it seems likely that NSM will change:

• Currently: missing optional arguments are not present in the Invocation object (in positionalArguments and namedArguments).
• New: missing optional arguments are present, and have the default value specified in the parameter.

See dart-lang/sdk#33031.

Mockito then sees all these null values in the Invocation, and assumes that the user forgot to use foo: typed(any, named: "foo"). It requires each named argument to be given a name, so that Mockito can store them properly. How to handle this proposed change?

CC @matanlurey @TedSander

[srawlins]

Concrete example:

class Cat {
  bool eatFood(String food, {bool hungry}) => true;
}

// Mock class
class MockCat extends Mock implements Cat {}

// mock creation
var cat = new MockCat();

Then stubbing with:

when(cat.eatFood(typed(any))).thenReturn(true);

Mockito yells, "Hey! you apparently wrote an argument matcher for hungry, because I got a null there. Why didn't you name your hungry argument matcher?"

[srawlins]

I don't have any great ideas. I have some... OK ideas.

No change to mockito

We just require users to spell out in every stub, for every named argument, foo: typed(any, named: "foo"). Ouch. Huge breaking change. Herculean effort to fix user code.

Provide a little helper for optional args you don't care about:

when(obj.fn(typed(any), typed(any), foo: typed(any, named: "foo")),
    andAnythingFor: ["bar", "baz"])...

I think this is the same herculean effort...

Resort to surprising results when argument matchers aren't named.

when(obj.fn(typed(any), typed(any), foo: typed(argThat(contains("item 1")))))...

Note the missing name for the foo arg matcher. This would send null to the function, and when Mockito gets a real call to the function, and looks up its stubs, it would never see the argThat(contains("item 1")) (or maybe it would if there was a 3rd, optional positional parameter)... and always/never match. No error would be reported to the user. Mockito would just look way broken.

Since the Invocation would be guaranteed to basically match the shape of the parameters (as opposed to the shape of the arguments today), we could check for some errors, I think. "This only contains 2 positional args, but you passed 3. Did you forget a name?" But then optional and named would be very confusing to users. Is that survivable? I think there are more consequences to error states that I haven't thought of here...

Others? 😄

[srawlins]

I think that 3rd option is probably what we need to do. It will definitely mean a poor user experience for casual Mockito users (as opposed to all of us hardcore Mockito users); Mockito cannot help very well when users don't name their named arguments. Can we write a linter plugin maybe?

[matanlurey]

I think its worth waiting on the resolution of dart-lang/sdk#33031; it's not clear me to the new nSM semantics fix any problem (as you already mentioned), and given that the feature is used infrequently anyway (basically Mockito), I think we could relax the changes...

[TedSander]

Personally I vote that if the dart language team wants to make this change (which I doubt will happen in dart2,) they need to work with us to have a way to support mocking, and they need to do most of the heavy lifting.

For example can we get more signals from nSM or the Invocation? If not who is signing up for a code gen solution? As evidence by the code we have in google3 mocking is vital to the testing ecosystem and it needs first class support.

[eernstg]

Trying to take a look at the situation, I noted that there are 'no issues' from dartanalyzer --preview-dart-2 and no compile-time nor run-time errors from dart --preview-dart-2 with bin/main.dart from the attached example mockito_122.zip.

What does it take to get something like the "Hey! you apparently wrote an argument matcher for hungry, because I got a null there. Why didn't you name your hungry argument matcher?" message mentioned above (given that using the Mockito branch three-oh-beta-compatibility wasn't enough)?

.. if the dart language team wants to make this change ..

Note that this change was accepted by the language team quite a while ago: The relevant feature specification was clarified on Oct 5 2017 to specify this treatment of optional parameters, and it had its status changed from 'under discussion' to 'under implementation' on Dec 1 2017 in this SDK commit.

That said, we must of course make things work and minimize the disruption. I hope we can spot a good solution soon.

[srawlins]

Dang, I thought that was a valid repro. I haven't repro'd myself. Let me check out the SDK, sync to dart-lang/sdk@7d5025e, and try to repro.

[srawlins]

Sorry, the mock method call needs at least one arg matcher, so I've changed that line above to:

when(cat.eatFood(typed(any))).thenReturn(false);

Then running dart --preview-dart-2 prints:

Unhandled exception:
Invalid argument(s): A typed argument was passed in as a named argument named "Symbol("hungry")", but did not pass a value for `named`. Each typed argument that is passed as a named argument needs to specify the `named` argument. For example: `when(obj.fn(x: typed(any, named: "x")))`.
#0      _InvocationForTypedArguments._reconstituteNamedArgs.<anonymous closure> (package:mockito/src/mock.dart:208:11)
#1      __InternalLinkedHashMap&_HashVMBase&MapMixin&_LinkedHashMapMixin.forEach (dart:collection/runtime/libcompact_hash.dart:358:8)
#2      MapView.forEach (dart:collection/maps.dart:340:10)
#3      _InvocationForTypedArguments._reconstituteNamedArgs (package:mockito/src/mock.dart:203:31)
#4      new _InvocationForTypedArguments (package:mockito/src/mock.dart:176:26)
#5      _useTypedInvocationIfSet (package:mockito/src/mock.dart:145:22)
#6      Mock.noSuchMethod (package:mockito/src/mock.dart:102:18)
#7      MockCat.eatFood (file:///Users/srawlins/Downloads/mockito_122/bin/main.dart:5:8)
#8      main (file:///Users/srawlins/Downloads/mockito_122/bin/main.dart:16:12)
#9      _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:279:19)
#10     _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:165:12)

[eernstg]

Thanks! I'll take another look later today.

[lrhn]

As far as I understand, you can already pass valid parameter values instead of registering a matcher, so you can do:

when(mock.foo(x: 42)).thenReturn(42);

Is this correct?
If so, any non-null default value should work without a hitch since you get the same value in the when call and the actual call, and they are identical and therefore equal (well, unless you use NaN as default value, so don't do that!)

As I understand it, you use a null argument in the when call as a marker that it corresponds to a recently registered matcher. You match the matchers to the parameters by going through actual positional arguments and consuming a matcher for each null, then you go through the named arguments and expect a named matcher for each null value there. (I assume that means you have to use an isNull matcher to match the actual null value).

If you always get the full argument list for both positional and named arguments, then you know the signature of the function immediately. If there are any named arguments, you can detect a missing named matcher simply by there being more unnamed matchers than null-valued positional arguments.

So if someone does:

when(obj.fn(typed(any), typed(any), foo: typed(argThat(contains("item 1")))))..

then you register three unnamed matchers, but the call to obj.fn would only and always have two positional arguments with null values. That tells you that the final typed matcher should probably have been named - you don't know which name if there is more than one named parameter, so you can't error correct, but you can error detect immediately.

In fact, this should work even now - if you get more unnamed matchers than null-valued positional parameters, there is a mistake. The only difference is that with full parameter invocations, there might be more null-valued positional arguments than matchers if there are omitted optional positional parameters, but in that case there will never be a named argument, so you can use that to detect the case.

(This would stop working if we get both optional positional and named parameters on the same function, then you can't know if an optional positional parameter with default value null was omitted and a named parameter matcher forgot the name, but a mockito aware lint might be able to catch that instead).

[srawlins]

As far as I understand, you can already pass valid parameter values instead of registering a matcher ... Is this correct?

Yes.

This would stop working if we get both optional positional and named parameters on the same function...

Oh, I forgot that we don't have that yet. I think you're right, we should be able to generate errors.

The remaining issue is a breaking change in the user expectations of the presence of arguments. Let's look at another example:

class A {
  int fn(one, [two: true]) => 0
}
class MockA extends Mock implements A {}

main() {
  var a = new MockA();
  when(a.fn(typed(any))).thenReturn(7);
  when(a.fn(typed(any), true)).thenReturn(8);
  a.fn(1); // Dart 1: returns 7. Dart 2 w/ NSM forwarding: returns 8.
}

(Same for named args.)

This situation is probably not common... we'll have to look internally to see what kind of a breaking change this would be.

[eernstg]

With the new example I could reproduce the issue, and it seems that some adjustments in/around _reconstituteNamedArgs should be enough to make it work (such that clients wouldn't have to start specifying all optional arguments explicitly).

The issue with the expectations about arguments being not present or present (with the value which is also the default) should not be useful for a mock object, because the corresponding non-mock object would not be able to make that distinction anyway (except if it would somehow magically be able to use a noSuchMethod with Dart 1 semantics ;-).

So it shouldn't be so bad after all, right?

[srawlins]

So it shouldn't be so bad after all, right?

We'll have to see.

[lrhn]

Let's look at another example:

when(a.fn(typed(any))).thenReturn(7);
when(a.fn(typed(any), true)).thenReturn(8);

With full parameter forwarding, those two calls would have the same matchers (both would pass true as second argument), so you are assigning two behaviors to the same trigger. I guess one of them will trigger first, so yes, a.fn(1) and a.fn(1, true) will both return the same thing (whether 7 or 8).

However, in this example you have behavior that cannot be implemented in plain Dart, so it's probably not the expected behavior of the actual class.
That makes it unlikely that testing will depend on such behavior. In fact, any black-box testing should be set up to behave the same whether you pass true or nothing, otherwise the test is fragile against seemingly semantics-preserving changes of the tested code.

[srawlins]

I'm not advocating that the old behavior is better. It's just different.

That makes it unlikely that testing will depend on such behavior.

We'll have to see.