Useing Awaitility for asynchroneous testing. #3392

CalvinKirs · 2022-07-07T03:22:40Z

Descriptions of the changes in this PR:

Using Thread.sleep in a test is just generally a bad idea. It creates brittle tests that can fail unpredictably depending on environment ("Passes on my machine!") or load. So I used Awaitility for asynchroneous testing.

Such as:

https://github.com/apache/bookkeeper/runs/7219059790?check_suite_focus=true
https://github.com/apache/bookkeeper/runs/7206535584?check_suite_focus=true
(He may not be a perfect guarantee, but it can improve the stability ratio.)

Motivation

Using Thread.sleep in a test is generally a bad idea. It creates brittle tests that can fail unpredictably depending on the environment ("Passes on my machine!") or load

Changes

Useing Awaitility for asynchronous testing.
Useing Awaitility test AutoRecoveryMainTest
Useing Awaitilitytest BookieZKExpireTest

nicoloboschi · 2022-07-07T07:42:05Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java

@@ -76,7 +78,7 @@ conf, new TestBookieImpl(conf),

            int secondsToWait = 5;
            while (!server.isRunning()) {
-                Thread.sleep(1000);
+                await().atMost(1000, TimeUnit.MILLISECONDS);


this loop should become

await().atMost(5, SECONDS).until(() -> server.isRunning())

nicoloboschi · 2022-07-07T07:42:29Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java

@@ -95,11 +97,11 @@ conf, new TestBookieImpl(conf),
            assertNotNull("Send thread not found", sendthread);

            sendthread.suspend();
-            Thread.sleep(2 * conf.getZkTimeout());
+            await().atMost(2L * conf.getZkTimeout(), TimeUnit.MILLISECONDS).await();


we can leave the sleep here, we are not waiting for a particular condition

nicoloboschi · 2022-07-07T07:43:12Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java

            sendthread.resume();

            // allow watcher thread to run
-            Thread.sleep(3000);
+            await().atMost(3, TimeUnit.SECONDS).await();


await().atMost(5, SECONDS).until(() -> server.isBookieRunning() && server.isRunning())

nicoloboschi · 2022-07-07T07:43:52Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java

-            }
-            Thread.sleep(1000);
-        }
+        await().atMost(20, SECONDS).until(()->!main1.auditorElector.isRunning() && !main1.replicationWorker.isRunning()


better to keep the same timeout (10 seconds)

nicoloboschi · 2022-07-07T07:43:56Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java

-                Thread.sleep(1000);
-            }
-        }
+        await().atMost(20, SECONDS).until(() -> main1.auditorElector.getAuditor() != null);


better to keep the same timeout (10 seconds)

Thanks,updated

hangc0276 · 2022-07-07T15:20:25Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java

-                    main.auditorElector.isRunning());
-            assertTrue("Replication worker should be running",
-                    main.replicationWorker.isRunning());
+            await().atMost(1, SECONDS).untilAsserted(() ->


Remove the atMost logic and use the default most await time?

The default await time is 10s

hangc0276 · 2022-07-07T15:20:36Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java

-        assertTrue("Replication worker should be running",
-                main.replicationWorker.isRunning());
-
+        await().atMost(1, SECONDS).untilAsserted(()->


The same above

hangc0276 · 2022-07-07T15:22:55Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java

            sendthread.resume();

            // allow watcher thread to run
-            Thread.sleep(3000);
+            await().atMost(3, TimeUnit.SECONDS).await();


shoothzj · 2022-07-25T06:24:50Z

@CalvinKirs Hi, could you please rebase this PR. Also I think there is a typo in your PR title. using instead of Useing

StevenLuMT · 2022-07-25T17:24:29Z

This branch has conflicts that must be resolved

hangc0276 · 2022-07-26T03:37:47Z

ping @CalvinKirs, do you have time to resolve the above comments?

CalvinKirs · 2022-07-26T03:43:09Z

ping @CalvinKirs, do you have time to resolve the above comments?

Hi, I think I've resolved what the comments said, what else do I need to do? Except for dealing with conflicts.

zymap · 2022-07-30T02:16:16Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java

-                Thread.sleep(1000);
-            }
-        }
+        await().until(() -> main1.auditorElector.getAuditor() != null);


Why it's different than before? looks like the getCurrentAuditor() is different than getAuditor()

zymap · 2022-07-30T02:21:07Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java

            sendthread.suspend();
-            Thread.sleep(2 * conf.getZkTimeout());


Looks like this is part of the testing. It wants to resume the send thread after 2* zkTimeout to test the bookie should still working even if the zk session is timeout.

StevenLuMT · 2022-08-24T07:59:51Z

fix old workflow,please see #3455 for detail

shoothzj · 2024-05-04T02:10:42Z

@CalvinKirs Hi, thanks for your contribution, I want to move forward this PR. Could you please help rebase the code? :)

CalvinKirs added 4 commits July 7, 2022 11:15

Useing Awaitility for asynchroneous testing.

3cac8c6

Useing Awaitility for asynchroneous testing.

dadd0ab

fix deprecated method

42651a4

replace sleep in BookieZKExpireTest

248eb3b

nicoloboschi requested changes Jul 7, 2022

View reviewed changes

hangc0276 requested changes Jul 7, 2022

View reviewed changes

CalvinKirs added 2 commits July 10, 2022 15:24

restore timeout parameter

b4b7bf1

fix checkstyle

51f94d1

hangc0276 assigned CalvinKirs Jul 25, 2022

hangc0276 added this to the 4.16.0 milestone Jul 25, 2022

hangc0276 added the area/tests label Jul 25, 2022

hangc0276 modified the milestones: 4.16.0, 4.17.0 Jul 29, 2022

zymap reviewed Jul 30, 2022

View reviewed changes

eolivelli removed this from the 4.17.0 milestone May 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Useing Awaitility for asynchroneous testing. #3392

Useing Awaitility for asynchroneous testing. #3392

CalvinKirs commented Jul 7, 2022 •

edited

Loading

nicoloboschi Jul 7, 2022

nicoloboschi Jul 7, 2022

nicoloboschi Jul 7, 2022

hangc0276 Jul 7, 2022

CalvinKirs Jul 10, 2022

nicoloboschi Jul 7, 2022

nicoloboschi Jul 7, 2022

CalvinKirs Jul 10, 2022

hangc0276 Jul 7, 2022

CalvinKirs Jul 10, 2022

hangc0276 Jul 7, 2022

liuzhuang2017 Jul 25, 2022

hangc0276 Jul 7, 2022

shoothzj commented Jul 25, 2022

StevenLuMT commented Jul 25, 2022

hangc0276 commented Jul 26, 2022

CalvinKirs commented Jul 26, 2022

zymap Jul 30, 2022

zymap Jul 30, 2022

StevenLuMT commented Aug 24, 2022

shoothzj commented May 4, 2024

Useing Awaitility for asynchroneous testing. #3392

Are you sure you want to change the base?

Useing Awaitility for asynchroneous testing. #3392

Conversation

CalvinKirs commented Jul 7, 2022 • edited Loading

Descriptions of the changes in this PR:

Motivation

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoothzj commented Jul 25, 2022

StevenLuMT commented Jul 25, 2022

hangc0276 commented Jul 26, 2022

CalvinKirs commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StevenLuMT commented Aug 24, 2022

shoothzj commented May 4, 2024

CalvinKirs commented Jul 7, 2022 •

edited

Loading