On Tue, 17 Mar 2026 22:04:00 GMT, Brian Burkhalter <[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`?
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.
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");
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)
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).
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?
test/jdk/java/io/OutputStream/NullOutputStream.java line 72:
> 70: @Test
> 71: public void testWrite() {
> 72: try {
Convert this `try`-`catch` as well?
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?
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?
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?
test/jdk/java/io/Reader/ReadIntoZeroLengthArray.java line 75:
> 73:
> 74: @MethodSource("readers")
> 75: void test0(Reader r) throws IOException {
`@ParameterizedTest` is missing?
test/jdk/java/io/Reader/ReadIntoZeroLengthArray.java line 80:
> 78:
> 79: @MethodSource("readers")
> 80: void test1(Reader r) throws IOException {
`@ParameterizedTest` is missing?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950181385
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950200039
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950213765
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950211165
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950217522
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950223289
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950227361
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950232292
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950232691
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950283539
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950293680
PR Review Comment: https://git.openjdk.org/jdk/pull/30289#discussion_r2950294012
