Repository: mockito/mockito
Issue: MockUtil.isMock() no longer checks null
Labels: bug

In version 1.9.5, MockUtil.isMock() is defined as:

```
return instance != null && isMockitoMock(instance);
```

In v1.10.19 and HEAD, the `instance != null` check is gone.

This method is called by MockScanner when injecting mock instances on fields, where field values can be null.

Is this a bug? Or it's up to MockMaker.getHandler() to check for null?

If it's a bug, I can fix it. But then we'll need to wait for v2.0 before upgrading Mockito for the company's code base. 

If it's up to the mock maker, then I guess we need to fix Google's DexmakerMockMaker to add a line of `if (mock == null) return null;`


Issue comments:
thanks for reporting this one !

ByteBuddyMockMaker.getHandler(Object mock) method checks for null using instanceof operator
`if (!(mock instanceof MockAccess)) {
    return null;
}`

I presume it is not a bug. 

> I presume it is not a bug.

Nope

How about we document the change in the behavior. Javadoc ok?

On Sun, Jul 19, 2015 at 11:28 PM, Brice Dutheil notifications@github.com
wrote:

> I presume it is not a bug.
> 
> Nope
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/mockito/mockito/issues/243#issuecomment-122708060.

## 

Szczepan Faber
Founder mockito.org; Core dev gradle.org
tweets as @szczepiq; blogs at blog.mockito.org

`MockUtil` is an internal class, I don't think we have to document that one. But I believe we should fix the code there.

The public API (or, SPI) is MockMaker.getHandler(Object mock).

The current JavaDoc doesn't mention whether null could be passed in. Although in v1.9.5 it was guaranteed that nulls won't be passed in.

If v1.10.19 drops that guarantee, Android's [DexMakerMockMaker](https://android.googlesource.com/platform/external/dexmaker/+/34f760b98a72000325cc83c88f41de011471432e/src/mockito/java/com/google/dexmaker/mockito/DexmakerMockMaker.java) is broken because it doesn't make the effort to check for null.

I personally think it's pointless for Mockito to ever call MockMaker.getHandler(null) because nothing but null would seem reasonable to be returned anyway, so why risk the NPE?

But if this change in the implicit contract needs to happen, the document change is at MockMaker.getHandler(). Something like "null could be passed in and implementations should return null in that case" would be clear enough.

yes agreed with @fluentfuture 

While working on #277, I fixed the null-check in the MockScanner (as tests were failing if you would remove this check). The check can be [found here](https://github.com/mockito/mockito/pull/277/files#diff-5f69ba708be52e3b8f1216aff3582f4aR34)


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/util/MockUtil.java b/src/main/java/org/mockito/internal/util/MockUtil.java
index 96b47a73bf..3ea20cded9 100644
--- a/src/main/java/org/mockito/internal/util/MockUtil.java
+++ b/src/main/java/org/mockito/internal/util/MockUtil.java
@@ -70,7 +70,7 @@ public boolean isSpy(Object mock) {
     }
 
     private <T> boolean isMockitoMock(T mock) {
-        return mockMaker.getHandler(mock) != null;
+        return mock != null && mockMaker.getHandler(mock) != null;
     }
 
     public MockName getMockName(Object mock) {


Test patch: