On Wed, 18 Mar 2026 00:16:49 GMT, Marcono1234 <[email protected]> wrote:
>> In the java/io tests, replace the TestNG framework with JUnit.
>
> test/jdk/java/io/DataOutputStream/WriteUTF.java line 69:
>
>> 67: @Test
>> 68: public void arrayIndexOutOfBoundsException() throws IOException {
>> 69: assertThrows(IOException.class, () -> writeUTF(Integer.MAX_VALUE
>> / 3 + 1));
>
> Is it intended that these got less specific, only expecting `IOException`
> instead of `UTFDataFormatException`?
No, that was an oversight, thanks.
> test/jdk/java/io/InputStream/NullInputStream.java line 71:
>
>> 69: final var value = new AtomicInteger();
>> 70: assertDoesNotThrow(() -> value.set(openStream.available()));
>> 71: assertEquals(0, value.get(), "available() != 0");
>
> Out of curiosity, is usage of `assertDoesNotThrow` here really needed? An
> uncaught exception will also mark a test as failed (see
> [documentation](https://docs.junit.org/6.0.3/writing-tests/exception-handling.html#uncaught))
> (unless there is something special how the build setup here runs the tests?).
> So it would simplify the code to add `throws IOException` / `throws
> Exception` and then store the result of `openStream.available()` in a normal
> local variable.
I agree, in particular as it allows to dispense with the Atomic variables.
> test/jdk/java/io/InputStream/NullInputStream.java line 118:
>
>> 116: value.set(b.length);
>> 117: });
>> 118: assertEquals(0, value.get(), "readNBytes(1, false) != 0");
>
> Was already like this before, but what is the "false" here supposed to mean?
>
> Suggestion:
>
> assertEquals(0, value.get(), "readNBytes(0) != 0");
>
> assertDoesNotThrow(() -> {
> byte[] b = openStream.readNBytes(1);
> value.set(b.length);
> });
> assertEquals(0, value.get(), "readNBytes(1) != 0");
The "false" was vestigial from some prior version.
> test/jdk/java/io/InputStream/NullInputStream.java line 161:
>
>> 159: public void testReadBIIClosed() {
>> 160: assertThrows(IOException.class,
>> 161: () ->closedStream.read(new byte[1], 0, 1));
>
> Incorrect formatting?
>
> Suggestion:
>
> () -> closedStream.read(new byte[1], 0, 1));
>
> (please don't "accept" this suggestion to not include me as co-author)
Yes.
> test/jdk/java/io/InputStreamReader/StatefulDecoderNearEOF.java line 52:
>
>> 50: public static Stream<Arguments> inputs() {
>> 51: return Stream.of
>> 52: (// BOM, followed by High surrogate (in UTF-16LE).
>
> Is the placement of the `(` here intentional?
>
> Suggestion:
>
> return Stream.of(
> // BOM, followed by High surrogate (in UTF-16LE).
It was intentional but ugly.
> test/jdk/java/io/ObjectStreamClass/ObjectStreamClassCaching.java line 32:
>
>> 30: import org.junit.jupiter.api.Test;
>> 31:
>> 32: import static org.junit.jupiter.api.Assertions.assertFalse;
>
> Is the `assertFalse` import here unused?
Yes.
> test/jdk/java/io/OutputStream/NullOutputStream.java line 72:
>
>> 70: @Test
>> 71: public void testWrite() {
>> 72: try {
>
> Convert this `try`-`catch` as well?
Sure.
> test/jdk/java/io/PrintStream/EncodingTest.java line 90:
>
>> 88: createFile(getPrintStream(type, file2.getPath(), null, charset));
>> 89:
>> 90: assertLinesMatch(
>
> `assertLinesMatch` provides additional functionality (see
> [doc](https://docs.junit.org/6.0.3/api/org.junit.jupiter.api/org/junit/jupiter/api/Assertions.html#assertLinesMatch(java.util.List,java.util.List))),
> which is probably undesired here?
I doubt it matters here but reverting to `assertEquals` is probably better.
> test/jdk/java/io/PrintWriter/EncodingTest.java line 91:
>
>> 89: createFile(getWriter(type, file2.getPath(), null, charset));
>> 90:
>> 91: assertLinesMatch(Files.readAllLines(Paths.get(file1.getPath()),
>> charset),
>
> `assertLinesMatch` provides additional functionality (see
> [doc](https://docs.junit.org/6.0.3/api/org.junit.jupiter.api/org/junit/jupiter/api/Assertions.html#assertLinesMatch(java.util.List,java.util.List))),
> which is probably undesired here?
Same.
> test/jdk/java/io/Reader/ReadCharBuffer.java line 111:
>
>> 109: } catch (IOException e) {
>> 110: throw new UncheckedIOException(e);
>> 111: }
>
> Could this be simplified by just adding `throws IOException` / `throws
> Exception` to the test method, and then omitting the `try`-`catch` here?
Yes.
> test/jdk/java/io/Reader/ReadIntoZeroLengthArray.java line 75:
>
>> 73:
>> 74: @MethodSource("readers")
>> 75: void test0(Reader r) throws IOException {
>
> `@ParameterizedTest` is missing?
Yes.
> test/jdk/java/io/Reader/ReadIntoZeroLengthArray.java line 80:
>
>> 78:
>> 79: @MethodSource("readers")
>> 80: void test1(Reader r) throws IOException {
>
> `@ParameterizedTest` is missing?
Again.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956805348
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956805875
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956808449
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956807033
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956809038
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956820318
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956826895
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956809627
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956810121
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956810778
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956811069
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2956811373
