You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/11/30 18:37:10 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
DomGarguilo opened a new pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816
Did my best to resolve #1763. Replaced the assert statements with an if statement so that they could be repeated if they don't pass immediately. The test will repeat until passed or until the added timeout occurs. As suggested, I left the assert statement after the loop which verifies the thread pool size remains unchanged after the previous operations. Not sure if I should have kept the other asserts as well but I don't see the point if the same conditions were verified already. Please suggest any changes or improvements.
----------------------------------------------------------------
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
[GitHub] [accumulo] DomGarguilo commented on pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816#issuecomment-735988574
Oops, I see I was off-base with this one. Will do my best to correct these issues with your suggestions in mind +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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816#issuecomment-736073587
> Not sure if I should extract the code containing the while(true) and create a separate method in order to reuse that portion of code.
No. These changes aren't big enough to warrant that.
> Not sure if I should add a local variable for the timeout length (even though there is one test case that requires a longer timeout).
No. Your timeouts are fine.
> I added a timeout to testSingleton() which probably doesn't need it so I am unsure whether I should remove it or keep it in.
In general, we try to make minimal unrelated changes, to make it easier to review, but it's often a judgement call. In this case, I would leave it in. It's a tiny change, and is related to your adding a timeout to all the other test cases.
----------------------------------------------------------------
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
[GitHub] [accumulo] DomGarguilo edited a comment on pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
DomGarguilo edited a comment on pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816#issuecomment-735988574
Oops, I see I was off-base with this one. Will do my best to correct these issues with your suggestions in mind :+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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816#discussion_r532918325
##########
File path: server/base/src/test/java/org/apache/accumulo/server/util/time/SimpleTimerTest.java
##########
@@ -70,35 +69,50 @@ public void run() {
}
}
- @Test
+ @Test(timeout = 5000)
public void testOneTimeSchedule() throws InterruptedException {
AtomicInteger i = new AtomicInteger();
Incrementer r = new Incrementer(i);
t.schedule(r, DELAY);
Thread.sleep(DELAY + PAD);
- assertEquals(1, i.get());
+ while (true) {
+ if (i.get() == 1) {
+ break;
+ }
+ Thread.sleep(PAD);
+ }
Review comment:
Just realized that you can simplify this to `while (i.get() != 1) { sleep }`; you can do similar things for the other tests as well.
----------------------------------------------------------------
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
[GitHub] [accumulo] DomGarguilo commented on pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816#issuecomment-736053490
I implemented the fix in the correct test cases. There were a few things that I was unsure about though. Not sure if I should extract the code containing the while(true) and create a separate method in order to reuse that portion of code. Not sure if I should add a local variable for the timeout length (even though there is one test case that requires a longer timeout). I added a timeout to testSingleton() which probably doesn't need it so I am unsure whether I should remove it or keep it in.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii merged pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816#issuecomment-736088592
Thanks for improving the test, @DomGarguilo !
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816#discussion_r532828863
##########
File path: server/base/src/test/java/org/apache/accumulo/server/util/time/SimpleTimerTest.java
##########
@@ -98,11 +97,15 @@ public void testFailure() throws InterruptedException {
assertTrue(r.wasRun);
}
- @Test
- public void testSingleton() {
- assertEquals(1, SimpleTimer.getInstanceThreadPoolSize());
- SimpleTimer t2 = SimpleTimer.getInstance(2);
- assertSame(t, t2);
+ @Test(timeout = 5000)
+ public void testSingleton() throws InterruptedException {
+ while (true) {
+ SimpleTimer t2 = SimpleTimer.getInstance(2);
+ if((SimpleTimer.getInstanceThreadPoolSize() == 1) && (t == t2)) {
+ break;
+ }
+ Thread.sleep(PAD);
+ }
Review comment:
This is the right strategy (albeit with a few extra parenthesis than necessary on that `if` statement :wink: )
----------------------------------------------------------------
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
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816#discussion_r532838657
##########
File path: server/base/src/test/java/org/apache/accumulo/server/util/time/SimpleTimerTest.java
##########
@@ -98,11 +97,15 @@ public void testFailure() throws InterruptedException {
assertTrue(r.wasRun);
}
- @Test
- public void testSingleton() {
- assertEquals(1, SimpleTimer.getInstanceThreadPoolSize());
- SimpleTimer t2 = SimpleTimer.getInstance(2);
- assertSame(t, t2);
+ @Test(timeout = 5000)
+ public void testSingleton() throws InterruptedException {
+ while (true) {
+ SimpleTimer t2 = SimpleTimer.getInstance(2);
+ if((SimpleTimer.getInstanceThreadPoolSize() == 1) && (t == t2)) {
+ break;
+ }
+ Thread.sleep(PAD);
+ }
Review comment:
Oops, I see I was off-base with this one. Will do my best to correct these issues with your suggestions in mind :+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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #1816: Fixes #1763 - Flaky test: SimpleTimerTest
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #1816:
URL: https://github.com/apache/accumulo/pull/1816#discussion_r532920000
##########
File path: server/base/src/test/java/org/apache/accumulo/server/util/time/SimpleTimerTest.java
##########
@@ -70,35 +69,50 @@ public void run() {
}
}
- @Test
+ @Test(timeout = 5000)
public void testOneTimeSchedule() throws InterruptedException {
AtomicInteger i = new AtomicInteger();
Incrementer r = new Incrementer(i);
t.schedule(r, DELAY);
Thread.sleep(DELAY + PAD);
- assertEquals(1, i.get());
+ while (true) {
+ if (i.get() == 1) {
+ break;
+ }
+ Thread.sleep(PAD);
+ }
Review comment:
Was considering that as well. I will add this change.
----------------------------------------------------------------
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