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

Update to OpenJDK 15. #2051

Merged
merged 3 commits into from Nov 15, 2020
Merged

Update to OpenJDK 15. #2051

merged 3 commits into from Nov 15, 2020

Conversation

raphw
Copy link
Member

@raphw raphw commented Sep 18, 2020

Use Java 15 in builds for most recent Java version.

@TimvdLippe
Copy link
Contributor

Seems like we got some test failures :(

@raphw raphw force-pushed the jdk15 branch 2 times, most recently from b6e0127 to 9dad024 Compare September 19, 2020 07:07
@raphw
Copy link
Member Author

raphw commented Sep 19, 2020

Needed to update Jacoco. Works now.

@raphw
Copy link
Member Author

raphw commented Sep 19, 2020

Strangely enough, it builds locally. Will investigate some more!

@raphw
Copy link
Member Author

raphw commented Sep 19, 2020

It's weird, if I build locally with the same command and Java version (build and major), I cannot reproduce this error...

@TimvdLippe
Copy link
Contributor

I don't think I have Java 15 installed on any of my machines and not sure if I can get a hold of them. Looking at the test failures, many of them seem related to how we handle null with primitive matchers. Is there some way to check the changelog to see if we see any suspicious changes?

@raphw
Copy link
Member Author

raphw commented Sep 19, 2020

I ran the exact build version locally and it works, on a fresh clone even, also on Linux.

@Marcono1234
Copy link
Contributor

I am seeing these failures for JDK 15 (build 15+36-1562) as well, but on Windows.
It looks like the cause for this is JDK-8233014 which enables "Helpful NullPointerExceptions" by default.

The tests which are failing apparently make assumptions about how NullPointerExceptions look like, which are now not true anymore:

@TimvdLippe
Copy link
Contributor

Thanks for the great investigation @Marcono1234 Do you mind creating a PR with the necessary fixes to make it work on JDK 15?

@Marcono1234
Copy link
Contributor

I fear that I am not familiar enough with Mockito to perform these changes while still keeping the intended functionality for the tests.
For StubbingWithDelegateTest it looks like this could be simplified to checking whether the returned Byte is null instead of provoking the NullPointerException, is that correct? I assume for DeepStubsSerializableTest it is similar, it might already suffice to check whether next() returned null.
But for InvalidUseOfMatchersTest maybe the only solution would be to drop the requirement that the message must be null.

@raphw
Copy link
Member Author

raphw commented Nov 10, 2020

The requirement does not seem meaningful to me, at least. I just fixed the tests to not require an empty message.

@codecov-io
Copy link

codecov-io commented Nov 10, 2020

Codecov Report

Merging #2051 (b8bb0f0) into release/3.x (2e6c816) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             release/3.x    #2051   +/-   ##
==============================================
  Coverage          84.74%   84.74%           
  Complexity          2717     2717           
==============================================
  Files                325      325           
  Lines               8277     8277           
  Branches             989      989           
==============================================
  Hits                7014     7014           
  Misses               992      992           
  Partials             271      271           

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 2e6c816...b8bb0f0. Read the comment docs.

Copy link
Contributor

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

I might not be familiar enough with Mockito, but wouldn't it be possible for StubbingWithDelegateTest and DeepStubsSerializableTest to not provoke the NullPointerException at all (as outlined above), but instead test for a null return value? That would probably make the tests more robust since they might otherwise catch unrelated NullPointerExceptions which are caused by bugs.

Comment on lines 76 to 80
assertThatThrownBy(
() ->
when(deserialized_deep_stub.iterator().next().get(42))
.thenReturn("no"))
.isInstanceOf(NullPointerException.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertThatThrownBy(
() ->
when(deserialized_deep_stub.iterator().next().get(42))
.thenReturn("no"))
.isInstanceOf(NullPointerException.class);
assertThat(deserialized_deep_stub.iterator().next()).isNull();

Due to Java expression evaluation order the enclosing when(...) and the following thenReturn(...) are not executed at all.

Comment on lines 104 to 109
assertThatThrownBy(
() -> {
@SuppressWarnings("unused")
byte b = methods.byteObjectReturningMethod();
})
.isInstanceOf(NullPointerException.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertThatThrownBy(
() -> {
@SuppressWarnings("unused")
byte b = methods.byteObjectReturningMethod();
})
.isInstanceOf(NullPointerException.class);
assertThat(methods.byteObjectReturningMethod()).isNull();

NullPointerException here is not triggered by Mockito, but instead by unboxing Bytebyte so this can probably simplified to checking for a null return value.

@raphw
Copy link
Member Author

raphw commented Nov 11, 2020

Good point, I just adjusted the tests.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Test changes LGTM.

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

Successfully merging this pull request may close these issues.

None yet

4 participants