-
Notifications
You must be signed in to change notification settings - Fork 604
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
Java 17 support #2248
base: main
Are you sure you want to change the base?
Java 17 support #2248
Conversation
Signed-off-by: Andras Katona <akatona@cloudera.com>
This change is not moving away from jdk11 yet, it's just making it possible to compile with java17 too. |
|
||
defaultTestJvmArgs = [] | ||
// "JEP 403: Strongly Encapsulate JDK Internals" causes some tests to fail when they try | ||
// to access internals (often via mocking libraries). We use `--add-opens` as a workaround |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
often or always? As I see it is only used in the test task def, so perhaps we could rephrase this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more about the occurrences rather than temporal frequency. OFC those tests always fail without the add-opens.
BTW I copied it from Kafka :D. Most of the cases tests were failing because of easymock and powermock to be specific, but I have no clue why exactly java.time
had to be opened, gradle wasn't kind enough to provide me more info about the failing test(s) before I added it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Kafka easymock/powermock was replaced with mockito over time. It's still not done, but that's the way to get rid of these add-opens lateron.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, makes sense. I agree that we should rather use Mockito, however I think that comes with some test refactoring as well, so let's not do that now.
@bgrishinko would you please review this? |
Summary
Expected Behavior
build goes smoothly with java 17
Actual Behavior
build and tests fail with java 17
Steps to Reproduce
./gradlew build
and see the crashesKnown Workarounds
Categorization