On Wed, 17 Dec 2025 11:50:53 GMT, eunbin son <[email protected]> wrote:
>> ## Summary >> Adds comprehensive edge case tests for `Objects.requireNonNull`, >> `requireNonNullElse`, and `requireNonNullElseGet` methods to improve >> test coverage. >> >> ## Problem >> The current test suite for `Objects.requireNonNull` methods covers >> basic cases but lacks edge case coverage. >> >> ## Solution >> This PR adds tests for the following edge cases: >> - requireNonNull with null Supplier parameter >> - requireNonNull with Supplier that throws exception >> - requireNonNullElse with both arguments null >> - requireNonNullElseGet with null supplier >> - requireNonNullElseGet with supplier returning null >> >> ## Issue >> Fixes JDK-8373661 >> >> **JBS Issue Link**: >> https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-8373661 >> >> ## Type of Change >> - [x] Test addition/modification >> - [ ] Bug fix >> - [ ] New feature >> - [ ] Documentation improvement >> - [ ] Refactoring >> >> ## Testing >> >> make test TEST="jtreg:test/jdk/java/util/Objects" > > eunbin son has updated the pull request incrementally with one additional > commit since the last revision: > > 8373661: Use assertSame for exception instance verification > > Updated testRequireNonNullWithSupplierThrowingException to allocate > the exception outside of the lambda and use assertSame to verify > the same exception instance is thrown, as suggested by @liach. > > This provides a more precise verification that the exception from > the supplier is thrown directly, not wrapped. > > Thanks to @liach for the feedback. Thanks for the conversion. It may be useful to force some of the asserts to fail and check that the error messages are readable. test/jdk/java/util/Objects/BasicObjectsTest.java line 31: > 29: */ > 30: > 31: import static org.junit.jupiter.api.Assertions.*; OpenJdk style is to avoid wildcard imports, especially for small numbers. test/jdk/java/util/Objects/BasicObjectsTest.java line 34: > 32: import org.junit.jupiter.api.Test; > 33: import java.util.*; > 34: import java.util.function.*; Please expand these too, though they are pre-existing. test/jdk/java/util/Objects/BasicObjectsTest.java line 49: > 47: assertEquals(expected, result, > 48: String.format("When equating %s to %s, got %b instead > of %b", > 49: a, b, result, expected)); Since the JUnit assertEquals already prints the expected and actual, please simplify, retaining a message that identifies the test. test/jdk/java/util/Objects/BasicObjectsTest.java line 134: > 132: @Override > 133: public String toString() { > 134: throw new RuntimeException(); In Junit, this should probably call Assertions.fail(...) test/jdk/java/util/Objects/BasicObjectsTest.java line 208: > 206: NullPointerException.class, > 207: () -> testFunc.apply(null), > 208: testFuncName + " should throw NPE"); AssertThrows will already include the message that it should throw; the message argument can be simplified to just the testFuncName. test/jdk/java/util/Objects/BasicObjectsTest.java line 218: > 216: "isNull(null) should return true"); > 217: assertFalse(Objects.isNull(Objects.class), > 218: "isNull(Objects.class) should return false"); Could be simplifed with Assertions.assertNull or assertNonNull. Here and elsewhere in this test. ------------- PR Review: https://git.openjdk.org/jdk/pull/28845#pullrequestreview-3589611390 PR Review Comment: https://git.openjdk.org/jdk/pull/28845#discussion_r2628605220 PR Review Comment: https://git.openjdk.org/jdk/pull/28845#discussion_r2628606965 PR Review Comment: https://git.openjdk.org/jdk/pull/28845#discussion_r2628616800 PR Review Comment: https://git.openjdk.org/jdk/pull/28845#discussion_r2628625305 PR Review Comment: https://git.openjdk.org/jdk/pull/28845#discussion_r2628633438 PR Review Comment: https://git.openjdk.org/jdk/pull/28845#discussion_r2628642090
