RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Pavel Rappo]

Martin,

> On 10 Sep 2019, at 17:40, Martin Buchholz <martinrb at google.com> wrote:
> 
> Here's a canonical example of an "expected timeout" test that seems natural and readable to me.
> timeoutMillis() returns 12ms, adjusted by timeout factor.  Ldap might need a bigger value.
> 
> 
>     /**
>      * timed await times out if not counted down before timeout
>      */
>     public void testAwaitTimeout() throws InterruptedException {
>         final CountDownLatch l = new CountDownLatch(1);
>         Thread t = newStartedThread(new CheckedRunnable() {
>             public void realRun() throws InterruptedException {
>                 assertEquals(1, l.getCount());
> 
>                 long startTime = System.nanoTime();
>                 assertFalse(l.await(timeoutMillis(), MILLISECONDS));
>                 assertTrue(millisElapsedSince(startTime) >= timeoutMillis());
> 
>                 assertEquals(1, l.getCount());
>             }});
> 
>         awaitTermination(t);
>         assertEquals(1, l.getCount());
>     }


I agree that this example [1] looks readable (depending on a reader, of course). I think it looks readable mostly because it is very explicit. However, in a domain other than concurrency we are not usually concerned with this level of detail. In such cases I would prefer to have a small set of focused methods that would allow me to explain my intentions succinctly and only once, taking care of all the low-level details.

As for the internal mechanics, I can see that this example [2]:

a. is not using any synchronization aid to make sure both threads wait for each other (probably, the timeout value accommodates that)
b. uses a number of tiny low-level methods like newStartedThread, awaitTermination, millisElapsedSince, manual time assertions, etc.
c. has assertions spread across 2 threads

(b) probably allows you to reuse components in places unrelated to timeouts. At the same time you don't seem to have any higher-level reusable component (i.e. my guess is that this code is more or less repeated in other places in that test suite, which is not necessarily a bad thing).

However, I fully agree with you that this functionality should be a utility method of the test library.

-------------------------
[1] I assume this is an excerpt from CountDownLatchTest.java. If so, then for the reader's convenience this method could be seen in its context at http://hg.openjdk.java.net/jdk/jdk/file/d52f77f0acb5/test/jdk/java/util/concurrent/tck/CountDownLatchTest.java#l198
[2] I'm not saying that those things are wrong or that I disagree with any of that. It's just an observation from reading this example.