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-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
