On Wed, 11 Mar 2026 22:58:49 GMT, David Beaumont <[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. Looking at assertCompletion(), which is nearly 60 lines of code, I'd find it impossible to reason about what's actually being tested here. All these test cases expect failure, and all by `NamingException`, which should be being thrown by the non-test code (which here seems to be the closure you're passing in). Can you just do: NamingException e = assertThrows(NamingException.class, () -> new InitialDirContext(env)); and get the same result? And if not, why not (i.e. what's tangled up in the test code that makes this not work)? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30203#discussion_r2921432203
