On Wed, 29 Apr 2026 13:26:37 GMT, sstremler <[email protected]> wrote:
>> Refactor stream-related tests from TestNG to JUnit. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > sstremler has updated the pull request incrementally with one additional > commit since the last revision: > > Flip the order of actual and expected arguments in JUnit assertEquals Hopefully these comments are useful, if not please let me know. These are only suggestions; I am not an OpenJDK member, so feel free to ignore my comments. So far I only had a look at 50% of the files. If you found my comments useful, please let me know. I will see if I can have a look at the remaining files. test/jdk/java/util/regex/PatternStreamTest.java line 226: > 224: } catch (ConcurrentModificationException e) { > 225: // Should reach here > 226: } Here and below, convert the `try { ... fail(); } catch (expected) { }` to JUnit's `assertThrows`? test/jdk/java/util/stream/boottest/TEST.properties line 1: > 1: # This file identifies root(s) of the test-ng hierarchy. Is this comment, mentioning "test-ng", still correct? test/jdk/java/util/stream/boottest/java.base/java/util/stream/DoubleNodeTest.java line 130: > 128: @MethodSource("nodes") > 129: public void testAsArray(double[] array, Node.OfDouble n) { > 130: assertArrayEquals(n.asPrimitiveArray(), array); All these `assertArrayEquals` in this class have 'expected' and 'actual' interchanged? test/jdk/java/util/stream/boottest/java.base/java/util/stream/FlagOpTest.java line 77: > 75: > 76: if (downstream != null) { > 77: assertTrue(flags == downstream.wrapFlags); Use `assertEquals`? test/jdk/java/util/stream/boottest/java.base/java/util/stream/FlagOpTest.java line 287: > 285: } > 286: > 287: } Maybe out-of-scope, but it seems code duplication could be reduced here since the only difference is `parKnown` vs. `serKnown`: EnumSet<StreamOpFlag> known = parallel ? parKnown : serKnown; for (StreamOpFlag f : known) { assertTrue(f.isKnown(flags), String.format("Flag %s is not known, but should be known.", f.toString())); } test/jdk/java/util/stream/boottest/java.base/java/util/stream/IntNodeTest.java line 130: > 128: @MethodSource("nodes") > 129: public void testAsArray(int[] array, Node.OfInt n) { > 130: assertArrayEquals(n.asPrimitiveArray(), array); For `assertArrayEquals` calls in this test class 'expected' and 'actual' are interchanged? test/jdk/java/util/stream/boottest/java.base/java/util/stream/LongNodeTest.java line 130: > 128: @MethodSource("nodes") > 129: public void testAsArray(long[] array, Node.OfLong n) { > 130: assertArrayEquals(n.asPrimitiveArray(), array); `assertArrayEquals` 'expected' and 'actual' are interchanged? test/jdk/java/util/stream/boottest/java.base/java/util/stream/NodeTest.java line 107: > 105: @MethodSource("nodes") > 106: public void testAsArray(Integer[] array, Node<Integer> n) { > 107: > assertArrayEquals(n.asArray(LambdaTestHelpers.integerArrayGenerator), array); `assertArrayEquals` 'expected' and 'actual' interchanged? test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferDoubleTest.java line 105: > 103: > 104: for (int i = 0; i < TEST_SIZE; i++) { > 105: assertEquals((double) i, sb.get(i), Double.toString(i)); `Double.toString` call here might be redundant? Standard assertion error message will include that. test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferIntTest.java line 104: > 102: > 103: for (int i = 0; i < TEST_SIZE; i++) { > 104: assertEquals(i, sb.get(i), Integer.toString(i)); `Integer.toString` call is redundant? Will be included in assertion error message by default? test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferIntegerTest.java line 115: > 113: > 114: for (int i = 0; i < TEST_SIZE; i++) { > 115: assertEquals((Integer) i, sb.get(i), Integer.toString(i)); `Integer.toString` call is redundant? Will be included in assertion error message by default? test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferLongTest.java line 104: > 102: > 103: for (int i = 0; i < TEST_SIZE; i++) { > 104: assertEquals(i, sb.get(i), Long.toString(i)); `Long.toString` call is redundant? Will be included in assertion error message by default? test/jdk/java/util/stream/boottest/java.base/java/util/stream/StreamFlagsTest.java line 52: > 50: assertTrue(flag.isKnown(value)); > 51: for (StreamOpFlag flag : clearFlags) > 52: assertTrue(!flag.isKnown(value)); Use `assertFalse`? test/jdk/java/util/stream/boottest/java.base/java/util/stream/StreamReuseTest.java line 61: > 59: else > 60: throw new AssertionError("Unexpected exception " + > e.getClass(), e); > 61: } Use `assertThrows`? assertThrows(exception, () -> second.apply(stream), text + " (seq)"); test/jdk/java/util/stream/boottest/java.base/java/util/stream/StreamReuseTest.java line 69: > 67: fail(text + " (par)"); > 68: } > 69: catch (Throwable e) { Use `assertThrows`? test/jdk/java/util/stream/test/org/openjdk/tests/java/lang/invoke/DeserializeMethodTest.java line 49: > 47: if (expectedPresent) > 48: fail("Expected to find $deserializeLambda$ in " + clazz); > 49: } Maybe out-of-scope, but I am wondering if separating the `try-catch` and the `expectedPresent` check would make it easier to read? boolean hasMethod; try { clazz.getDeclaredMethod("$deserializeLambda$", SerializedLambda.class); hasMethod = true; } catch (NoSuchMethodException e) { hasMethod = false; } assertEquals(expectedPresent, hasMethod); test/jdk/java/util/stream/test/org/openjdk/tests/java/lang/invoke/SerializedLambdaTest.java line 81: > 79: catch (NotSerializableException e) { > 80: // success > 81: } Use `assertThrows`? test/jdk/java/util/stream/test/org/openjdk/tests/java/lang/invoke/SerializedLambdaTest.java line 105: > 103: assertSerial(pred, > 104: p -> { > 105: assertTrue(p instanceof Predicate); Use `assertInstanceOf` here and in the remainder of this test? test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorsTest.java line 259: > 257: throws ReflectiveOperationException { > 258: if (!clazz.isAssignableFrom(value.getClass())) > 259: fail(String.format("Class mismatch in > ToCollectionAssertion: %s, %s", clazz, value.getClass())); Here and for the other `isAssignableFrom` checks below, could use `assertInstanceOf`? test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorsTest.java line 287: > 285: Optional<U> reduced = > source.get().map(mapper).reduce(reducer); > 286: if (value == null) > 287: assertTrue(!reduced.isPresent()); `assertFalse`? test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorsTest.java line 500: > 498: new ToMapAssertion<>(keyFn, > valueFn, op, HashMap.class)); > 499: if (dataAsList.size() != dataAsSet.size()) > 500: fail("Expected ISE on input with duplicates"); `assertEquals`? assertEquals(dataAsList.size(), dataAsSet.size(), "Expected ISE on input with duplicates"); test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorsTest.java line 504: > 502: catch (IllegalStateException e) { > 503: if (dataAsList.size() == dataAsSet.size()) > 504: fail("Expected no ISE on input without duplicates"); `assertNotEquals`? assertNotEquals(dataAsList.size(), dataAsSet.size(), "Expected ISE on input with duplicates"); test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorsTest.java line 524: > 522: if (dataAsList.size() == dataAsSet.size()) > 523: fail("Expected no ISE on input without duplicates"); > 524: } Same as above, use `assertEquals` / `assertNotEquals` here? test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorsTest.java line 787: > 785: fail("Expecting immutable result"); > 786: } > 787: catch (UnsupportedOperationException ignored) { } `assertThrows`? test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/DistinctOpTest.java line 207: > 205: assertEquals(1, l.size()); > 206: > assertEquals(System.identityHashCode(expectedElement), > 207: System.identityHashCode(l.get(0))); Instead of checking `identityHashCode`, should this maybe use `assertSame`? (not sure though) test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/DoublePrimitiveOpsTests.java line 84: > 82: { > 83: double[] array = Arrays.stream(content).sorted().toArray(); > 84: assertArrayEquals(array, sortedContent); Here and below for `assertArrayEquals` calls, 'expected' and 'actual' are interchanged? test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/DoublePrimitiveOpsTests.java line 135: > 133: { > 134: double[] actual = DoubleStream.iterate(1.0, i -> i + > 1.0).limit(9).toArray(); > 135: assertTrue(Arrays.equals(expected, actual)); Use `assertArrayEquals`? test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/DoublePrimitiveOpsTests.java line 140: > 138: { > 139: double[] actual = LongStream.range(1, > 100).parallel().asDoubleStream().limit(9).toArray(); > 140: assertTrue(Arrays.equals(expected, actual)); Use `assertArrayEquals`? (and fix indentation) test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/GroupByOpTest.java line 157: > 155: static void assertObjectEquals(Object a, Object b) { > 156: assertTrue(Objects.equals(a, b)); > 157: } Is this custom method really needed? Isn't that equivalent to `assertEquals`? test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/IntPrimitiveOpsTests.java line 132: > 130: { > 131: int[] array = IntStream.range(1, 10).map(i -> i * > 2).toArray(); > 132: assertArrayEquals(array, new int[]{2, 4, 6, 8, 10, 12, 14, > 16, 18}); Here and below `assertArrayEquals` arguments are interchanged test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/IntPrimitiveOpsTests.java line 254: > 252: { > 253: int[] actual = IntStream.range(1, > 100).parallel().limit(9).toArray(); > 254: assertTrue(Arrays.equals(expected, actual)); Use `assertArrayEquals` instead of manual `Arrays.equals`? ------------- PR Review: https://git.openjdk.org/jdk/pull/30954#pullrequestreview-4204408128 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167303316 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167581992 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167394467 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167416752 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167436699 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167453538 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167461157 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167488101 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167507991 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167517886 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167516136 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167518950 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167530991 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167565706 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167566883 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167598610 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167608849 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3167614137 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168133411 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168139228 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168167810 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168170836 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168173404 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168154368 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168242485 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168262754 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168254349 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168255559 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168329396 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168530653 PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3168539037
