[GitHub] [helix] jiajunwang commented on a change in pull request #1447: Fix unit test Thread.sleep() with TestHelper.verify()

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

jiajunwang commented on a change in pull request #1447:
URL: https://github.com/apache/helix/pull/1447#discussion_r500672847



##########
File path: helix-core/src/test/java/org/apache/helix/TestZKCallback.java
##########
@@ -165,8 +166,10 @@ public void testInvocation() throws Exception {
 
       ExternalView extView = new ExternalView("db-12345");
       accessor.setProperty(keyBuilder.externalView("db-12345"), extView);
-      Thread.sleep(100);
-      AssertJUnit.assertTrue(testListener.externalViewChangeReceived);
+      boolean result = TestHelper.verify(() -> {
+        return testListener.externalViewChangeReceived == true;

Review comment:
       nit, "== true" is not necessary.

##########
File path: helix-core/src/test/java/org/apache/helix/TestListenerCallback.java
##########
@@ -131,22 +131,28 @@ public void testConfigChangeListeners() throws Exception {
     TestConfigListener listener = new TestConfigListener();
     listener.reset();
     _manager.addInstanceConfigChangeListener(listener);
-    Assert.assertTrue(listener._instanceConfigChanged,
-        "Should get initial instanceConfig callback invoked");
+    boolean result = TestHelper.verify(()-> {
+      return listener._instanceConfigChanged == true;

Review comment:
       nit, "== true" is not necessary.

##########
File path: helix-core/src/test/java/org/apache/helix/controller/stages/TestRebalancePipeline.java
##########
@@ -152,15 +152,15 @@ public void testDuplicateMsg() throws Exception {
     Assert.assertTrue(messages.get(0).getFromState().equalsIgnoreCase("SLAVE"));
     Assert.assertTrue(messages.get(0).getToState().equalsIgnoreCase("MASTER"));
 
-    runPipeline(event, dataRefresh);
-
+    Thread.sleep(MessageGenerationPhase.DEFAULT_OBSELETE_MSG_PURGE_DELAY);
     // Verify the stale message should be deleted
     Assert.assertTrue(TestHelper.verify(() -> {
+      runPipeline(event, dataRefresh);

Review comment:
       This changes the original test logic. We shall only need one pipeline run after the delay. And it shall guarantee the operation is done correctly.
   If there is a concern about the delay time, we can wait for 2 * DEFAULT_OBSELETE_MSG_PURGE_DELAY.

##########
File path: helix-core/src/test/java/org/apache/helix/integration/rebalancer/TestClusterInMaintenanceModeWhenReachingOfflineInstancesLimit.java
##########
@@ -130,18 +130,24 @@ public void testWithDisabledInstancesLimit() throws Exception {
       admin.enableInstance(CLUSTER_NAME, instance, false);
     }
 
-    Thread.sleep(500);
-
-    maintenanceSignal = _dataAccessor.getProperty(_dataAccessor.keyBuilder().maintenance());
-    Assert.assertNull(maintenanceSignal);
+    boolean result = TestHelper.verify(() -> {
+      MaintenanceSignal ms = _dataAccessor.getProperty(_dataAccessor.keyBuilder().maintenance());
+      return ms == null;
+    }, TestHelper.WAIT_DURATION);
+    Assert.assertTrue(result);
 
     String instance = _participants.get(i).getInstanceName();
     admin.enableInstance(CLUSTER_NAME, instance, false);
 
-    Thread.sleep(500);
-    maintenanceSignal = _dataAccessor.getProperty(_dataAccessor.keyBuilder().maintenance());
-    Assert.assertNotNull(maintenanceSignal);
-    Assert.assertNotNull(maintenanceSignal.getReason());
+    ZkHelixClusterVerifier clusterVerifier1 =

Review comment:
       Please mind the naming of the variables. A meaningful name is required in all cases.

##########
File path: helix-core/src/test/java/org/apache/helix/TestHelper.java
##########
@@ -76,7 +76,7 @@
 
 public class TestHelper {
   private static final Logger LOG = LoggerFactory.getLogger(TestHelper.class);
-  public static final long WAIT_DURATION = 20 * 1000L; // 20 seconds
+  public static final long WAIT_DURATION = 60 * 1000L; // 60 seconds

Review comment:
       This reminds me that we are not testing for the latency for now. For example, if the callback does not happen within one second, then we may want to fail the test.
   Will it be a good idea to have at least 2 Duration?
   Pipeline_Wait_Duration = 60000;
   Callback_Wait_Duration = 2000;




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org