Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add annotation to mark a type as DoNotMock #1833

Merged
merged 11 commits into from Nov 19, 2021
Merged

Add annotation to mark a type as DoNotMock #1833

merged 11 commits into from Nov 19, 2021

Conversation

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Nov 28, 2019

Mocking types that users not own 1 or are severely complicating test
logic 2 leads to brittle or wrong tests. In particular, the
StackOverflow answer is wrong, as the contract of java.util.Map is
violated. When a new key is added to the Map, the stubbed return would be wrong.
In Google we have used the DoNotMock annotation via ErrorProne 3
to annotate these types, as well as an internal list of types that can't
be mocked (this includes several java.util types). We are using a custom
Mockmaker to enforce this on run-time.

Based on our successful experience with DoNotMock (we have seen a large
reduction in bad/broken tests for types involved), we are proposing to
open source this into Mockito itself.

The DoNotMock annotation can be added to any type, e.g. classes and
interfaces. If, in the type hierarchy of the class-to-be-mocked, there
is a type that is annotated with DoNotMock, Mockito will throw a
DoNotMockException.

This would help preventing issues such as #1827 and #1734 which is
in-line with the guidance on our wiki 1. A follow-up change would
allow us to define external types (like the java.util types) that can't
be mocked. (We can't add the annotation to the types, as they live in the
JDK instead.)

Mocking types that users not own [1] or are severely complicating test
logic [2] leads to brittle or wrong tests. In particular, the
StackOverflow answer is wrong, as the contract of java.util.Map is
violated. When a new key is added to the Map, the stubbed return would be wrong.
In Google we have used the DoNotMock annotation via ErrorProne [3]
to annotate these types, as well as an internal list of types that can't
be mocked (this includes several java.util types). We are using a custom
Mockmaker to enforce this on run-time.

Based on our successful experience with DoNotMock (we have seen a large
reduction in bad/broken tests for types involved), we are proposing to
open source this into Mockito itself.

The DoNotMock annotation can be added to any type, e.g. classes and
interfaces. If, in the type hierarchy of the class-to-be-mocked, there
is a type that is annotated with DoNotMock, Mockito will throw a
DoNotMockException.

This would help preventing issues such as #1827 and #1734 which is
in-line with the guidance on our wiki [1]. A follow-up change would
allow us to define external types (like the java.util types) that can't
be mocked. (We can't add the annotation to the types, as they live in the
JDK instead.)

[1]: https://github.com/mockito/mockito/wiki/How-to-write-good-tests#dont-mock-a-type-you-dont-own
[2]: https://stackoverflow.com/a/15820143
[3]: https://errorprone.info/api/latest/com/google/errorprone/annotations/DoNotMock.html
@TimvdLippe TimvdLippe requested review from mockitoguy, szpak, bric3 and raphw Nov 28, 2019
@codecov-io
Copy link

@codecov-io codecov-io commented Nov 28, 2019

Codecov Report

Merging #1833 into release/3.x will increase coverage by 0.38%.
The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff                @@
##             release/3.x   #1833      +/-   ##
================================================
+ Coverage          86.71%   87.1%   +0.38%     
- Complexity          2492    2610     +118     
================================================
  Files                311     316       +5     
  Lines               6551    6793     +242     
  Branches             822     848      +26     
================================================
+ Hits                5681    5917     +236     
- Misses               674     675       +1     
- Partials             196     201       +5
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/org/mockito/internal/MockitoCore.java 98.43% <100%> (+0.28%) 48 <8> (+8) ⬆️
...internal/configuration/plugins/PluginRegistry.java 100% <100%> (ø) 8 <1> (+1) ⬆️
...ockito/internal/configuration/plugins/Plugins.java 88.88% <100%> (+1.38%) 8 <1> (+1) ⬆️
...ockito/exceptions/misusing/DoNotMockException.java 100% <100%> (ø) 1 <1> (?)
...ternal/configuration/DefaultDoNotMockEnforcer.java 100% <100%> (ø) 5 <5> (?)
...l/configuration/plugins/DefaultMockitoPlugins.java 79.16% <100%> (+0.9%) 7 <0> (ø) ⬇️
...to/internal/stubbing/StubbedInvocationMatcher.java 100% <0%> (ø) 13% <0%> (+4%) ⬆️
...ugpatterns/AbstractMockitoAnyForPrimitiveType.java 72.91% <0%> (ø) 7% <0%> (?)
.../bugpatterns/MockitoAnyClassWithPrimitiveType.java 100% <0%> (ø) 4% <0%> (?)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc6eadc...aa93d90. Read the comment docs.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Nov 29, 2019

