On Wed, 11 Mar 2026 23:04:57 GMT, David Beaumont <[email protected]> wrote:
>> 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)? 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30203#discussion_r2924222333
