[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3384: fix 3372 - use waitFor to handle async conditions

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#discussion_r1189776103


##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryStarvedScanIT.java:
##########
@@ -272,7 +273,7 @@ public void testScanPauses() throws Exception {
         Thread.sleep(1500);
         assertEquals(currentCount, fetched.get());
 
-        assertEquals(1, LOW_MEM_DETECTED.get());
+        assertTrue(Wait.waitFor(() -> (LOW_MEM_DETECTED.get() == 1), 20_000L, 1000L));

Review Comment:
   All of these lines use the same max wait time and delay, and all of them assert that the condition is true (that assertion could probably be moved into the Wait class.. but I didn't check to see where all it was used). All of them also seem to have an unnecessary parens around the condition in the lambda. You should remove all this boilerplate by creating a local method:
   
   ```suggestion
           waitForTrue(() -> LOW_MEM_DETECTED.get() == 1);
   ```
   
   The method would look like:
   
   ```java
     private static void waitForTrue(Wait.Condition condition) throws Exception {
       // could specify 20 seconds, but the default is 30, and that's fine. The default delay is 1.
       assertTrue(Wait.waitFor(condition));
     }
   ```



-- 
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@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org