Great submission, thank you!

we are proposing to open source this into Mockito itself.

Fantastic idea. THANK you!

We are using a custom Mockmaker to enforce this on run-time.

Wow! Interesting use of this API. Can you enumerate other interesting use cases Google has extended Mockito behavior via our public API? I'd love us to offer first class support for such features.

A follow-up change would
allow us to define external types (like the java.util types) that can't
be mocked.

+1

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Nov 29, 2019

Request: can we add a new method to override this just in case? Here's a use case: let's say someone incorrectly sets DoNotMock annotation on a type and ships the code in a library. The consumers of the library don't have a good way to remove DoNotMock annotation if they don't own the library code. Thoughts?

Example API:

mock(Foo.class, withSettings().overrideDoNotMock())

We can this API later, no need to include this in this PR.

Copy link
Member

@mockitoguy mockitoguy left a comment

I quickly went through and wrote some feedback. I will review fully later today and approve it. I love this change. It will help our users with cleaner code.

mockTypeToCheck = mockTypeToCheck.getSuperclass();
}

if (unmockableTypes.stream().anyMatch(clazz -> clazz.isAnnotationPresent(DoNotMock.class))) {
Copy link
Member

@mockitoguy mockitoguy Nov 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to honor this specific annotation (Mockito API) or any annotation with name "DoNotMock".

The latter seems more useful. For example: a library owner can mark types with any "DoNotMock" annotation without adding compile dependency on mockito-core.

Copy link
Contributor Author

@TimvdLippe TimvdLippe Nov 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep this for now and open it up later to match on name. If we would implement that now, I would break all users of ErrorProne that rely on @DoNotMock but have grandfathered mock violations.

If we can gather feedback first, we can see adoption patterns and change it to match on the name later. WDYT?

Copy link
Member

@mockitoguy mockitoguy Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a happy medium so that we don't need to revisit this in the future (and cause breaking changes :) - we can match for "org.mockito.DoNotMock" by String. This should be backwards compatible for G. and other companies / projects. Thoughts?

Copy link
Contributor Author

@TimvdLippe TimvdLippe Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

import org.mockito.MockitoAnnotations;
import org.mockito.exceptions.misusing.DoNotMockException;

public class DoNotMockTest {
Copy link
Member

@mockitoguy mockitoguy Nov 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test coverage.

@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Nov 29, 2019

Wow! Interesting use of this API. Can you enumerate other interesting use cases Google has extended Mockito behavior via our public API? I'd love us to offer first class support for such features.

Yes I am auditing/discovering usages of Mockito API and reviewing them whether they are worth keeping (and open sourcing) or should be changed. This is one of many 😄

Request: can we add a new method to override this just in case? Here's a use case: let's say someone incorrectly sets DoNotMock annotation on a type and ships the code in a library. The consumers of the library don't have a good way to remove DoNotMock annotation if they don't own the library code. Thoughts?

Yes we already have the concept of "grandfathered" mock violations. E.g. all existing mocks of a type that you want to add @DoNotMock to will not immediately break. This allows us to put a hold on new violations while users can (over time) remove violations. I am still trying to come up with a scalable solution for implementing that logic, which will solve your use case as well.

Copy link
Member

@mockitoguy mockitoguy left a comment

Please consider my feedback, especially about the name matching for fully qualified org.mockito.DoNotMock (I am really keen on this - name matching is great for library authors!).

creationSettings.getExtraInterfaces());

Class<?> mockTypeToCheck = typeToMock;
while (mockTypeToCheck != null && mockTypeToCheck != Object.class) {
Copy link
Member

@mockitoguy mockitoguy Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played with the code and noticed that "mockTypeToCheck != null" part of this statement is either: a) not needed or b) not covered with tests.

Do we need this part?

Copy link
Contributor Author

