Suggestions for non-nullable api #305
Comments
|
Thanks for the report, @KalilDev , can you say what version of mockito you are using? |
|
I'm using |
|
@KalilDev at first I did not understand your first request, which is why I asked for your version; it's a total red herring 😆 . It is really good to get this real-world feedback on features like throw-on-missing-stub. I get the idea that you should provide stubs for methods that code you care about in tests calls, but if code somewhere calls I'm curious though, I am surprised that mocks in tests have these methods called on them a lot, causing you to have to write a bunch of stubs. Can you give examples about the |
Yeah this came up when designing the code-gen idea. "What about a whole new API?" I think it might be possible to design a sibling API which is completely separate from the current |
Yes, completely understandable |
|
Something else that may need a different default behavior are Calling a void function on a mocked class just throws this: And calling a setter on a mocked class throws this: with this being the mocked class: class Foo {
String content = 'content';
void doSomething() {}
}and this the test: @GenerateMocks([Foo])
void main() {
test('call void function', () {
final foo = MockFoo();
foo.doSomething();
});
test('call a setter', () {
final foo = MockFoo();
// doesn't make a difference whether it's a field or an actual setter
foo.content = '';
});
}It works when using |
|
Dennis, this happens because you did not stub the setter and function in an null-safe library. The mockito codegen automatically opts in throwing on missing stub, and in Using |
|
I thought a long while about this, and with that, i think that:
I think i will implement an prototype of this api and check how it feels like. |
|
Maybe I'm missing something, but how would I go about stubbing a void function and a setter? when(foo.doSomething()).thenAnswer((_) {});
when(foo.content = '').thenAnswer((_) => '');works, but is that really what I'm expected to with nullsafety? I'd expect void functions and setters to be pretty much noops unless you do something with Concerning stubbing |
Here, i made the prototype and created a test case to demonstrate the usability: KalilDev@4b8423d |
|
re: "primitive" return values: instead of throwing and acting like a Fake for a Mock, can the Mock just return a standard/sane default (0, 0.0, false, ''). if i had to make a strong argument for just one of these, i would strongly want bool to just always return false. bools are important b/c dart compliation/transpilation barfs when evaluating boolean expressions in branches where the mock returns null (not of type bool in a boolean expression).
|
…Future<void>. This requires a breaking change, changing Mock.noSuchMethod's optional second parameter to two optional named parameters. Any code which calls noSuchMethod with a second positional parameter, or which overrides Mock.noSuchMethod, will need to change. Users have found the throw-if-unstubbed setting to be too constrained for setters, other methods that return void, and methods which return a (non-nullable) Future<void>. The method calls should be allowed, as stubbing is probably meaningless. Fixes #305 PiperOrigin-RevId: 355438518
…Future<void>. This requires a breaking change, changing Mock.noSuchMethod's optional second parameter to two optional named parameters. Any code which calls noSuchMethod with a second positional parameter, or which overrides Mock.noSuchMethod, will need to change. Users have found the throw-if-unstubbed setting to be too constrained for setters, other methods that return void, and methods which return a (non-nullable) Future<void>. The method calls should be allowed, as stubbing is probably meaningless. Fixes #305 PiperOrigin-RevId: 355438518
|
cee9efd doesn't address primitive bool return values returning nulls in mocks...? |
|
@razamatan That is correct. The motivation that I used for methods that return then a user might want to allow code-under-test to call All that said, I think it does make sense to require a stub for An important consequence of that proposed behavior is that it would lead to very-hard-to-diagnose test bugs. Devs unfamiliar with the default return values (like |
|
but if they want to get breaking exceptions for unimplemented things, a fake is warranted no? why isn't the current stub behavior of always returning null not surprising already given your rationale? to me, deciding to return a reasonable, consistent default for the classic "primitives" seems reasonable and consistent and won't break things beyond what silent null return values currently do.
…________________________________
From: Sam Rawlins <notifications@github.com>
Sent: Wednesday, February 3, 2021 7:37 PM
To: dart-lang/mockito <mockito@noreply.github.com>
Cc: razamatan <razamatan@hotmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [dart-lang/mockito] Suggestions for non-nullable api (#305)
@razamatan<https://github.com/razamatan> That is correct. The motivation that I used for methods that return void (like setters) or Future<void> is that there is sort of nothing to stub. If you have a class:
abstract class C {
void set a(int value);
bool isSomething(int x, int y);
}
then a user might want to allow code-under-test to call c.a = 7, given MockC c, without the system throwing saying "you forgot to stub a=!!" The remedy for that situation is quite strange, because stubbing is largely (but not entirely) about return values, as @denniskaselow<https://github.com/denniskaselow> pointed out. You would tell Mockito to stub a= with a function that ... does nothing.
All that said, I think it does make sense to require a stub for isSomething. If code-under-test calls c.isSomething(1, 2), but you have not declared any stubs that match that call, then who is Mockito to pick a return value (either true or false), which your code-under-test will then use in possibly surprising ways.
An important consequence of that proposed behavior is that it would lead to very-hard-to-diagnose test bugs. Devs unfamiliar with the default return values (like false, 0, '') would really be scratching their heads at the weird return values coming out of their mocks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#305 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAD53T6VXDSG7GM42CVZM3LS5IJAFANCNFSM4VENE6BQ>.
|
We believe it was surprising, which is why
Right, but we think silent null is already bad, and that throwing on missing stubs is better than returning null, or sane values. I think it is perhaps reasonable to support the old "just return something" way in 'null safe' type system for simple types, as you are suggesting, but not with the default generated mocks. I think we can add another flag to the custom mocks. Right now there is |
|
ok. thanks for the clarifications. having a "simple default" option would be nice, too. :) generally, what would be the purpose of Mock over Fake in the direction things are headed? |
While migrating an package, i noticed that the Object props (hashCode, operator==, runtimeType and toString()) were throwing when the dart: libraries used any, because i did not stub them, and stubbing them would be really cumbersome, so i temporarily modified the generated code and after that a lot of tests passed successfully.
For an faster migration on
--no-sound-null-safetyi enabledreturnNullOnMissingStub, then i continued with the migration and after i finished i moved to sound-null-safety, and noticed that a lot ofFuture<void>returns needed to be stubbed, and this resulted in more verbose tests.So, my suggestions for the MockSpec and related codegen are:
stubObjectMethodsbool parameter or andoNotStubList<Symbol> parameter with an list of methods/accessors to not have mockito code generated. Then store the List that should throw an error in case noSuchMethod is called with them, informing the user that such method/accessor should not be used onwhenstatements or that they should have an concrete implementation in case they were used on a real call.returnValidNullableFutureOnMissingStubbool parameter or anreturnFakeOnMissingStubList<Symbol> which will in the first case return an Future.value() on Future<dynamic>, Future<T?> or Future<void>, or in the second case, a more general approach, it will return the Fake value that was used as the second parameter on noSuchMethod, which is always of the correct type, but not usable.Also, now that
any,anyNamed,captureAny,captureAnyNamed,argThatandcaptureThatcan not be used with non-nullable parameters, maybe an option for having the same functionality would be to add an non-fluent api that allows the user to manually create the argument matchers, for example,when(mockObject, #get).matchParams(none).thenReturn(1)would return 1 in case there wasgetcall with 0 positional parameters, andwhen(mockObject, #set).matchParams([anyValue, equals(1)]).thenThrow('invalid')which would throw 'invalid' when trying to call #set and the second param is 1.The text was updated successfully, but these errors were encountered: