Repository: mockito/mockito
Issue: Add default answers for java.util.Optional{Double,Int,Long} and java.util.stream.{Double,Int,Long}Stream

A natural generalization of #191 and #429 is to add default answers for the primitive `Optional` and `Stream` variants as well. I'm working on a PR proposal; will file soonish.

Issue comments:
When mockito runs on Java 8 it already provide default answers for `Optional` / `Stream`
I tested that before filing this issue; that's true for `Optional`, but not for `OptionalInt` and friends.
Oh ok I read too fast the issue description. Sorry ;)

looking forward the PR.
No worries :). PR filed: #782.
NB: I opened this ticket before opening the PR only because the contribution guidelines [state](https://github.com/mockito/mockito/blob/release/2.x/.github/CONTRIBUTING.md) that at least one commit must be of the form `Fixes #id :`. But it seems a number of PRs (like the aforementioned #429) don't follow this guideline. Also, the guidelines indicate there should be a colon after the issue ID, but that rule seems to be followed "almost never". So what's your current view on this?
@Stephan202 Sorry for the late reply, I'm in the middle of painful refactoring at work.

Well this generally apply when there's a separate issue either because it's the same person, or because there's delay between the issue and the pull request, but you can propose a pull request that fixes an issue, that's ok, just as long as you describe properly the problem or motivate properly the change ;)

Pull request review comments:
It just asks to simplify it with parameterized tests, e.g. with https://github.com/Pragmatists/JUnitParams/wiki/Quickstart . @Stephan202, would you dare to do it? ;)
@szpak: I'm fine opening an extra PR, but I think the Mockito team should first decide on which parameterized test framework to use. The one you linked is an option, but JUnit also comes with built-in support for [`@RunWith(Theories.class)`](https://github.com/junit-team/junit4/wiki/Theories) and [`@RunWith(Parameterized.class)`](https://github.com/junit-team/junit4/wiki/parameterized-tests). There may be others, and it seems right now none of these approaches are used.