@TimvdLippe TimvdLippe Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is covered by mocking an interface that directly has the DoNotMock annotation. I added a test to cover this case.

@raphw
Copy link
Member

@raphw raphw commented Dec 2, 2019

Really interesting feature, also with regards to returning mocks from methods, I think. But very much +1 on the override functionality. There is always a "but" that might need consideration.

TimvdLippe added 2 commits Dec 2, 2019
This introduces the DoNotMockEnforcer interface which users can override
to implement their special handling of types annotated with DoNotMock.
@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Dec 2, 2019

It is now possible to provide a custom DoNotMockEnforcer where you have full control over what types can be mocked. See the extTest test for an example.

Given the positive response, I will now spend some time integrating the changes in Google and see if it all builds. Given that is all positive and you have no more review comments, I will merge this PR.

This name is clearer for implementers.
@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Dec 3, 2019

It is now possible to provide a custom DoNotMockEnforcer

Curious, are you going to use this new plugin at G? If we don't have a user, I suggest we avoid complexity (it can be added later if needed).

Thank you for implementing name matching for the annotation!!!

@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Dec 3, 2019

It is now possible to provide a custom DoNotMockEnforcer

Curious, are you going to use this new plugin at G? If we don't have a user, I suggest we avoid complexity (it can be added later if needed).

Yes we will immediately use it. We have some additional classes that we are annotating as well as our "grandfathering violations" system.

Note that there is no other nice way of overriding the method as requested in #1833 (comment) (a method call per mock creation would mean that we would need to change every existing mock in our codebase to include it, which is very cumbersome)

Thank you for implementing name matching for the annotation!!!

Glad you like it 😄

Copy link
Member

@mockitoguy mockitoguy left a comment

Really nice work with this feature. Thanks!

* @param type The type to check
* @return Optional message if this type can not be mocked, or an empty optional if type can be mocked
*/
Optional<String> checkTypeForDoNotMockViolation(Class<?> type);
Copy link
Member

@mockitoguy mockitoguy Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you prefer the enforcer to return the message instead of just throwing?

Works for me. Just checking.

Copy link
Contributor Author

@TimvdLippe TimvdLippe Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt like this was the nicer way of handling state, since we have to stop type traversal. I will mark it as Incubating and might change later, depending on how well it integrates with our internal system.

import java.lang.annotation.Target;

/**
* Test to make sure that we are matching on name rather than only {@link org.mockito.DoNotMock}
Copy link
Member

@mockitoguy mockitoguy Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice coverage

public void thrown_exception_includes_special_non_mockable_reason() {
assertThatThrownBy(() -> {
NotMockableWithReason notMockable = mock(NotMockableWithReason.class);
}).isInstanceOf(DoNotMockException.class).hasMessageContaining("Special reason");
Copy link
Member

@mockitoguy mockitoguy Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice coverage, thx!

annotation -> annotation.annotationType().getName().endsWith("org.mockito.DoNotMock"))
.findFirst()
.map(annotation -> {
String exceptionMessage = type + " is annotated with @DoNoMock and can't be mocked.";
Copy link
Member

@mockitoguy mockitoguy Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to say "is annotated with org.mockito.@DoNotMock"?

Copy link
Contributor Author

@TimvdLippe TimvdLippe Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's better :)

@@ -50,6 +56,9 @@
@SuppressWarnings("unchecked")
public class MockitoCore {

private static final DoNotMockEnforcer DO_NOT_MOCK_ENFORCER = Plugins.getDoNotMockEnforcer();
private static final Set<Class<?>> SAFE_DONOTMOCK_ENFORCED_CLASSES = Collections.synchronizedSet(new HashSet<>());
Copy link
Member

@mockitoguy mockitoguy Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest "MOCKABLE_CLASSES"

Copy link
Contributor Author

@TimvdLippe TimvdLippe Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

checkDoNotMockAnnotationForType(type.getSuperclass());
Arrays.stream(type.getInterfaces()).forEach(MockitoCore::checkDoNotMockAnnotationForType);

SAFE_DONOTMOCK_ENFORCED_CLASSES.add(type);
Copy link
Member

@mockitoguy mockitoguy Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel uneasy about this perf tweak. Do we need it? Does it really make things faster? Static state that keeps growing with every mocked type may bite us later.

Did you find this perf tweak useful at G?

Copy link
Contributor Author

@TimvdLippe TimvdLippe Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure this is optimization is necessary as we can have large type hierarchies. I have asked internally for more background, but I expect this optimization to be necessary to not have a large performance impact for existing applications.

Copy link
Member

@mockitoguy mockitoguy Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding perf tweaks "on hunch", without evidence is not good for the codebase (growing complexity :-). I don't mind this change but I'd love to get your assurance that this is useful.

You have the approval so ship this change at will!

Copy link
Contributor Author

@TimvdLippe TimvdLippe Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will double check before merging. I haven't had time during the Christmas vacation to do this work. I probably get to this late next week.

They won't compile for Android users
@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Jan 28, 2020

I have done an initial pass at integrating this internally with good results! We are doing additional verification to confirm everything is working as expected, but thus far it is working out.

@grimreaper
Copy link
Contributor

@grimreaper grimreaper commented Apr 12, 2020

Hey, this is a fantastic feature and I'd love for this to make it in. Can I gently nudge the and reviewers to finish this up?

@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Apr 12, 2020

I still have to integrate this in the Google monorepository and figure out whether it is working correctly. Sadly I haven't had the time to do so.

@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Dec 9, 2020

#2026 is another instance where we could benefit from DoNotMock to prevent users from running into problematic situations.

@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Nov 3, 2021

There are a fair few issues in our backlog about mocking classes like File, Method and Map, all of which cause undefined behavior in Mockito internals. Therefore, I think it is worth reviving this PR and see if we can land this to avoid people running into this problem. It would be a breaking change though, so we probably need an incubation timeline to get people to test it out.

bric3
bric3 approved these changes Nov 4, 2021
Copy link
Contributor

@bric3 bric3 left a comment

This is a nice feature in my opinion. And it can really help improve the quality of large code base.

About third party types, could it make sense to have a programmatic way to fill the non-mockable types in the default plugin implementation.

src/main/java/org/mockito/DoNotMock.java Outdated Show resolved Hide resolved
src/main/java/org/mockito/DoNotMock.java Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Nov 19, 2021

Codecov Report

Merging #1833 (aa93d90) into main (a58dcc5) will increase coverage by 0.64%.
The diff coverage is 100.00%.

Current head aa93d90 differs from pull request most recent head 498f2b7. Consider uploading reports for the commit 498f2b7 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1833      +/-   ##
============================================
+ Coverage     86.46%   87.10%   +0.64%     
+ Complexity     2763     2610     -153     
============================================
  Files           318      316       -2     
  Lines          8304     6793    -1511     
  Branches       1016      848     -168     
============================================
- Hits           7180     5917    -1263     
+ Misses          852      675     -177     
+ Partials        272      201      -71     
Impacted Files Coverage Δ
...ockito/exceptions/misusing/DoNotMockException.java 100.00% <100.00%> (ø)
...rc/main/java/org/mockito/internal/MockitoCore.java 98.43% <100.00%> (+5.15%) ⬆️
...ternal/configuration/DefaultDoNotMockEnforcer.java 100.00% <100.00%> (ø)
...l/configuration/plugins/DefaultMockitoPlugins.java 79.16% <100.00%> (-10.49%) ⬇️
...internal/configuration/plugins/PluginRegistry.java 100.00% <100.00%> (ø)
...ockito/internal/configuration/plugins/Plugins.java 88.88% <100.00%> (-11.12%) ⬇️
...ito/internal/stubbing/UnusedStubbingReporting.java 50.00% <0.00%> (-50.00%) ⬇️
...internal/hamcrest/MatcherGenericTypeExtractor.java 50.00% <0.00%> (-50.00%) ⬇️
...xceptions/base/MockitoInitializationException.java 0.00% <0.00%> (-50.00%) ⬇️
src/main/java/org/mockito/junit/MockitoJUnit.java 66.66% <0.00%> (-33.34%) ⬇️
... and 276 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d373357...498f2b7. Read the comment docs.

@TimvdLippe TimvdLippe merged commit ebc1685 into main Nov 19, 2021
7 checks passed
@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Nov 19, 2021

Thanks everyone for your input and feedback! I will be publishing Mockito 4.1.0 that includes this feature 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants