On Sat, 9 May 2026 21:06:57 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:
>
> Use JUnit-specific assertions if possible
> Thank you for the review @Marcono1234, the comments are very useful. I added
> your suggestions.
Thanks, that is great to hear! I have looked at the remaining changes now as
well; hopefully these comments are useful too.
Note that apparently the GitHub UI does not permit me to "resolve" my own
review comments (possibly because I am not a member of this repository),
therefore I cannot close the previous ones which are already done.
test/jdk/java/util/regex/PatternStreamTest.java line 243:
> 241: } catch (ConcurrentModificationException e) {
> 242: // Should reach here
> 243: }
Use `assertThrows` for these two as well?
test/jdk/java/util/regex/PatternStreamTest.java line 267:
> 265: } catch (ConcurrentModificationException e) {
> 266: // Should reach here
> 267: }
Use `assertThrows` for these as well?
test/jdk/java/util/stream/boottest/java.base/java/util/stream/StreamReuseTest.java
line 26:
> 24: /*
> 25: * @test
> 26: */
Is this new comment needed? It seems some of the other tests don't have it
either.
test/jdk/java/util/stream/test/TEST.properties line 1:
> 1: # This file identifies root(s) of the test-ng hierarchy.
Suggestion:
# This file identifies root(s) of the JUnit hierarchy.
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/FillableStringTest.java
line 53:
> 51: String s = generate().collect(Collectors.joining());
> 52: assertEquals("THREEFOURFIVE", s);
> 53: }
As side note: It seems this `testStringBuffer` is identical to
`testStringBuilder`?
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/RangeTest.java
line 137:
> 135:
> 136: @Test
> 137: public void tesIntRangeReduce() {
Might be good to take the opportunity to fix this existing typo:
<code>tes<ins>t</ins></code>
Suggestion:
public void testIntRangeReduce() {
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/SortedOpTest.java
line 116:
> 114: assertNotNull(caught, "Expected an instance of exception
> IllegalArgumentException but no exception thrown");
> 115: assertInstanceOf(IllegalArgumentException.class, caught,
> 116: String.format("Expected an instance of exception
> IllegalArgumentException but got %s", caught));
Use `assertThrows(IllegalArgumentException.class, ...)`?
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/StreamCloseTest.java
line 139:
> 137: caught = true;
> 138: }
> 139: assertTrue(caught);
Here and for the similar code below, would it make sense to use `assertThrows`?
That would also make it clearer that the implicit `close()` call is what is
expected to throw.
Might be out-of-scope, or might not improve readability that much though.
For example something like:
Stream<Integer> ints = countTo(100).stream();
ints.onClose(close1).onClose(close2).onClose(close3);
ints.forEach(i -> {});
var e = assertThrows(RuntimeException.class, () -> ints.close());
assertCascaded(e, 3);
assertTrue(holder[0] && holder[1] && holder[2]);
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToArrayOpTest.java
line 26:
> 24: /*
> 25: * @test
> 26: */
Out of curiosity, is this new comment actually needed?
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToArrayOpTest.java
line 70:
> 68:
> 69: Object[] objects = exerciseTerminalOps(data, s -> s.map(i ->
> (Integer) (i + i)), s -> s.toArray());
> 70: assertTrue(objects.length == data.size());
Here and below, replace the remaining occurrences of `assertTrue(... == ...)`
with `assertEquals`?
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToArrayOpTest.java
line 143:
> 141: }
> 142: assertTrue(caught != null);
> 143: assertEquals(ArrayStoreException.class,
> caught.getClass());
Use `assertThrows(ArrayStoreException.class, ...)`?
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToArrayOpTest.java
line 212:
> 210: }
> 211: assertNotNull(caught);
> 212: assertEquals(ArrayStoreException.class,
> caught.getClass());
Use `assertThrows(ArrayStoreException.class, ...)`?
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToArrayOpTest.java
line 248:
> 246:
> @MethodSource("java.util.stream.IntStreamTestDataProvider#intStreamTestData")
> 247: public void testIntOpsWithFlatMap(String name, TestData.OfInt data) {
> 248: // Int the size of the source
Possibly out-of-scope, but maybe this comment here should actually say "Double
the size"? Given that this is what it does, and the sibling tests are "Retain
the size" and "Reduce the size".
(Maybe this comment was copied from the test handling `DoubleStream`, and then
"_Double_ the size" was erroneously replaced with "Int" and "Long".)
Suggestion:
// Double the size of the source
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToArrayOpTest.java
line 328:
> 326:
> @MethodSource("java.util.stream.LongStreamTestDataProvider#longStreamTestData")
> 327: public void testLongOpsWithFlatMap(String name, TestData.OfLong
> data) {
> 328: // Long the size of the source
Same as above, maybe this should be "Double the size"?
Suggestion:
// Double the size of the source
test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToListOpTest.java
line 59:
> 57: fail("List.set did not throw
> UnsupportedOperationException");
> 58: } catch (UnsupportedOperationException ignore) { }
> 59: }
Use `assertThrows(UnsupportedOperationException.class, ...)`?
test/jdk/lib/testlibrary/bootlib/java.base/java/util/SpliteratorTestHelper.java
line 641:
> 639: }
> 640: if (s.hasCharacteristics(Spliterator.SIZED)) {
> 641: assertTrue(s.estimateSize() != Long.MAX_VALUE);
Use `assertNotEquals`?
test/jdk/lib/testlibrary/bootlib/java.base/java/util/stream/OpTestCase.java
line 53:
> 51: */
> 52: @ExtendWith(LoggingTestCase.Extension.class)
> 53: public abstract class OpTestCase extends LoggingTestCase {
Do you know if instead of annotating all subclasses with `@ExtendWith`, it
would be possible to annotate `LoggingTestCase` instead (and have the
annotation inherited)?
The `@ExtendWith` documentation [mentions
inheritance](https://docs.junit.org/6.0.3/api/org.junit.jupiter.api/org/junit/jupiter/api/extension/ExtendWith.html#inheritance-heading)
but only in the context of fields, so maybe this is not possible?
If not, maybe it would be useful to extend the `LoggingTestCase` Javadoc to
mention that subclasses need to add
`@ExtendWith(LoggingTestCase.Extension.class)`.
test/jdk/lib/testlibrary/bootlib/java.base/java/util/stream/StreamTestDataProvider.java
line 202:
> 200: }
> 201:
> 202: // Return an array of ( String name, StreamTestData<Integer>
Closing `)` was accidentally removed
Suggestion:
// Return an array of ( String name, StreamTestData<Integer> )
test/jdk/lib/testlibrary/bootlib/java.base/java/util/stream/ThrowableHelper.java
line 39:
> 37:
> 38: assertNotNull(caught);
> 39: assertTrue(ce.isInstance(caught));
Use `assertThrows(ce, r)`? (respectively just inline this into the call sites
and remove this `checkException` method?)
-------------
PR Review: https://git.openjdk.org/jdk/pull/30954#pullrequestreview-4271786144
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3225977750
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3225982199
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3226028903
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3234615270
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3234570381
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3230276756
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3230343735
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3233041972
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3233151913
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3233225602
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3233232842
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3233235256
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3233178654
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3233208241
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3233254786
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3234837857
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3234748707
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3234782835
PR Review Comment: https://git.openjdk.org/jdk/pull/30954#discussion_r3234798851