Alternatively, the test duplication can be avoided as follows:
```diff
diff --git a/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValuesTest.java b/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValuesTest.java
index 480a9a1..ab9aad9 100644
--- a/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValuesTest.java
+++ b/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValuesTest.java
@@ -90,70 +90,32 @@ public class ReturnsEmptyValuesTest extends TestBase {
 
     @Test
     public void should_return_empty_Optional() throws Exception {
-        Class<?> streamType = getClassOrSkipTest("java.util.stream.Stream");
-
-        //given
-        Object stream = mock(streamType);
-        Object optional = streamType.getMethod("findAny").invoke(stream);
-        assertNotNull(optional);
-        assertFalse((Boolean) Class.forName("java.util.Optional").getMethod("isPresent").invoke(optional));
-
-        Invocation findAny = this.getLastInvocation();
-
-        //when
-        Object result = values.answer(findAny);
-
-        //then
-        assertEquals(optional, result);
+        verify_empty_Optional_is_returned("java.util.stream.Stream", "java.util.Optional");
     }
 
     @Test
     public void should_return_empty_OptionalDouble() throws Exception {
-        Class<?> streamType = getClassOrSkipTest("java.util.stream.DoubleStream");
-
-        //given
-        Object stream = mock(streamType);
-        Object optional = streamType.getMethod("findAny").invoke(stream);
-        assertNotNull(optional);
-        assertFalse((Boolean) Class.forName("java.util.OptionalDouble").getMethod("isPresent").invoke(optional));
-
-        Invocation findAny = this.getLastInvocation();
-
-        //when
-        Object result = values.answer(findAny);
-
-        //then
-        assertEquals(optional, result);
+        verify_empty_Optional_is_returned("java.util.stream.DoubleStream", "java.util.OptionalDouble");
     }
 
     @Test
     public void should_return_empty_OptionalInt() throws Exception {
-        Class<?> streamType = getClassOrSkipTest("java.util.stream.IntStream");
-
-        //given
-        Object stream = mock(streamType);
-        Object optional = streamType.getMethod("findAny").invoke(stream);
-        assertNotNull(optional);
-        assertFalse((Boolean) Class.forName("java.util.OptionalInt").getMethod("isPresent").invoke(optional));
-
-        Invocation findAny = this.getLastInvocation();
-
-        //when
-        Object result = values.answer(findAny);
-
-        //then
-        assertEquals(optional, result);
+        verify_empty_Optional_is_returned("java.util.stream.IntStream", "java.util.OptionalInt");
     }
 
     @Test
     public void should_return_empty_OptionalLong() throws Exception {
-        Class<?> streamType = getClassOrSkipTest("java.util.stream.LongStream");
+        verify_empty_Optional_is_returned("java.util.stream.LongStream", "java.util.OptionalLong");
+    }
+
+    private void verify_empty_Optional_is_returned(String streamFqcn, String optionalFqcn) throws Exception {
+        Class<?> streamType = getClassOrSkipTest(streamFqcn);
 
         //given
         Object stream = mock(streamType);
         Object optional = streamType.getMethod("findAny").invoke(stream);
         assertNotNull(optional);
-        assertFalse((Boolean) Class.forName("java.util.OptionalLong").getMethod("isPresent").invoke(optional));
+        assertFalse((Boolean) Class.forName(optionalFqcn).getMethod("isPresent").invoke(optional));
 
         Invocation findAny = this.getLastInvocation();
 
@@ -166,54 +128,34 @@ public class ReturnsEmptyValuesTest extends TestBase {
 
     @Test
     public void should_return_empty_Stream() throws Exception {
-        // given
-        Class<?> streamType = getClassOrSkipTest("java.util.stream.Stream");
-
-        // when
-        Object stream = values.returnValueFor(streamType);
-        long count = (Long) streamType.getMethod("count").invoke(stream);
-
-        // then
-        assertEquals("count of empty Stream", 0L, count);
+        verify_empty_Stream_is_returned("java.util.stream.Stream");
     }
 
     @Test
     public void should_return_empty_DoubleStream() throws Exception {
-        // given
-        Class<?> streamType = getClassOrSkipTest("java.util.stream.DoubleStream");
-
-        // when
-        Object stream = values.returnValueFor(streamType);
-        long count = (Long) streamType.getMethod("count").invoke(stream);
-
-        // then
-        assertEquals("count of empty DoubleStream", 0L, count);
+        verify_empty_Stream_is_returned("java.util.stream.DoubleStream");
     }
 
     @Test
     public void should_return_empty_IntStream() throws Exception {
-        // given
-        Class<?> streamType = getClassOrSkipTest("java.util.stream.IntStream");
-
-        // when
-        Object stream = values.returnValueFor(streamType);
-        long count = (Long) streamType.getMethod("count").invoke(stream);
-
-        // then
-        assertEquals("count of empty IntStream", 0L, count);
+        verify_empty_Stream_is_returned("java.util.stream.IntStream");
     }
 
     @Test
     public void should_return_empty_LongStream() throws Exception {
+        verify_empty_Stream_is_returned("java.util.stream.LongStream");
+    }
+
+    private void verify_empty_Stream_is_returned(String streamFqcn) throws Exception {
         // given
-        Class<?> streamType = getClassOrSkipTest("java.util.stream.LongStream");
+        Class<?> streamType = getClassOrSkipTest(streamFqcn);
 
         // when
         Object stream = values.returnValueFor(streamType);
         long count = (Long) streamType.getMethod("count").invoke(stream);
 
         // then
-        assertEquals("count of empty LongStream", 0L, count);
+        assertEquals("count of empty " + streamFqcn, 0L, count);
     }
 
     /**
```

I think that your proposed refactor is sufficient enough.
It's a poor man's parameterized tests, but just for that case it is probably enough.

Btw, I haven't used `Theories`, but `Parameterized` from JUnit is very verbose and limited. To define one parameterized test you need to sacrifice the whole class (you need to use fields and a constructor). There's no way to create more than one parameterized test in the same test class (which very often is very handy). JUnitParams is much more flexible (no to mentioned Spock brings it to the another level :) ).
Alright, pending a decision on which framework to use, I opened PR #783 with the changes shown above.

(Regarding Spock: my colleague @zshamrock gave a presentation/demo about that just last Thursday. It does look like a perfect fit for this sort of thing indeed.)
Is this a breaking change?
Well it's a behavioural change, so by definition one can write a test that passed prior but not afterwards. For this particular change that'd look something like the following:

1. One has a class `C` with a method `m` returning `OptionalDouble`.
2. One mocks `C`, producing an instance `c`.
3. One fails to explicitly mock the behaviour of `c.m`.
4. One uses `c` in some code that invokes `c.m` and relies on the result being `null`.

I think the "breakage" in this case is acceptable for the following reasons:

