On Thu, 12 Mar 2026 15:35:28 GMT, Christian Stein <[email protected]> wrote:
>> This is checking timeouts. We want to check that NamingException is thrown,
>> and that it's thrown within the expected time frame - not too early and not
>> too late.
>> We obviously don't want to repeat this logic in every test.
>
> Does inlining the "complex _assert_ method" help? Like so:
>
>
> Index: test/jdk/com/sun/jndi/ldap/LdapTimeoutTest.java
> IDEA additional info:
> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
> <+>UTF-8
> ===================================================================
> diff --git a/test/jdk/com/sun/jndi/ldap/LdapTimeoutTest.java
> b/test/jdk/com/sun/jndi/ldap/LdapTimeoutTest.java
> --- a/test/jdk/com/sun/jndi/ldap/LdapTimeoutTest.java (revision Staged)
> +++ b/test/jdk/com/sun/jndi/ldap/LdapTimeoutTest.java (date 1773329518733)
> @@ -35,7 +35,6 @@
>
> import org.junit.jupiter.api.BeforeEach;
> import org.junit.jupiter.api.Test;
> -import org.junit.jupiter.api.function.Executable;
>
> import javax.naming.Context;
> import javax.naming.NamingException;
> @@ -186,11 +185,10 @@
> env.put(Context.PROVIDER_URL, urlTo(server));
> server.start();
> server.starting().join();
> - Executable completion =
> + NamingException e = assertThrows(NamingException.class,
> () -> assertCompletion(CONNECT_MILLIS,
> - 2 * CONNECT_MILLIS + TOLERANCE,
> - () -> new InitialDirContext(env));
> - NamingException e = assertThrows(NamingException.class,
> completion);
> + 2 * CONNECT_MILLIS + TOLERANCE,
> + () -> new InitialDirContext(env)));
> String msg = e.getMessage();
> assertTrue(msg != null && msg.contains("timeout")
> &&
> msg.contains(String.valueOf(CONNECT_MILLIS)),
> @@ -210,11 +208,10 @@
> InitialDirContext ctx = new InitialDirContext(env);
> SearchControls scl = new SearchControls();
> scl.setSearchScope(SearchControls.SUBTREE_SCOPE);
> - Executable completion =
> + NamingException e = assertThrows(NamingException.class,
> () -> assertCompletion(READ_MILLIS,
> - READ_MILLIS + TOLERANCE,
> - () ->
> ctx.search("ou=People,o=JNDITutorial", "(objectClass=*)", scl));
> - NamingException e = assertThrows(NamingException.class,
> completion);
> + READ_MILLIS + TOLERANCE,
> + () -> ctx.search("ou=People,o=JNDITutorial",
> "(objectClass=*)", scl)));
> String msg = e.getMessage();
> assertTrue(msg != null && msg.contains("t...
The "complex assert method" is the 60-line "assertCompletion" method (not the
immediate lambda), which is *test* code in this class, rather than any
code-under-test.
I'm just wary of negative tests with overly broad scope (e.g. test that this
N-statement sequence of calls throws some exception *somewhere*). These make it
hard for a reader to reason about whether the exception came from the expected
place or not, and allows the test to pass when it shouldn't (i.e. a
false-positive).
Thanks to Daniel's comment, I realise that this case isn't quite that, because
additional specific testing *is* being done on the resulting exception, so if
it were thrown erroneously by something else, that test almost certainly
wouldn't continue to pass.
In light of that, I'm fine with it as-is (though an explanatory comment or two
wouldn't go amiss - e.g. on the first example at line it's used at line 189 or
the test method JavaDoc).
As it is, if I were investigating a failure of this test I wouldn't really
understand where to start looking.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30203#discussion_r2926220927
