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

[ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress #1959

Merged
merged 3 commits into from Feb 15, 2023

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented Dec 12, 2022

Some tests do not pass on latest JDK20 (and also in JDK19) because we are trying to mock InetAddress.

More context here
https://issues.apache.org/jira/browse/ZOOKEEPER-4647

Unfortunately upgrading Mockito doesn't help and so I had to add Burning Wave DNS tools on the classpath.

https://dev.to/jjbrt/how-to-configure-hostname-resolution-to-use-a-universal-custom-hostname-resolver-in-java-14p0

The tests that mock InetAddress now pass on JDK19 and JDK20 on my laptop

@eolivelli eolivelli force-pushed the ZOOKEEPER-4647-upgrade-mockito branch from 1f8170c to a43cd32 Compare January 17, 2023 08:53
@eolivelli eolivelli changed the title [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress - upgrade Mockito [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress Jan 17, 2023
@eolivelli eolivelli requested review from cnauroth, ztzg and anmolnar and removed request for cnauroth January 17, 2023 08:55
@eolivelli
Copy link
Contributor Author

Copy link
Contributor

@cnauroth cnauroth left a comment

Choose a reason for hiding this comment

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

@eolivelli , thank you for the patch. It looks good overall. I entered one question about potential impact to total test coverage.

Copy link
Contributor Author

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

@cnauroth I have followed your suggestions.

I really like the new form of the test, the assertions are now more accurate as we can test the exact sequence of verifications

cc @anmolnar

@eolivelli
Copy link
Contributor Author

@cnauroth any other comments ?
I would like to merge the patch as soon as CI passes

@eolivelli eolivelli force-pushed the ZOOKEEPER-4647-upgrade-mockito branch from bf1422b to 98fa34b Compare February 14, 2023 08:46
@eolivelli eolivelli merged commit 2e9c3f3 into apache:master Feb 15, 2023
@eolivelli eolivelli deleted the ZOOKEEPER-4647-upgrade-mockito branch February 15, 2023 07:36
eolivelli added a commit that referenced this pull request Feb 15, 2023
…tAddress (#1959)

- upgrade Mockito to 4.9.0
- use BurningWave DNS mock tools

(cherry picked from commit 2e9c3f3)
Copy link
Contributor

@cnauroth cnauroth left a comment

Choose a reason for hiding this comment

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

@eolivelli , sorry I went silent on this one. Belated +1, and thank you for incorporating the code review feedback!

@eolivelli
Copy link
Contributor Author

@cnauroth no worries. You are welcome.
Glad to have fixed this problem the best way thanks to your feedback

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