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 minimum support Java version from 8 to 11 #2804

Merged
merged 2 commits into from Dec 18, 2022

Conversation

reta
Copy link
Contributor

@reta reta commented Nov 28, 2022

Signed-off-by: Andriy Redko andriy.redko@aiven.io

Fixes #2798

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

build.gradle Show resolved Hide resolved
gradle.properties Show resolved Hide resolved
@reta
Copy link
Contributor Author

reta commented Nov 28, 2022

@TimvdLippe started the initial work here, thank you

@reta
Copy link
Contributor Author

reta commented Nov 28, 2022

It seems like I run into mojohaus/animal-sniffer#172 again (but this time using 1.22) :(

@TimvdLippe
Copy link
Contributor

Thanks for the PR, this is great! We will need to publish mockito-subclass in Mockito 4 first (#2589 (comment)), so until then we can't merge this PR.

Seems like CI is unhappy about our Javadoc. I couldn't find the failing error, but we have to build our Javadoc with no errors.

@reta
Copy link
Contributor Author

reta commented Nov 28, 2022

Thanks for the PR, this is great! We will need to publish mockito-subclass in Mockito 4 first (#2589 (comment)), so until then we can't merge this PR.

👍

Seems like CI is unhappy about our Javadoc. I couldn't find the failing error, but we have to build our Javadoc with no errors.

Thanks @TimvdLippe , I will certainly take a look

@reta
Copy link
Contributor Author

reta commented Nov 29, 2022

@TimvdLippe looks like regression mojohaus/animal-sniffer#172 is back in the new version, I will try to figure that out.

[Undefined reference | android-api-level-24-7.0_r2] org.mockito.internal.creation.bytebuddy.(InlineBytecodeGenerator.java:1)
  >> java.lang.instrument.Instrumentation
> Task :animalsnifferMain FAILED

[Undefined reference | android-api-level-24-7.0_r2] org.mockito.internal.creation.bytebuddy.(InlineDelegateByteBuddyMockMaker.java:1)
  >> java.lang.instrument.Instrumentation

[Undefined reference | android-api-level-24-7.0_r2] org.mockito.internal.creation.proxy.(MethodHandleProxy.java:1)
  >> java.lang.invoke.MethodHandles.Lookup

[Undefined reference | android-api-level-24-7.0_r2] org.mockito.internal.creation.proxy.(MethodHandleProxy.java:1)
  >> java.lang.invoke.MethodHandle

[Undefined reference | android-api-level-24-7.0_r2] org.mockito.internal.util.reflection.(InstrumentationMemberAccessor.java:1)
  >> java.lang.instrument.Instrumentation

[Undefined reference | android-api-level-24-7.0_r2] org.mockito.internal.util.reflection.(InstrumentationMemberAccessor.java:1)
  >> java.lang.invoke.MethodHandles.Lookup

Note: Some input files use or override a deprecated API.

Note: Recompile with -Xlint:deprecation for details.
> Task :errorprone:compileTestJava

@reta reta force-pushed the issue-2798 branch 3 times, most recently from fe59f40 to cd0c8f9 Compare November 30, 2022 14:12
@reta
Copy link
Contributor Author

reta commented Nov 30, 2022

@TimvdLippe looks like regression mojohaus/animal-sniffer#172 is back in the new version, I will try to figure that out.

Huh ... found a workaround here: https://github.com/mockito/mockito/pull/2804/files#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R109, not ideal but the only one which works

@reta reta force-pushed the issue-2798 branch 4 times, most recently from 42161f9 to af69a17 Compare November 30, 2022 16:18
@reta reta force-pushed the issue-2798 branch 2 times, most recently from 5c0902f to 877152e Compare November 30, 2022 18:31
@reta
Copy link
Contributor Author

reta commented Nov 30, 2022

image

Should be good to go :)

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2022

Codecov Report

Base: 84.91% // Head: 84.91% // No change to project coverage 👍

Coverage data is based on head (3863238) compared to base (3863238).
Patch has no changes to coverable lines.

❗ Current head 3863238 differs from pull request most recent head 1ce6b84. Consider uploading reports for the commit 1ce6b84 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2804   +/-   ##
=========================================
  Coverage     84.91%   84.91%           
  Complexity     2812     2812           
=========================================
  Files           320      320           
  Lines          8574     8574           
  Branches       1042     1042           
=========================================
  Hits           7281     7281           
  Misses         1012     1012           
  Partials        281      281           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@reta reta marked this pull request as ready for review December 1, 2022 13:31
@reta reta changed the title [WIP] Update minimum support Java version from 8 to 11 Update minimum support Java version from 8 to 11 Dec 1, 2022
@@ -24,26 +24,13 @@ private interface Factory {
}

private static Factory createLocationFactory() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you were initially working on this PR to get these changes in. However, I would like to include these changes in Mockito 5.1.0, so to avoid pulling in too many changes into 1 go. Can you split out these changes into a separate PR so that we can land that? We can then also consider cleaning up some more of these Java 8 utility classes such as JavaEightUtil (

} else if ("java.util.Optional".equals(type.getName())) {
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TimvdLippe sure, I can split that, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TimvdLippe I reverted the changes, would you be open to have a plugin for LocationFactory so the alternative implementation could be supplied? (with revert, we are back to reflection <-> SecurityManager issues we've started with). I will open separate issue / pull request, should be a low risk change from implementation perspective, but I am wondering how it is aligned with the project.

@TimvdLippe
Copy link
Contributor

All of these changes look good! I would like to land the functional changes in a separate PR and in Mockito 5.1.0 to minimize impact. Other than that, we are mostly blocked on mockito-subclass (#2589 (comment)) before we can merge this PR.

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
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.

This is great work, thank you so much!

import java.util.stream.Collectors;
import java.util.stream.Stream;

class LocationImpl implements Location, Serializable {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably have been removed from this PR as well, but I don't see any harm in including an unused class for now. Since I don't want another review cycle, let's merge this now so we can get things going! Thanks 😄

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.

Update minimum support Java version from 8 to 11
3 participants