1. Since values of type `Optional*` should never be `null`, the failure points out a misuse of `Optional*`, and as such does the user a favour.
2. If one insists on a "nullable `Optional*`", adding a simple mock statement in step 3 above clearly documents that contract, consistent with the [principle of least astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment).
3. More generally, if a test crucially relies on a certain method's return value, then it's good practise to document this precondition explicitly.

I feel the above arguments apply to `Stream*` as much as they do to `Optional*`, because a stream is a (proto) collection in some sense, and nullable collections are also decidedly an anti-pattern. WDYT?
Your change is really good, it improves Mockito behavior for Java8!

Regardless of the final decision whether to merge it or not, I find it disturbing that it wasn't discussed as a breaking change in this PR. @bric3, @TimvdLippe thoughts?
To be honest, I had not realised this. Especially because I dont think someone relies on the previous null behavior.

Nonetheless we should have discussed this, good point.
JUnitParams is fine, I have nothing against, let's go for it, test readability and maintainability is just as important as production code.
Well my reasoning was fast : 

1. We forgot that in mockito 2, as a user I usually see that as a bug.
2. The change is consistent with the goal of mockito 2 aka support java 8 best way.
3. The change is breaking for users that uses primitive variant and that expect null for these stubs, which is not what those type are made for, so pretty rare.

All in a all this a reasonable change, that imo does not mean mockito should bump any version or postpone the change.

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValues.java b/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValues.java
index 58e40c2c01..2216cca7aa 100644
--- a/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValues.java
+++ b/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValues.java
@@ -37,7 +37,10 @@
  * Returns zero if references are equals otherwise non-zero for Comparable#compareTo(T other) method (see issue 184)
  * </li>
  * <li>
