On Wed, 11 Mar 2026 16:14:38 GMT, Christian Stein <[email protected]> wrote:

> Please review this change to use JUnit Jupiter API in `jdk/com/sun` tests.

test/jdk/com/sun/jndi/ldap/LdapTimeoutTest.java line 189:

> 187:             server.start();
> 188:             server.starting().join();
> 189:             Executable completion =

Weird not to inline the lambda.
But more importantly, it feels really odd to have an assertThrows() calling a 
complex *assert* method.
Typically you want assertThrows() to take exactly one statement lambda of the 
code under test, otherwise you risk false-positive passing the test when the 
exception you expect is thrown, but not by the code you think you're testing.
If I were looking at this I'd be digging into assertCompletion() and seeing if 
things could be rearranged to only test a minimal amount of code here.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/30203#discussion_r2921415435

Reply via email to