(Request) package:meta - @doNotMock

[matanlurey]

Forked from #27372.

@doNotMock could be placed on a class definition to warn if noSuchMethod is used:

@doNotMock
class Point {
  final int x;
  final int y;

  Point(this.x, this.y);
}

So, use of mockito (or similar), would receive a warning:

import 'package:mockito/mockito.dart';

// Warns: 'Mock implements "noSuchMethod", but Point is annotated with "doNotMock".
// Classes annotated with "doNotMock" are meant to be used in their concrete form in
// testing.
class MockPoint extends Mock implements Point {}

cc @yjbanov @bwilkerson @eernstg @lrhn

[lrhn]

I have to ask: Why should you care what people do in testing?
The author of Point may know that you don't need to mock Point, but most users should be able to realize that too from looking at the API. If they decide to mock it anyway, there might be a reason that neither of us could imagine, so why get in their way?

Ofcourse it's only a warning, so you can ignore it if you know what you are doing. At that point, it's really just a way to give a hint that the class is so simple that you probably don't need to mock it.

Has there been cases where someone mocks a class for testing, and that has caused problems?
(In other words: What problem is this annotation intended to solve?)

[eernstg]

My first reaction is to agree with Lasse with respect to the motivation for having such a mechanism --- I'm not yet convinced that it would help anyone very much. But it would be great to have some more details about the concrete context where this idea arose, it might be useful and important in ways that we just haven't discovered yet.

[matanlurey]

Sure, it is to prevent the following, which is real code written by Googlers:

class MockDateTime extends Mock implements DateTime {}

main() {
  var date = new MockDateTime();
  when(date.inSeconds).thenReturn(1000);
  // ...
}

... instead of just writing:

main() {
  var date = new DateTime(seconds: 1000 /* Or whatever the API is. */);
  // ...
}

There are some simple examples of classes that should not be mocked:

1. Duration
2. DateTime
3. List

Another common one is RPC objects, for example:

class UserProtoRequest {
  String firstName;
  String emailAddress;
}

This shouldn't be "mocked". You can "mock" one by using "new":

new UserProtoRequest()..firstName = 'Mock'..emailAddress = '[email protected]';

But users don't know that and use mockito/noSuchMethod.

[matanlurey]

And importantly, this would be an optional hint/warning, not an error.

[lrhn]

Those particular classes (Duration, DateTime, List) won't be annotated anyway. They're in the SDK, and can't see the meta package.

For other classes, I still don't see a problem with using mocks. It's unnecessary, but its just a test, and we have reviewers to point that out. And then, once in a blue moon, you actually do want to mock the interface, perhaps to test what happens if it breaks expected behavior (a DateTime that changes time zone spontaneously or some other obscure behavior).

Requiring people to add the @doNotMock annotation to their classes is unreasonable. It's not something you should have to think about when you design a class - you should build the class to solve its problem, then users should do whatever they need to solve their problems. Making it the class writer's responsibility to predict whether a user might mock, and whether it's reasonable, is asking the wrong person to do the job.

If you think that someone is mocking DateTime unnecessarily, that's the thing we need to fix and we should fix it by telling them why some classes don't need to be mocked, and how to recognize such classes, so that they can tell the difference in the future. We should not try to predict which classes someone might try to mock, that's a loosing game.
(I guess I have an underlying design principle for language features which shows here: If you have to predict the future to use a feature correctly, it's a bad feature and it's probably trying to make the wrong person make a decision. That's why people always adds getter and setter methods in Java, it means you don't have to predict whether the field needs to be virtual or not, and it's why we have virtual fields in Dart.)

I'd be much more encouraging about an analyzer feature that recognize classes that don't need to be mocked (typically simple to build and self-contained - so something where you can initialize every field and where no method accesses non-object state). If you then try to mock such a class, we could suggest that you just use the constructor to create one.

[yjbanov]

@lrhn, why not allow mocking int, double, bool and String then?

[lrhn]

The reason to not mock int, double and String is that they are system integration types used to communicate with the underlying platform. If I need to print a String, I won't know how to do that if it isn't a system String. Calling toString won't help me if it keeps returning some other kind of string. (Well, technically, we could use codeUnitAt to deconstruct the string and print the underlying characters, like Java's String implements CharSequence, but then we still need to know what an int is).

So, basically, it's because it can't be turtles all the way down.

We could allow mocking integers, and just throw if a non-system-integer reaches a place where we need a "real" integer, but that's not very user friendly, and it was decided that reimplementing a system integration type is probably an error. It's true that you also never need to, because you can always create instance that you need, which is what the @doNotMock annotation would represent.

The reason to not mock bool is probably just performance., or consistency. It would require every branch to handle a value that is neither true nor false - but it already has to handle null, and we survived that, so it's not really a big issue. So, probably mainly consistency, and "you won't need it".
We also say that you can't implement an enum, for the same reasons as bool - it makes it possible to know that switch cases are complete.

[yjbanov]

@lrhn Thanks for the explanation. I agree with the reasoning of int/String/etc being "system integration types" and as such should not be polymorphic. However, I think that the definition of what the system is is too limited for the variety of use-cases and platforms Dart is aimed at.

When I optimize a piece of code for performance, parts of the system I consider are the CPU, memory and my knowledge of how the compiler compiles my code. When I define a class and intend it to have specific performance characteristics I rely on how the system works. As an example, I might rely on static method dispatch or inlining based on the knowledge that the compiler can optimize my code if types are monomorphic. Or I might rely on data locality knowing how my objects are laid out in memory, or whether they are allocated on the stack or the heap. If the user of my class is given the ability to unknowingly change my assumptions about how the system works, I cannot guarantee the performance characteristics that I promised to the user.

The web is in a similar situation. Perhaps I'm defining a Dart class whose instances are backed by JS objects. I will be relying on the properties of dart2js and package:js, and I'll be putting limitations on the class that are outside the scope of the standard libraries.

Having said that, I think a @doNotMock annotation in package:meta is too weak to solve the problem. I think we need stronger language primitives, something along the lines of value types/structs, sealed classes, and/or non-nullability.

[matanlurey]

I'd prefer sealed classes to @doNotMock, FWIW.

[matanlurey]

Closing in favor of @sealed at this point in time.