You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/07/07 03:22:42 UTC
[GitHub] [bookkeeper] CalvinKirs opened a new pull request, #3392: Useing Awaitility for asynchroneous testing.
CalvinKirs opened a new pull request, #3392:
URL: https://github.com/apache/bookkeeper/pull/3392
### 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
(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`
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#issuecomment-1194958100
ping @CalvinKirs, do you have time to resolve the above comments?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on a diff in pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#discussion_r933708589
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java:
##########
@@ -98,19 +96,7 @@ public void testAutoRecoverySessionLoss() throws Exception {
*/
ZKMetadataClientDriver zkMetadataClientDriver1 = startAutoRecoveryMain(main1);
ZooKeeper zk1 = zkMetadataClientDriver1.getZk();
-
- // Wait until auditor gets elected
- for (int i = 0; i < 10; i++) {
- try {
- if (main1.auditorElector.getCurrentAuditor() != null) {
- break;
- } else {
- Thread.sleep(1000);
- }
- } catch (IOException e) {
- Thread.sleep(1000);
- }
- }
+ await().until(() -> main1.auditorElector.getAuditor() != null);
Review Comment:
Why it's different than before? looks like the `getCurrentAuditor()` is different than `getAuditor()`
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java:
##########
@@ -73,35 +74,26 @@ conf, new TestBookieImpl(conf),
NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
new MockUncleanShutdownDetection());
server.start();
-
- int secondsToWait = 5;
- while (!server.isRunning()) {
- Thread.sleep(1000);
- if (secondsToWait-- <= 0) {
- fail("Bookie never started");
- }
- }
+ BookieServer finalServer = server;
+ await().atMost(5, SECONDS).until(finalServer::isRunning);
Thread sendthread = null;
threadCount = Thread.activeCount();
threads = new Thread[threadCount * 2];
threadCount = Thread.enumerate(threads);
for (int i = 0; i < threadCount; i++) {
- if (threads[i].getName().indexOf("SendThread") != -1
+ if (threads[i].getName().contains("SendThread")
&& !threadset.contains(threads[i])) {
sendthread = threads[i];
break;
}
}
assertNotNull("Send thread not found", sendthread);
-
sendthread.suspend();
- Thread.sleep(2 * conf.getZkTimeout());
Review Comment:
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.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#issuecomment-1194383432
This branch has conflicts that must be resolved
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a diff in pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on code in PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#discussion_r915562366
##########
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();
sendthread.resume();
// allow watcher thread to run
- Thread.sleep(3000);
+ await().atMost(3, TimeUnit.SECONDS).await();
Review Comment:
```
await().atMost(5, SECONDS).until(() -> server.isBookieRunning() && server.isRunning())
```
##########
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);
Review Comment:
this loop should become
```
await().atMost(5, SECONDS).until(() -> server.isRunning())
```
##########
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();
Review Comment:
we can leave the sleep here, we are not waiting for a particular condition
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java:
##########
@@ -146,20 +132,15 @@ public void testAutoRecoverySessionLoss() throws Exception {
* wait for some time for all the components of AR1 and AR2 are
* shutdown.
*/
- for (int i = 0; i < 10; i++) {
- if (!main1.auditorElector.isRunning() && !main1.replicationWorker.isRunning()
- && !main1.isAutoRecoveryRunning() && !main2.auditorElector.isRunning()
- && !main2.replicationWorker.isRunning() && !main2.isAutoRecoveryRunning()) {
- break;
- }
- Thread.sleep(1000);
- }
+ await().atMost(20, SECONDS).until(()->!main1.auditorElector.isRunning() && !main1.replicationWorker.isRunning()
Review Comment:
better to keep the same timeout (10 seconds)
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java:
##########
@@ -98,19 +96,7 @@ public void testAutoRecoverySessionLoss() throws Exception {
*/
ZKMetadataClientDriver zkMetadataClientDriver1 = startAutoRecoveryMain(main1);
ZooKeeper zk1 = zkMetadataClientDriver1.getZk();
-
- // Wait until auditor gets elected
- for (int i = 0; i < 10; i++) {
- try {
- if (main1.auditorElector.getCurrentAuditor() != null) {
- break;
- } else {
- Thread.sleep(1000);
- }
- } catch (IOException e) {
- Thread.sleep(1000);
- }
- }
+ await().atMost(20, SECONDS).until(() -> main1.auditorElector.getAuditor() != null);
Review Comment:
better to keep the same timeout (10 seconds)
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] CalvinKirs commented on a diff in pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on code in PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#discussion_r917395419
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java:
##########
@@ -49,11 +52,9 @@ public void testStartup() throws Exception {
AutoRecoveryMain main = new AutoRecoveryMain(confByIndex(0));
try {
main.start();
- Thread.sleep(500);
- assertTrue("AuditorElector should be running",
- main.auditorElector.isRunning());
- assertTrue("Replication worker should be running",
- main.replicationWorker.isRunning());
+ await().atMost(1, SECONDS).untilAsserted(() ->
Review Comment:
The default await time is 10s
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java:
##########
@@ -98,19 +96,7 @@ public void testAutoRecoverySessionLoss() throws Exception {
*/
ZKMetadataClientDriver zkMetadataClientDriver1 = startAutoRecoveryMain(main1);
ZooKeeper zk1 = zkMetadataClientDriver1.getZk();
-
- // Wait until auditor gets elected
- for (int i = 0; i < 10; i++) {
- try {
- if (main1.auditorElector.getCurrentAuditor() != null) {
- break;
- } else {
- Thread.sleep(1000);
- }
- } catch (IOException e) {
- Thread.sleep(1000);
- }
- }
+ await().atMost(20, SECONDS).until(() -> main1.auditorElector.getAuditor() != null);
Review Comment:
Thanks,updated
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Shoothzj commented on pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#issuecomment-1193633187
@CalvinKirs Hi, could you please rebase this PR. Also I think there is a typo in your PR title. `using` instead of `Useing`
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#issuecomment-1225339625
fix old workflow,please see #3455 for detail
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] liuzhuang2017 commented on a diff in pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
liuzhuang2017 commented on code in PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#discussion_r928470408
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java:
##########
@@ -66,12 +67,9 @@ public void testStartup() throws Exception {
public void testShutdown() throws Exception {
AutoRecoveryMain main = new AutoRecoveryMain(confByIndex(0));
main.start();
- Thread.sleep(500);
- assertTrue("AuditorElector should be running",
- main.auditorElector.isRunning());
- assertTrue("Replication worker should be running",
- main.replicationWorker.isRunning());
-
+ await().atMost(1, SECONDS).untilAsserted(()->
Review Comment:
+1
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#discussion_r916003323
##########
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();
sendthread.resume();
// allow watcher thread to run
- Thread.sleep(3000);
+ await().atMost(3, TimeUnit.SECONDS).await();
Review Comment:
+1
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java:
##########
@@ -49,11 +52,9 @@ public void testStartup() throws Exception {
AutoRecoveryMain main = new AutoRecoveryMain(confByIndex(0));
try {
main.start();
- Thread.sleep(500);
- assertTrue("AuditorElector should be running",
- main.auditorElector.isRunning());
- assertTrue("Replication worker should be running",
- main.replicationWorker.isRunning());
+ await().atMost(1, SECONDS).untilAsserted(() ->
Review Comment:
Remove the atMost logic and use the default most await time?
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AutoRecoveryMainTest.java:
##########
@@ -66,12 +67,9 @@ public void testStartup() throws Exception {
public void testShutdown() throws Exception {
AutoRecoveryMain main = new AutoRecoveryMain(confByIndex(0));
main.start();
- Thread.sleep(500);
- assertTrue("AuditorElector should be running",
- main.auditorElector.isRunning());
- assertTrue("Replication worker should be running",
- main.replicationWorker.isRunning());
-
+ await().atMost(1, SECONDS).untilAsserted(()->
Review Comment:
The same above
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] CalvinKirs commented on pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#issuecomment-1194961200
> 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.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] CalvinKirs commented on a diff in pull request #3392: Useing Awaitility for asynchroneous testing.
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on code in PR #3392:
URL: https://github.com/apache/bookkeeper/pull/3392#discussion_r917395456
##########
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();
sendthread.resume();
// allow watcher thread to run
- Thread.sleep(3000);
+ await().atMost(3, TimeUnit.SECONDS).await();
Review Comment:
Thx,done
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org