On Tue, 16 Dec 2025 22:14:27 GMT, Roger Riggs <rriggs@openjdk.org> 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"

The new tests scope is good. The current standard for tests is JUnit and it would be better to convert the test to JUnit before adding new tests.

Thank you for the detailed feedback, @RogerRiggs and @liach! I've addressed the following: - [x] Expanded wildcard imports to individual imports (as requested by @RogerRiggs) - [x] Simplified test messages to just identify the test (as both reviewers suggested) - [x] Used Assertions.fail() where appropriate (as suggested by @RogerRiggs) - [x] Kept assertTrue/assertFalse for isNull API tests (as @liach correctly noted) - [x] Simplified assertThrows message argument to just testFuncName (as suggested by @RogerRiggs) Regarding wildcard imports and Assertions.fail vs exceptions: I've made the changes as requested, but I understand from @liach's comment that these are acceptable either way. I've followed @RogerRiggs's preferences for consistency. The changes have been pushed. Please review again. ------------- PR Comment: https://git.openjdk.org/jdk/pull/28845#issuecomment-3669913387

On Tue, 16 Dec 2025 23:16:42 GMT, eunbin son <duke@openjdk.org> 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:

8373661: Convert BasicObjectsTest to JUnit format

- Converted all 17 test methods from jtreg main-method format to JUnit - Removed main() method - Added @run junit annotation - All tests follow OpenJDK JUnit testing patterns

Thanks to @RogerRiggs for the feedback.

test/jdk/java/util/Objects/BasicObjectsTest.java line 305:

