ISPN-7141 LimitedExecutorTest.testConcurrencyLimit random failures #4633
Conversation
danberindei
force-pushed
the
ISPN-7141_LimitedExecutorTest
branch
from
9abae99
to
a533911
Compare
| assertFalse(cf2.isDone()); | ||
|
|
||
| latch.countDown(); | ||
| assertEquals("bla", cf1.get(10, SECONDS)); | ||
| assertEquals("bla", cf2.get(10, SECONDS)); | ||
| eventuallyEquals(0, executor::getActiveCount); |
There was a problem hiding this comment.
Do you still need the two threads with this eventuallyEquals? I would rather move that on the beginning of the test method, as this line may not be reached upon test failure (and you don't want to fail the next test method).
There was a problem hiding this comment.
Yes, I realized I actually needed 2 threads for testConcurrencyLimit().
But I added another eventuallyEquals() call at the beginning of the test methods that use the executor.
There was a problem hiding this comment.
Okay, so the 2 threads are there to allow the possibility that the tasks will run in parallel, right? But what do you actually test here? The test confirms that eventually both tasks will be executed, but there's nothing testing the level of parallelism.
There was a problem hiding this comment.
I've added javadocs to the test methods. testConcurrencyLimit() uses regular execute(), so it needs 2 threads to check that one task really blocks the other. This test method doesn't need 2 threads, because it uses executeAsync(), and the 1st task blocks the executor without using any threads.
There was a problem hiding this comment.
I would still add a check into the second task verifying that the first task has finished; otherwise you could replace LimitedExecutor with some executor that starts executing the second task a bit later (than the 10 ms) and it would still succeed.
There was a problem hiding this comment.
I guess having written LimitedExecutor myself, I didn't think it could ever delay a task without a good reason :)
Anyway, I've updated the test to better deal with the possibility.
danberindei
force-pushed
the
ISPN-7141_LimitedExecutorTest
branch
5 times, most recently
from
1db88dd
to
066e711
Compare
danberindei
force-pushed
the
ISPN-7141_LimitedExecutorTest
branch
2 times, most recently
from
802fc51
to
2bc98b0
Compare
danberindei
force-pushed
the
ISPN-7141_LimitedExecutorTest
branch
from
2bc98b0
to
a26b15b
Compare
danberindei
force-pushed
the
ISPN-7141_LimitedExecutorTest
branch
from
a26b15b
to
47cd309
Compare
|
The tests would work with single |
* Increase the thread pool size to 2 for testConcurrencyLimit * Wait for the executor to settle before/after each test
danberindei
force-pushed
the
ISPN-7141_LimitedExecutorTest
branch
from
47cd309
to
946f3fa
Compare
|
Pulling.. |
|
Integrated into master, thanks @danberindei ! |
https://issues.jboss.org/browse/ISPN-7141