On Mon, 29 Jun 2026 09:52:03 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review of this test-only change which addresses the >> issues in its parameterized tests, as noted here >> https://github.com/openjdk/jdk/pull/31123/changes#r3486082890? >> >> While at it, I also did a general clean up of the test and added comments to >> the test methods and the classes used in that test to clarify what's being >> covered by this test. >> >> The test continues to pass after this change and the test methods get run >> more number of times (as expected) due to fixing the parameters being >> generated for the parameterized tests. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > adjust test method comment test/jdk/java/io/Serializable/valueObjects/ValueSerializationTest.java line 141: > 139: void testSerializationFails(Object obj, Class<? extends Exception> > expectedException) > 140: throws Exception { > 141: serialize(obj, expectedException); Let's inline here. test/jdk/java/io/Serializable/valueObjects/ValueSerializationTest.java line 144: > 142: } > 143: > 144: static Stream<Arguments> serializingInstances() { If you have only one argument for your parameterized test, you can use a `Stream<Object>` or `Object[]` where each element is used as the single argument to the invoked test. This avoids `Arguments.of()` and the varargs casts. test/jdk/java/io/Serializable/valueObjects/ValueSerializationTest.java line 216: > 214: assertArrayEquals((Object[]) actual, (Object[]) obj); > 215: } else { > 216: assertEquals(actual, obj); JUnit places "expected" argument in the first position and the "actual" argument in the second position. So swap the arguments. test/jdk/java/io/Serializable/valueObjects/ValueSerializationTest.java line 285: > 283: } > 284: > 285: private static <T> byte[] serialize(T obj, Class<? extends > Exception> expectedException) I find that `expectedException` is always null in one site and always non-null in another site. I recommend inlining this method to the corresponding places given we have a very ugly branching `if (expectedException != null)`. Same remark for `deserialize`. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2590#discussion_r3492264635 PR Review Comment: https://git.openjdk.org/valhalla/pull/2590#discussion_r3492226322 PR Review Comment: https://git.openjdk.org/valhalla/pull/2590#discussion_r3492271872 PR Review Comment: https://git.openjdk.org/valhalla/pull/2590#discussion_r3492238607
