Repository: mockito/mockito
Issue: Default mock of `Optional` is not `empty` when using `RETURN_DEEP_STUBS`
Labels: please contribute

Version affected: Mockito: 4.x & 5.x

If you create a mock with `RETURN_DEEP_STUBS`, then any method that returns a collection will, by default, return an empty collection, i.e. `isEmpty` returns `true`, etc.  This is great, as it means no additional configuration of the mock is required before passing to logic that would attempt to access elements of the collection.

However, the same is not true for `Optional`, (and I'm assuming `OptionalLong` etc): if `RETURN_DEEP_STUBS` is used, methods that return `Optional` aren't treated in any special way and just return a default mock with not stubbing configured. This means `isEmpty` returns `false`, but `get()` returns `null`.   This breaks the contract of `Optional`, causing test failures that aren't actually valid, or requiring manual configuration of the `Optional` mock to do-the-right-thing, cluttering test code.

Here's an example test that demonstrates this:

```java
interface Address {
   Optional<String> getState();
}

@Test
public void shouldDefaultDeepMockOptionalsToEmpty() {
    final Address address = mock(Address.class, RETURNS_DEEP_STUBS);

    assertThat(address.getState()).isEqualTo(Optional.empty());  // <--- fails
}

@Test
public void shouldDefaultMockOptionalsToEmpty() {
    final Address address = mock(Address.class); // <-- no RETURNS_DEEP_STUBS

    assertThat(address.getState()).isEqualTo(Optional.empty());  // <--- passes
}
```

Note the second test, which doesn't use deep mocking, passes.

 - [x] The mockito message in the stacktrace have useful information, but it didn't help
 - [x] The problematic code (if that's possible) is copied here;
       Note that some configuration are impossible to mock via Mockito
 - [x] Provide versions (mockito / jdk / os / any other relevant information)
 - [x] Provide a [Short, Self Contained, Correct (Compilable), Example](http://sscce.org) of the issue
       (same as any question on stackoverflow.com)
 - [x] Read the [contributing guide](https://github.com/mockito/mockito/blob/main/.github/CONTRIBUTING.md)




Issue comments:
This should have been covered in https://github.com/mockito/mockito/blob/0aba0d5b031b4282c9b6686add2b443bda761817/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValues.java#L132 but I am not sure why that is breaking. #2731 is supposed to tackle that, but it needs to be rebased. Feel free to submit a PR that fixes this issue!
I'm seeing a similar [test failures](https://app.circleci.com/pipelines/github/palantir/tritium/2642/workflows/773f100f-fe40-4d13-9e03-a2337249bc67/jobs/19318/tests) with deep stubs and methods returning `Optional<Long>` and `OptionalLong` after the upgrade to Mockito 5.x in https://github.com/palantir/tritium/pull/1635 . The exception we see is below:

```
java.lang.ClassCastException: class com.github.benmanes.caffeine.cache.Policy$Eviction$MockitoMock$nsH7a8Ju cannot be cast to class java.lang.Long (com.github.benmanes.caffeine.cache.Policy$Eviction$MockitoMock$nsH7a8Ju is in unnamed module of loader 'app'; java.lang.Long is in module java.base of loader 'bootstrap')
	at com.palantir.tritium.metrics.caffeine.CaffeineStats.lambda$weightedSize$1(CaffeineStats.java:60)
	at com.palantir.tritium.metrics.caffeine.CaffeineCacheMetricsTest.lambda$weightedSize$2(CaffeineCacheMetricsTest.java:87)
	at org.assertj.core.api.AbstractObjectAssert.returns(AbstractObjectAssert.java:1106)
	at com.palantir.tritium.metrics.caffeine.CaffeineCacheMetricsTest.weightedSize(CaffeineCacheMetricsTest.java:87)
```
@schlosna, as far was I'm aware, this was not a regression, just an existing issue, effecting at least the latest 4.x and 5.x versions. 

What version were you updating from?
> @schlosna, as far was I'm aware, this was not a regression, just an existing issue, effecting at least the latest 4.x and 5.x versions. 
> 
> What version were you updating from?

I saw this in https://github.com/palantir/tritium/pull/1635 when upgrading from Mockito 4.11.0 to 5.0.0 and running on JDK 17. 

I tried dropping the tests above into a mockito clone locally, but they did not reproduce. I can try a few more things to try to reproduce.

@big-andy-coates  I tried to fix this but eventually I came across an issue that if you have RETURNS_DEEP_STUBS, why would you expect an Optional to be not a mock? If it wouldn't be you would make some users quite surprised.

Having List.isEmpty = false is a happy coincidence, because it is a mock and not a `Collection.emptyList` nor `List.of()`.

In case of Optional, you get one that is not `Optional.empty()`, but gives you `false` when you ask for `Optional.isPresent`, the only issue is that it also returns `false` when you ask `Optional.isEmpty` - but this is normal mock behavior, defaults are set to `false` for booleans.
Should we consider leaving it as a mock, but stubbing the `isPresent` method to return `false`?  This would require the mock to be `lenient`, but I think that's preferable to the current non-sensical defaults.
The PR #3097 would now return `true` for all `Optional.isEmpty()` classes when using `RETURNS_DEEP_STUBS` and will return a real Optional also in the `RETURNS_DEEP_STUBS`, as discussen with @TimvdLippe in the PR.

The behavior of `Collection.isEmpty()` remains the same.

Pull request review comments:
In this answer, on line 68 we already call `delegate().answer()` where the delegate is `ReturnsEmptyValues`. This class in turn already returns an empty optional: https://github.com/mockito/mockito/blob/076e8ac837c17e81347854feaad69687cd7d2b83/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValues.java#L142-L149

Is there a different reason why we are not hitting that codepath?
1. Because `Optional` returns to be mockable in line 66 `!typeMockabilityOf(rawType, mockSettings.getMockMaker()).mockable()`.
2. The `delegate().answer()` would return a real `Optional`, which was mentioned in the ticket not to be the right thing to return. As the `RETURNS_DEEP_STUBS` also do not return `delegate().answer()` for e.g. `Lists`.
In that case I would say the delegation is not working as expected and we should fix that instead. I would like us not to reimplement the logic of returning empty values in another place, which is why we have the delegate. If that is not working properly, I consider that the bug. Can you look into fixing that while we make sure we don't break other use cases?
@TimvdLippe Yes I can do that.
But just to be sure, that will also return a real Optional in the `RETURNS_DEEP_STUBS` case.
So the observable behavior for the client will be different for e.g. Lists and Optionals, when using  `RETURNS_DEEP_STUBS`.
@TimvdLippe done
Are we sure this is what we want? Playing devils advocate for a moment...

This would make the default behaviour consistent, i.e. returning an empty optional, but would change the way users would need to deep mock methods returning optionals (and collections?).  Instead of mocking the calls to the optional's methods, users would need to mock the method returning the optional.

For example:

```java
when(a.getOptional().isPresent()).thenReturn(true);
when(a.getOptional().get()).thenReturn("thing");
```

Would need to become:

```java
when(a.getOptional()).thenReturn(Optional.of("thing"));
```

Or, if they really did still want an optional mock for some reason:

```java
Optional<String> mockOptional = mock(Optional.class);
when(mockOptional.isPresent()).thenReturn(true);
when(mockOptional.get()).thenReturn("thing");
when(a.getOptional()).thenReturn(mockOptional);
```

Personally, I think returning a real empty optional is better as the optional is an immutable object, so returning an immutable instance means the immutability is maintained in testing by default, highlighting unreachable branches etc.

It is a change that would need documenting both in the standard docs and in the release notes to help users upgrade (it's a breaking change).

If we choose to go with an actual `Optional` instance... are there other default types that should follow the same route? I'm not sure collections should, as they aren't necessarily immutable.

 
We don't support mocking Java internal types such as `Optional`. The fact that it worked by accident I consider a bug, not a feature. Therefore, while we can document it, I consider this a bug fix to fix code that should have never worked and we did not support. I don't even know if it works as we would expect it does, so who knows what the undefined behaviour is.

For all of these, we should delegate to `ReturnsEmptyValues` so that we have one place where we define this. Since Collections are returned and are in the same category, they would follow as well.
Rather than coding the list here, what if we call the delegate for this type, check if it is `null` and if not, return it. Otherwise, we create the deep stub as normal. This then also removes the need for calling `typeMockabilityOf`, as it is weird we call that first anyways.
@TimvdLippe The proposed change to use `ReturnsEmptyValues` breaks multiple existing (>=12) tests.
Shall I go forward and change these tests to comply to the new behavior?
This seams like an invasive change.

Example:
`org.mockitousage.basicapi.MocksSerializationTest#BUG_ISSUE_399_try_some_mocks_with_current_answers`
`org.mockito.internal.stubbing.defaultanswers.ReturnsGenericDeepStubsTest#will_return_default_value_on_non_mockable_nested_generic`

and more...
Hm yes that is too many breakages unfortunately. Still, I would prefer us to not implement this same list twice. Can we come up with a way that uses both?
I could delegate the check into `ReturnsEmptyValue` where I extract the Optional checks into an own method, then we have the different logic, but near each other to see the differences.
@TimvdLippe Done.

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsDeepStubs.java b/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsDeepStubs.java
index 8356c1b79b..27faed9d9a 100644
--- a/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsDeepStubs.java
+++ b/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsDeepStubs.java
@@ -54,6 +54,10 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
         MockCreationSettings<?> mockSettings = MockUtil.getMockSettings(invocation.getMock());
 
         Class<?> rawType = returnTypeGenericMetadata.rawType();
+        final var emptyValue = ReturnsEmptyValues.returnCommonEmptyValueFor(rawType);
+        if (emptyValue != null) {
+            return emptyValue;
+        }
         if (!typeMockabilityOf(rawType, mockSettings.getMockMaker()).mockable()) {
             if (invocation.getMethod().getReturnType().equals(rawType)) {
                 return delegate().answer(invocation);
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 20e9d85afe..c2dd0f7b2b 100644
--- a/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValues.java
+++ b/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValues.java
@@ -139,14 +139,6 @@ Object returnValueFor(Class<?> type) {
             return new TreeMap<>();
         } else if (type == LinkedHashMap.class) {
             return new LinkedHashMap<>();
-        } else if (type == Optional.class) {
-            return Optional.empty();
-        } else if (type == OptionalDouble.class) {
-            return OptionalDouble.empty();
-        } else if (type == OptionalInt.class) {
-            return OptionalInt.empty();
-        } else if (type == OptionalLong.class) {
-            return OptionalLong.empty();
         } else if (type == Stream.class) {
             return Stream.empty();
         } else if (type == DoubleStream.class) {
@@ -160,8 +152,27 @@ Object returnValueFor(Class<?> type) {
         } else if (type == Period.class) {
             return Period.ZERO;
         }
-
         // Let's not care about the rest of collections.
+
+        return returnCommonEmptyValueFor(type);
+    }
+
+    /**
+     * Returns empty values for common known types, shared between {@link ReturnsEmptyValues} and {@link ReturnsDeepStubs}.
+     *
+     * @param type the type to check
+     * @return the empty value, or {@code null}
+     */
+    static Object returnCommonEmptyValueFor(Class<?> type) {
+        if (type == Optional.class) {
+            return Optional.empty();
+        } else if (type == OptionalDouble.class) {
+            return OptionalDouble.empty();
+        } else if (type == OptionalInt.class) {
+            return OptionalInt.empty();
+        } else if (type == OptionalLong.class) {
+            return OptionalLong.empty();
+        }
         return null;
     }
 }


Test patch:
diff --git a/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsOptionalMocksTest.java b/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsOptionalMocksTest.java
new file mode 100644
index 0000000000..3364b6d984
--- /dev/null
+++ b/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsOptionalMocksTest.java
@@ -0,0 +1,154 @@
+/*
+ * Copyright (c) 2023 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockito.internal.stubbing.defaultanswers;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockingDetails;
+
+import java.util.Optional;
+import java.util.OptionalDouble;
+import java.util.OptionalInt;
+import java.util.OptionalLong;
+
+import org.junit.Test;
+
+public class ReturnsOptionalMocksTest {
+    private interface Type {
+        Optional<String> getOptString();
+
+        OptionalLong getOptLong();
+
+        OptionalDouble getOptDouble();
+
+        OptionalInt getOptInt();
+    }
+
+    @Test
+    public void deepStubs_Optional_should_return_normal_optional_empty_Issue2865() {
+        final Type type = mock(Type.class, RETURNS_DEEP_STUBS);
+        assertThat(type.getOptString()).isEqualTo(Optional.empty());
+        assertIsNoMock(type.getOptString());
+    }
+
+    @Test
+    public void deepStubs_OptionalLong_should_return_normal_optional_empty() {
+        final Type type = mock(Type.class, RETURNS_DEEP_STUBS);
+        assertThat(type.getOptLong()).isEqualTo(OptionalLong.empty());
+        assertIsNoMock(type.getOptLong());
+    }
+
+    @Test
+    public void deepStubs_OptionalDouble_should_normal_optional_empty() {
+        final Type type = mock(Type.class, RETURNS_DEEP_STUBS);
+        assertThat(type.getOptDouble()).isEqualTo(OptionalDouble.empty());
+        assertIsNoMock(type.getOptDouble());
+    }
+
+    @Test
+    public void deepStubs_OptionalInt_should_normal_optional_empty() {
+        final Type type = mock(Type.class, RETURNS_DEEP_STUBS);
+        assertThat(type.getOptInt()).isEqualTo(OptionalInt.empty());
+        assertIsNoMock(type.getOptInt());
+    }
+
+    @Test
+    public void normal_mock_Optional_should_return_normal_optional_empty() {
+        final Type type = mock(Type.class);
+        assertThat(type.getOptString()).isEqualTo(Optional.empty());
+        assertIsNoMock(type.getOptString());
+    }
+
+    @Test
+    public void normal_mock_OptionalLong_should_return_normal_optional_empty() {
+        final Type type = mock(Type.class);
+        assertThat(type.getOptLong()).isEqualTo(OptionalLong.empty());
+    }
+
+    @Test
+    public void normal_mock_OptionalDouble_should_return_normal_optional_empty() {
+        final Type type = mock(Type.class);
+        assertThat(type.getOptDouble()).isEqualTo(OptionalDouble.empty());
+    }
+
+    @Test
+    public void normal_mock_OptionalInt_should_return_normal_optional_empty() {
+        final Type type = mock(Type.class);
+        assertThat(type.getOptInt()).isEqualTo(OptionalInt.empty());
+    }
+
+    @Test
+    public void deepStubs_Optional_isPresent_isEmpty_Issue2865() {
+        final Type type = mock(Type.class, RETURNS_DEEP_STUBS);
+        Optional<String> opt = type.getOptString();
+        assertThat(opt.isPresent()).isEqualTo(false);
+        assertThat(opt.isEmpty()).isEqualTo(true);
+    }
+
+    @Test
+    public void deepStubs_OptionalLong_isPresent_isEmpty_Issue2865() {
+        final Type type = mock(Type.class, RETURNS_DEEP_STUBS);
+        OptionalLong opt = type.getOptLong();
+        assertThat(opt.isPresent()).isEqualTo(false);
+        assertThat(opt.isEmpty()).isEqualTo(true);
+    }
+
+    @Test
+    public void deepStubs_OptionalDouble_isPresent_isEmpty_Issue2865() {
+        final Type type = mock(Type.class, RETURNS_DEEP_STUBS);
+        OptionalDouble opt = type.getOptDouble();
+        assertThat(opt.isPresent()).isEqualTo(false);
+        assertThat(opt.isEmpty()).isEqualTo(true);
+    }
+
+    @Test
+    public void deepStubs_OptionalInt_isPresent_isEmpty_Issue2865() {
+        final Type type = mock(Type.class, RETURNS_DEEP_STUBS);
+        OptionalInt opt = type.getOptInt();
+        assertThat(opt.isPresent()).isEqualTo(false);
+        assertThat(opt.isEmpty()).isEqualTo(true);
+    }
+
+    @Test
+    public void normal_mock_Optional_isPresent_isEmpty_Issue2865() {
+        final Type type = mock(Type.class);
+
+        Optional<String> opt = type.getOptString();
+        assertThat(opt.isPresent()).isEqualTo(false);
+        assertThat(opt.isEmpty()).isEqualTo(true);
+    }
+
+    @Test
+    public void normal_mock_OptionalLong_isPresent_isEmpty_Issue2865() {
+        final Type type = mock(Type.class);
+
+        OptionalLong opt = type.getOptLong();
+        assertThat(opt.isPresent()).isEqualTo(false);
+        assertThat(opt.isEmpty()).isEqualTo(true);
+    }
+
+    @Test
+    public void normal_mock_OptionalDouble_isPresent_isEmpty_Issue2865() {
+        final Type type = mock(Type.class);
+
+        OptionalDouble opt = type.getOptDouble();
+        assertThat(opt.isPresent()).isEqualTo(false);
+        assertThat(opt.isEmpty()).isEqualTo(true);
+    }
+
+    @Test
+    public void normal_mock_OptionalInt_isPresent_isEmpty_Issue2865() {
+        final Type type = mock(Type.class);
+
+        OptionalInt opt = type.getOptInt();
+        assertThat(opt.isPresent()).isEqualTo(false);
+        assertThat(opt.isEmpty()).isEqualTo(true);
+    }
+
+    private void assertIsNoMock(Object mock) {
+        assertThat(mockingDetails(mock).isMock()).isFalse();
+    }
+}