demery-pivotal commented on a change in pull request #7357:
URL: https://github.com/apache/geode/pull/7357#discussion_r804903588
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionTest.java
##########
@@ -276,4 +278,26 @@ public void
doNormalMessageShouldNotProcessMessageWhenTerminated() {
assertThat(spy.getProcessMessages()).isFalse();
verify(spy, never()).resumeThreadMonitoring();
}
+
+ @Test
+ public void cleanClientAuthShouldNullIt() {
+ ClientUserAuths clientUserAuths = mock(ClientUserAuths.class);
+ ServerConnection spy = spy(serverConnection);
+ spy.setClientUserAuths(clientUserAuths);
+ spy.cleanClientAuths();
+ assertThat(spy.getClientUserAuths()).isNull();
+ }
Review comment:
This test exercises and verifies only implementation details. None of
the methods invoked by this test are ever called by a user of the
`ServerConnection`, which means that nothing in this test exercises an actual
responsibility.
To make this test test an actual responsibility:
- What method could a production caller call, and under what conditions,
that should cause `clientUserAuths` to change from non-null to null? Make the
test establish those conditions and call that method.
- If `clientUserAuths` is null, what effect should affect should that have
on the subsequent behavior of methods that a production caller can call? Make
the test call those methods and verify the results.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionTest.java
##########
@@ -276,4 +278,26 @@ public void
doNormalMessageShouldNotProcessMessageWhenTerminated() {
assertThat(spy.getProcessMessages()).isFalse();
verify(spy, never()).resumeThreadMonitoring();
}
+
+ @Test
+ public void cleanClientAuthShouldNullIt() {
+ ClientUserAuths clientUserAuths = mock(ClientUserAuths.class);
+ ServerConnection spy = spy(serverConnection);
+ spy.setClientUserAuths(clientUserAuths);
+ spy.cleanClientAuths();
+ assertThat(spy.getClientUserAuths()).isNull();
+ }
+
+ @Test
+ public void handleTerminationShouldNotNullClientAuths() {
+ when(acceptor.getClientHealthMonitor()).thenReturn(clientHealthMonitor);
Review comment:
This test verifies only an implementation detail. If
`handleTermination(...)` leaves `clientUserAuths` non-null, what effect does
that have on the subsequent behavior of methods that a production caller can
call? To test an actual responsibility, make the test call those methods and
verify the results.
Also, under some conditions, `handleTermination` definitely nulls
`clientUserAuths`, so there should be separate tests for the conditions where
it nulls `clientUserAuths` and the conditions where it does not.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
