Default mock of Optional.isEmpty() returns true for RETURN_DEEP_STUBS #3097

AndreasTu · 2023-08-17T19:53:16Z

ReturnsDeepStubs now answers with true on Optional.isEmpty() when using RETURN_DEEP_STUBS.
ReturnsDeepStubs now returns real Optionals instead of mocks.

Fixes #2865

Checklist

Read the contributing guide
PR should be motivated, i.e. what does it fix, why, and if relevant how
If possible / relevant include an example in the description, that could help all readers
including project members to get a better picture of the change
Avoid other runtime dependencies
Meaningful commit history ; intention is important please rebase your commit history so that each
commit is meaningful and help the people that will explore a change in 2 years
The pull request follows coding style
Mention Fixes #<issue number> in the description if relevant
At least one commit should mention Fixes #<issue number> if relevant

TimvdLippe · 2023-08-17T22:01:50Z

src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsDeepStubs.java

    private static final long serialVersionUID = -7105341425736035847L;

    @Override
    public Object answer(InvocationOnMock invocation) throws Throwable {


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:

mockito/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsEmptyValues.java

Lines 142 to 149 in 076e8ac

} 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();

Is there a different reason why we are not hitting that codepath?

Because Optional returns to be mockable in line 66 !typeMockabilityOf(rawType, mockSettings.getMockMaker()).mockable().

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

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.

@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.

src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsDeepStubs.java

…P_STUBS ReturnsDeepStubs now answers with true on Optional.isEmpty() when using RETURN_DEEP_STUBS. ReturnsDeepStubs now returns real Optionals instead of mocks. Fixes mockito#2865

codecov-commenter · 2023-08-22T14:26:30Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.45% ⚠️

Comparison is base (c637192) 85.45% compared to head (4601a88) 85.01%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3097      +/-   ##
============================================
- Coverage     85.45%   85.01%   -0.45%     
+ Complexity     2889     2878      -11     
============================================
  Files           329      329              
  Lines          8822     8826       +4     
  Branches       1095     1095              
============================================
- Hits           7539     7503      -36     
- Misses          995     1047      +52     
+ Partials        288      276      -12

Files Changed	Coverage Δ
...rnal/stubbing/defaultanswers/ReturnsDeepStubs.java	`98.24% <100.00%> (+0.09%)`	⬆️
...al/stubbing/defaultanswers/ReturnsEmptyValues.java	`97.01% <100.00%> (+0.04%)`	⬆️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TimvdLippe

Thanks!

AndreasTu mentioned this pull request Aug 17, 2023

Default mock of Optional is not empty when using RETURN_DEEP_STUBS #2865

Closed

5 tasks

TimvdLippe reviewed Aug 17, 2023

View reviewed changes

AndreasTu force-pushed the fix_2865 branch 2 times, most recently from a88b734 to d8e94f1 Compare August 18, 2023 08:06

TimvdLippe reviewed Aug 18, 2023

View reviewed changes

src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsDeepStubs.java Outdated Show resolved Hide resolved

Default mock of Optional.isEmpty() returns true when using RETURN_DEE…

4601a88

…P_STUBS ReturnsDeepStubs now answers with true on Optional.isEmpty() when using RETURN_DEEP_STUBS. ReturnsDeepStubs now returns real Optionals instead of mocks. Fixes mockito#2865

AndreasTu force-pushed the fix_2865 branch from d8e94f1 to 4601a88 Compare August 22, 2023 14:16

AndreasTu requested a review from TimvdLippe August 22, 2023 14:16

TimvdLippe approved these changes Aug 25, 2023

View reviewed changes

TimvdLippe merged commit 63d1cd4 into mockito:main Aug 25, 2023

AndreasTu deleted the fix_2865 branch August 25, 2023 18:31

	} 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();

Uh oh!

Default mock of Optional.isEmpty() returns true for RETURN_DEEP_STUBS #3097

Default mock of Optional.isEmpty() returns true for RETURN_DEEP_STUBS #3097

Uh oh!

Conversation

AndreasTu commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TimvdLippe left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AndreasTu commented Aug 17, 2023 •

edited

Loading

codecov-commenter commented Aug 22, 2023 •

edited

Loading