- * Returns an {@code java.util.Optional#empty() empty Optional} for Optional (see issue 191).
+ * Returns an {@code java.util.Optional#empty() empty Optional} for Optional. Similarly for primitive optional variants.
+ * </li>
+ * <li>
+ * Returns an {@code java.util.stream.Stream#empty() empty Stream} for Stream. Similarly for primitive stream variants.
  * </li>
  * <li>
  * Returns null for everything else
@@ -109,8 +112,20 @@ Object returnValueFor(Class<?> type) {
             return new LinkedHashMap<Object, Object>();
         } else if ("java.util.Optional".equals(type.getName())) {
             return JavaEightUtil.emptyOptional();
+        } else if ("java.util.OptionalDouble".equals(type.getName())) {
+            return JavaEightUtil.emptyOptionalDouble();
+        } else if ("java.util.OptionalInt".equals(type.getName())) {
+            return JavaEightUtil.emptyOptionalInt();
+        } else if ("java.util.OptionalLong".equals(type.getName())) {
+            return JavaEightUtil.emptyOptionalLong();
         } else if ("java.util.stream.Stream".equals(type.getName())) {
             return JavaEightUtil.emptyStream();
+        } else if ("java.util.stream.DoubleStream".equals(type.getName())) {
+            return JavaEightUtil.emptyDoubleStream();
+        } else if ("java.util.stream.IntStream".equals(type.getName())) {
+            return JavaEightUtil.emptyIntStream();
+        } else if ("java.util.stream.LongStream".equals(type.getName())) {
+            return JavaEightUtil.emptyLongStream();
         }
 
         //Let's not care about the rest of collections.
diff --git a/src/main/java/org/mockito/internal/util/JavaEightUtil.java b/src/main/java/org/mockito/internal/util/JavaEightUtil.java
index f09ae7dcde..80ac6353c1 100644
--- a/src/main/java/org/mockito/internal/util/JavaEightUtil.java
+++ b/src/main/java/org/mockito/internal/util/JavaEightUtil.java
@@ -14,16 +14,18 @@
  */
 public final class JavaEightUtil {
 
-    // No need for volatile, Optional#empty() is already a safe singleton.
+    // No need for volatile, these optionals are already safe singletons.
     private static Object emptyOptional;
+    private static Object emptyOptionalDouble;
+    private static Object emptyOptionalInt;
+    private static Object emptyOptionalLong;
 
     private JavaEightUtil() {
         // utility class
     }
 
     /**
-     * Creates an empty Optional using reflection to stay backwards-compatible with older
-     * JDKs (see issue 191).
+     * Creates an empty Optional using reflection to stay backwards-compatible with older JDKs.
      *
      * @return an empty Optional.
      */
@@ -33,35 +35,110 @@ public static Object emptyOptional() {
             return emptyOptional;
         }
 
-        try {
-            final Class<?> optionalClass = Class.forName("java.util.Optional");
-            final Method emptyMethod = optionalClass.getMethod("empty");
+        return emptyOptional = invokeNullaryFactoryMethod("java.util.Optional", "empty");
+    }
 
-            return emptyOptional = emptyMethod.invoke(null);
-            // any exception is really unexpected since the type name has
-            // already been verified to be java.util.Optional
-        } catch (Exception e) {
-            throw new InstantiationException("Could not create java.util.Optional#empty(): " + e, e);
+
+    /**
+     * Creates an empty OptionalDouble using reflection to stay backwards-compatible with older JDKs.
+     *
+     * @return an empty OptionalDouble.
+     */
+    public static Object emptyOptionalDouble() {
+        // no need for double-checked locking
+        if (emptyOptionalDouble != null) {
+            return emptyOptionalDouble;
+        }
+
+        return emptyOptionalDouble = invokeNullaryFactoryMethod("java.util.OptionalDouble", "empty");
+    }
+
+    /**
+     * Creates an empty OptionalInt using reflection to stay backwards-compatible with older JDKs.
+     *
+     * @return an empty OptionalInt.
+     */
+    public static Object emptyOptionalInt() {
+        // no need for double-checked locking
+        if (emptyOptionalInt != null) {
+            return emptyOptionalInt;
+        }
+
+        return emptyOptionalInt = invokeNullaryFactoryMethod("java.util.OptionalInt", "empty");
+    }
+
+    /**
+     * Creates an empty OptionalLong using reflection to stay backwards-compatible with older JDKs.
+     *
+     * @return an empty OptionalLong.
+     */
+    public static Object emptyOptionalLong() {
+        // no need for double-checked locking
+        if (emptyOptionalLong != null) {
+            return emptyOptionalLong;
         }
+
+        return emptyOptionalLong = invokeNullaryFactoryMethod("java.util.OptionalLong", "empty");
     }
 
     /**
-     * Creates an empty Stream using reflection to stay backwards-compatible with older
-     * JDKs.
+     * Creates an empty Stream using reflection to stay backwards-compatible with older JDKs.
      *
      * @return an empty Stream.
      */
     public static Object emptyStream() {
         // note: the empty stream can not be stored as a singleton.
+        return invokeNullaryFactoryMethod("java.util.stream.Stream", "empty");
+    }
+
+    /**
+     * Creates an empty DoubleStream using reflection to stay backwards-compatible with older JDKs.
+     *
+     * @return an empty DoubleStream.
+     */
+    public static Object emptyDoubleStream() {
+        // note: the empty stream can not be stored as a singleton.
+        return invokeNullaryFactoryMethod("java.util.stream.DoubleStream", "empty");
+    }
+
+    /**
+     * Creates an empty IntStream using reflection to stay backwards-compatible with older JDKs.
+     *
+     * @return an empty IntStream.
+     */
+    public static Object emptyIntStream() {
+        // note: the empty stream can not be stored as a singleton.
+        return invokeNullaryFactoryMethod("java.util.stream.IntStream", "empty");
+    }
+
+    /**
+     * Creates an empty LongStream using reflection to stay backwards-compatible with older JDKs.
+     *
+     * @return an empty LongStream.
+     */
+    public static Object emptyLongStream() {
+        // note: the empty stream can not be stored as a singleton.
+        return invokeNullaryFactoryMethod("java.util.stream.LongStream", "empty");
+    }
+
+    /**
+     * Invokes a nullary static factory method using reflection to stay backwards-compatible with older JDKs.
+     *
+     * @param fqcn The fully qualified class name of the type to be produced.
+     * @param methodName The name of the factory method.
+     * @return the object produced.
+     */
+    private static Object invokeNullaryFactoryMethod(final String fqcn, final String methodName) {
         try {
-            final Class<?> optionalClass = Class.forName("java.util.stream.Stream");
-            final Method emptyMethod = optionalClass.getMethod("empty");
+            final Class<?> type = Class.forName(fqcn);
+            final Method method = type.getMethod(methodName);
 
-            return emptyMethod.invoke(null);
+            return method.invoke(null);
             // any exception is really unexpected since the type name has
             // already been verified
-        } catch (Exception e) {
-            throw new InstantiationException("Could not create java.util.stream.Stream#empty(): " + e, e);
+        } catch (final Exception e) {
+            throw new InstantiationException(
+                    String.format("Could not create %s#%s(): %s", fqcn, methodName, e), e);
         }
     }
 }


Test patch:
diff --git a/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValuesTest.java b/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValuesTest.java
index b1553542b4..480a9a1c7f 100644
--- a/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValuesTest.java
+++ b/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValuesTest.java
@@ -89,7 +89,7 @@ public void should_return_zero_if_mock_is_compared_to_itself() {
     }
 
     @Test
-    public void should_return_empty_optional() throws Exception {
+    public void should_return_empty_Optional() throws Exception {
         Class<?> streamType = getClassOrSkipTest("java.util.stream.Stream");
 
         //given
@@ -108,7 +108,64 @@ public void should_return_empty_optional() throws Exception {
     }
 
     @Test
-    public void should_return_empty_stream() throws Exception {
+    public void should_return_empty_OptionalDouble() throws Exception {
+        Class<?> streamType = getClassOrSkipTest("java.util.stream.DoubleStream");
+
+        //given
+        Object stream = mock(streamType);
+        Object optional = streamType.getMethod("findAny").invoke(stream);
+        assertNotNull(optional);
+        assertFalse((Boolean) Class.forName("java.util.OptionalDouble").getMethod("isPresent").invoke(optional));
+
+        Invocation findAny = this.getLastInvocation();
+
+        //when
+        Object result = values.answer(findAny);
+
+        //then
+        assertEquals(optional, result);
+    }
+
+    @Test
+    public void should_return_empty_OptionalInt() throws Exception {
+        Class<?> streamType = getClassOrSkipTest("java.util.stream.IntStream");
+
+        //given
+        Object stream = mock(streamType);
+        Object optional = streamType.getMethod("findAny").invoke(stream);
+        assertNotNull(optional);
+        assertFalse((Boolean) Class.forName("java.util.OptionalInt").getMethod("isPresent").invoke(optional));
+
+        Invocation findAny = this.getLastInvocation();
+
+        //when
+        Object result = values.answer(findAny);
+
+        //then
+        assertEquals(optional, result);
+    }
+
+    @Test
+    public void should_return_empty_OptionalLong() throws Exception {
+        Class<?> streamType = getClassOrSkipTest("java.util.stream.LongStream");
+
+        //given
+        Object stream = mock(streamType);
+        Object optional = streamType.getMethod("findAny").invoke(stream);
+        assertNotNull(optional);
+        assertFalse((Boolean) Class.forName("java.util.OptionalLong").getMethod("isPresent").invoke(optional));
+
+        Invocation findAny = this.getLastInvocation();
+
+        //when
+        Object result = values.answer(findAny);
+
+        //then
+        assertEquals(optional, result);
+    }
+
+    @Test
+    public void should_return_empty_Stream() throws Exception {
         // given
         Class<?> streamType = getClassOrSkipTest("java.util.stream.Stream");
 
@@ -120,6 +177,45 @@ public void should_return_empty_stream() throws Exception {
         assertEquals("count of empty Stream", 0L, count);
     }
 
+    @Test
+    public void should_return_empty_DoubleStream() throws Exception {
+        // given
+        Class<?> streamType = getClassOrSkipTest("java.util.stream.DoubleStream");
+
+        // when
+        Object stream = values.returnValueFor(streamType);
+        long count = (Long) streamType.getMethod("count").invoke(stream);
+
+        // then
+        assertEquals("count of empty DoubleStream", 0L, count);
+    }
+
+    @Test
+    public void should_return_empty_IntStream() throws Exception {
+        // given
+        Class<?> streamType = getClassOrSkipTest("java.util.stream.IntStream");
+
+        // when
+        Object stream = values.returnValueFor(streamType);
+        long count = (Long) streamType.getMethod("count").invoke(stream);
+
+        // then
+        assertEquals("count of empty IntStream", 0L, count);
+    }
+
+    @Test
+    public void should_return_empty_LongStream() throws Exception {
+        // given
+        Class<?> streamType = getClassOrSkipTest("java.util.stream.LongStream");
+
+        // when
+        Object stream = values.returnValueFor(streamType);
+        long count = (Long) streamType.getMethod("count").invoke(stream);
+
+        // then
+        assertEquals("count of empty LongStream", 0L, count);
+    }
+
     /**
      * Tries to load the given class. If the class is not found, the complete test is skipped.
      */