[GitHub] [samza] mynameborat commented on a change in pull request #1334: SAMZA-2496: TestContainerHeartbeatMonitor does not properly stop the ContainerHeartbeatMonitor

[GitBox <gi...@apache.org>]

mynameborat commented on a change in pull request #1334: SAMZA-2496: TestContainerHeartbeatMonitor does not properly stop the ContainerHeartbeatMonitor
URL: https://github.com/apache/samza/pull/1334#discussion_r399517388
 
 

 ##########
 File path: samza-core/src/test/java/org/apache/samza/container/TestContainerHeartbeatMonitor.java
 ##########
 @@ -20,44 +20,77 @@
 package org.apache.samza.container;
 
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
-import junit.framework.Assert;
 import org.junit.Test;
 
-import static org.mockito.Mockito.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyLong;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
-public class TestContainerHeartbeatMonitor {
 
+public class TestContainerHeartbeatMonitor {
   @Test
-  public void testCallbackWhenHeartbeatDead()
-      throws InterruptedException {
+  public void testCallbackWhenHeartbeatDead() throws InterruptedException {
     ContainerHeartbeatClient mockClient = mock(ContainerHeartbeatClient.class);
     CountDownLatch countDownLatch = new CountDownLatch(1);
-    Runnable onExpired = () -> {
-      countDownLatch.countDown();
-    };
-    ContainerHeartbeatMonitor monitor = new ContainerHeartbeatMonitor(onExpired, mockClient);
+    Runnable onExpired = countDownLatch::countDown;
     ContainerHeartbeatResponse response = new ContainerHeartbeatResponse(false);
     when(mockClient.requestHeartbeat()).thenReturn(response);
+    ScheduledExecutorService scheduler = buildScheduledExecutorService();
+    ContainerHeartbeatMonitor monitor = new ContainerHeartbeatMonitor(onExpired, mockClient, scheduler);
     monitor.start();
     boolean success = countDownLatch.await(2, TimeUnit.SECONDS);
-    Assert.assertTrue(success);
+    assertTrue(success);
+    // check that the shutdown task got submitted, but don't actually execute it since it will shut down the process
+    verify(scheduler).schedule(any(Runnable.class), eq((long) ContainerHeartbeatMonitor.SHUTDOWN_TIMOUT_MS),
+        eq(TimeUnit.MILLISECONDS));
+
+    monitor.stop();
+    verify(scheduler).shutdown();
   }
 
   @Test
-  public void testDoesNotCallbackWhenHeartbeatAlive()
-      throws InterruptedException {
+  public void testDoesNotCallbackWhenHeartbeatAlive() throws InterruptedException {
     ContainerHeartbeatClient client = mock(ContainerHeartbeatClient.class);
     CountDownLatch countDownLatch = new CountDownLatch(1);
-    Runnable onExpired = () -> {
-      countDownLatch.countDown();
-    };
-    ContainerHeartbeatMonitor monitor = new ContainerHeartbeatMonitor(onExpired, client);
+    Runnable onExpired = countDownLatch::countDown;
     ContainerHeartbeatResponse response = new ContainerHeartbeatResponse(true);
     when(client.requestHeartbeat()).thenReturn(response);
+    ScheduledExecutorService scheduler = buildScheduledExecutorService();
+    ContainerHeartbeatMonitor monitor = new ContainerHeartbeatMonitor(onExpired, client, scheduler);
     monitor.start();
     boolean success = countDownLatch.await(2, TimeUnit.SECONDS);
 
 Review comment:
   I see you are the author of tests. But, do we need to wait for 2 seconds here for this test? Isn't verifying the scheduler for no task submissions below sufficient?

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


With regards,
Apache Git Services