You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/12/12 11:09:37 UTC
[GitHub] [zookeeper] eolivelli opened a new pull request, #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress - upgrade Mockito
eolivelli opened a new pull request, #1959:
URL: https://github.com/apache/zookeeper/pull/1959
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] cnauroth commented on a diff in pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress
Posted by GitBox <gi...@apache.org>.
cnauroth commented on code in PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#discussion_r1072845647
##########
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java:
##########
@@ -148,9 +156,6 @@ public void testServerHostnameVerificationWithHostnameVerificationDisabled() thr
X509Certificate[] certificateChain = createSelfSignedCertifcateChain(IP_ADDRESS, HOSTNAME);
zkTrustManager.checkServerTrusted(certificateChain, null, mockSocket);
- verify(mockInetAddress, times(0)).getHostAddress();
Review Comment:
These `verify` calls are used to assert for expected behavior of hostname verification in various use cases. Without the `verify` calls, we would lose some test coverage, and I think several of these tests would all be testing the same thing.
I'm not familiar with the new burningwave library. Do you know if there are any options it offers for intercepting these calls so that we could try to preserve this test coverage?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#issuecomment-1385045432
@li4wang @tisonkun @anmolnar @symat @cnauroth PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#issuecomment-1428311031
@cnauroth any other comments ?
I would like to merge the patch as soon as CI passes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#issuecomment-1481986604
@cnauroth no worries. You are welcome.
Glad to have fixed this problem the best way thanks to your feedback
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a diff in pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#discussion_r1073546869
##########
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java:
##########
@@ -148,9 +156,6 @@ public void testServerHostnameVerificationWithHostnameVerificationDisabled() thr
X509Certificate[] certificateChain = createSelfSignedCertifcateChain(IP_ADDRESS, HOSTNAME);
zkTrustManager.checkServerTrusted(certificateChain, null, mockSocket);
- verify(mockInetAddress, times(0)).getHostAddress();
Review Comment:
I knew that we are going to lose these assertions, the problem is that the tests verified that these two methods were not called (or called N times).
We cannot spy the InetAddress object anymore.
we could create two "versions" of this test, if we are on JDK19+ we do it this way, and if we are on older JDKs we keep the mock and run the test the same way as before.
I don't think that there will be short term plans to abandon older JDKs soon (years)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a diff in pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#discussion_r1092863933
##########
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java:
##########
@@ -85,20 +106,7 @@ public static void removeBouncyCastleProvider() throws Exception {
public void setup() throws Exception {
mockX509ExtendedTrustManager = mock(X509ExtendedTrustManager.class);
- mockInetAddress = mock(InetAddress.class);
Review Comment:
we don't need it anymore, removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] cnauroth commented on a diff in pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress
Posted by GitBox <gi...@apache.org>.
cnauroth commented on code in PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#discussion_r1074016629
##########
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java:
##########
@@ -148,9 +156,6 @@ public void testServerHostnameVerificationWithHostnameVerificationDisabled() thr
X509Certificate[] certificateChain = createSelfSignedCertifcateChain(IP_ADDRESS, HOSTNAME);
zkTrustManager.checkServerTrusted(certificateChain, null, mockSocket);
- verify(mockInetAddress, times(0)).getHostAddress();
Review Comment:
Here is another potential idea. I believe the intent of these tests is not necessarily to confirm that we called `InetAddress#getHostAddress()` or `getHostName()`, but rather to confirm that we performed hostname verification in the right circumstances. Therefore, we could try a refactoring as follows:
1. The `ZKTrustManager` constructor currently calls `new ZKHostnameVerifier()` directly. Change it to use dependency injection, with the constructor receiving an instance of the `HostnameVerifier` interface.
1. Alternative: if this ends up being too large of a change, keep the existing constructor and add a new one annotated `VisibleForTesting` that accepts the `HostnameVerifier`.
2. Keep the existing tests using `ZKHostnameVerifier` so that we're still fully covering that code path, using the new Burning Wave setup, even though we can't do complete assertions on it.
3. Remove all existing checks on `InetAddress#getHostAddress()` and `getHostName()`, just like you've already done.
4. Add a few new similar tests that mock and verify the `HostnameVerifier` instead of `InetAddress`. We should be able to keep mocking `HostnameVerifier` across all JDK versions.
@eolivelli , let me know your thoughts. Thanks!
##########
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java:
##########
@@ -85,20 +106,7 @@ public static void removeBouncyCastleProvider() throws Exception {
public void setup() throws Exception {
mockX509ExtendedTrustManager = mock(X509ExtendedTrustManager.class);
- mockInetAddress = mock(InetAddress.class);
Review Comment:
We are still left with a `mockInetAddress` member variable, which could cause some confusion since it's not really a mock anymore.
Can the member variable be removed completely? (Maybe not. Maybe it's still needed to return from the mock socket.)
If we still need it, then I suggest renaming it to `inetAddress` so that it's clearer that it's not really a mock.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli merged pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli merged PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org