303: RuntimeException.class, 304: () -> Objects.requireNonNull(null, () -> { 305: throw new RuntimeException("Supplier exception");

Instead of detecting by message, you can allocate an exception outside of the lambda, throw it here, and assertSame your exception with the assertThrows returned exception. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28845#discussion_r2625097382

On Wed, 17 Dec 2025 21:03:48 GMT, Roger Riggs <rriggs@openjdk.org> wrote:

...

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.

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.

I think this is testing `isNull` API and shouldn't be simplified. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28845#discussion_r2628817056

On Wed, 17 Dec 2025 22:43:44 GMT, Chen Liang <liach@openjdk.org> wrote:

...

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.

I agree with Roger's points about simplifying some test messages. Sometimes we don't even write test messages if the subject is clear from the test code context, because the exceptions usually include the line numbers already. However, these are not critical.

However, I think Roger might be too strict on imports and Assertions.fail vs exceptions - these are acceptable because they don't affect the test quality, so I am not concerned.

Thank you for the feedback, @liach and @RogerRiggs! I've addressed your concerns: - [x] Updated description contents and formatting , copyright year (as requested by @RogerRiggs) - [x] Removed exception message tests to allow API flexibility (as suggested by @liach) - [x] Removed verbose Javadoc and inline comments (as suggested by @liach) - [x] Simplified test messages to use method names only The tests now follow OpenJDK patterns where only exception types are tested, not messages. The code is more concise and maintainable. The changes have been pushed. Please review again. ------------- PR Comment: https://git.openjdk.org/jdk/pull/28845#issuecomment-3677444777

On Thu, 18 Dec 2025 11:54:41 GMT, eunbin son <duke@openjdk.org> 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: Address code style feedback

- Expanded wildcard imports to individual imports - Simplified test messages to identify tests only - Used Assertions.fail() where appropriate - Kept assertTrue/assertFalse for isNull API tests

Thanks to @RogerRiggs and @liach for the feedback.

Comment on the description contents and formatting: Skara (the automation for OpenJDK PRs) adds its own content (using Markdown) at the end to document the progress and steps. The final bit of `preformatted` text in the description does not seem to be closed properly so the Skara comment shows up as raw Markdown. ------------- PR Comment: https://git.openjdk.org/jdk/pull/28845#issuecomment-3675814936

On Thu, 18 Dec 2025 11:54:41 GMT, eunbin son <duke@openjdk.org> 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: Address code style feedback

- Expanded wildcard imports to individual imports - Simplified test messages to identify tests only - Used Assertions.fail() where appropriate - Kept assertTrue/assertFalse for isNull API tests

Thanks to @RogerRiggs and @liach for the feedback.

The copyright date needs to be updated to 2025. Tests should be as readable and maintainable as the other source code. The coding styles vary a bit, having accumulated by many authors over the years. Changes should follow the existing coding style in the file, package, module, etc. The changes look good; thanks for taking on the JUnit conversion and the addition of tests. ------------- Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/28845#pullrequestreview-3599377100

On Thu, 18 Dec 2025 11:54:41 GMT, eunbin son <duke@openjdk.org> 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: Address code style feedback

- Expanded wildcard imports to individual imports - Simplified test messages to identify tests only - Used Assertions.fail() where appropriate - Kept assertTrue/assertFalse for isNull API tests

Thanks to @RogerRiggs and @liach for the feedback.

If an LLM was used in this PR, please provide the model's name and version. ------------- PR Comment: https://git.openjdk.org/jdk/pull/28845#issuecomment-3676330769

On Sat, 20 Dec 2025 06:08:11 GMT, eunbin son <duke@openjdk.org> wrote:

but all changes have been reviewed and refined based on maintainer feedback

OpenJDK reviewers are not obligated to babysit a contribution back and forth for multiple iterations to get the patch right, regardless if it is from a bot or a human. For example, I asked to run a command to fix the copyright year and to remove redundant message, both of which are done incorrectly. We simply cannot afford extra review cycles in this. ------------- PR Comment: https://git.openjdk.org/jdk/pull/28845#issuecomment-3677473559

On Thu, 18 Dec 2025 11:54:41 GMT, eunbin son <duke@openjdk.org> 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: Address code style feedback

- Expanded wildcard imports to individual imports - Simplified test messages to identify tests only - Used Assertions.fail() where appropriate - Kept assertTrue/assertFalse for isNull API tests

Thanks to @RogerRiggs and @liach for the feedback.

I don't think we can accept this patch as-is - Even though tests would "increase coverage", this patch adds a lot of messages that are probably never printed that we cannot really validate them. In particular, this patch is verbose - a lot of the message strings and comments are simply repeating what is immediately clear from the code. Such spam of information reminds people of large language models, which tend to generate code that seems rich on the first glance but ultimately becomes problematic in the long run. For example, we wish for freedom to update exception messages in the future, while the added tests are restricting the messages our APIs can return. ------------- Changes requested by liach (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/28845#pullrequestreview-3600024617

On Sun, 21 Dec 2025 23:41:42 GMT, Florent Guillaume <duke@openjdk.org> wrote:

...

eunbin son has updated the pull request incrementally with one additional commit since the last revision:

8373661: Address liach's feedback on copyright year and redundant messages

I apologize for the earlier incorrect implementation. As @liach noted, "I asked to run a command to fix the copyright year and to remove redundant message, both of which are done incorrectly." I have now properly addressed both concerns:

- Copyright year: Updated to 2025 (the copyright year was already set in the previous commit, but I should have used the update_copyright_year.sh script as requested)

- Redundant messages: Removed all exception message tests, verbose inline comments, and simplified all test messages to use only test method names

Thanks to @liach for the detailed feedback.

So much time and power extended on reviewing a PR for one test file whose update changes essentially nothing to the JDK... Using AI tools on the JDK for no other purpose than using AI... There are other priorities imho. This PR should be closed for lack of relevance.

Thank you for the review and for pointing out the priority concerns. @efge My intention was simply to make a small, good-faith contribution to the JDK test code, not to add review burden or to promote the use of AI tools. I understand if this change is not considered relevant. I appreciate the time you’ve already spent on this. @liach @RogerRiggs ------------- PR Comment: https://git.openjdk.org/jdk/pull/28845#issuecomment-3680215142