On Thu, 12 Mar 2026 17:28:25 GMT, David Beaumont <[email protected]> wrote:
>> 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)));
>> ...
>
> 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.
Either form work for me. If the current form, with the completion variable,
makes lines shorter, then maybe we should keep that ?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30203#discussion_r2930136187
