On Mon, 2 Nov 2026 09:05:32 GMT, Volkan Yazici <[email protected]> wrote:
>> Fix `RandomAccessFile#skipBytes(int)` and overhaul associated tests.
>>
>> ### Testing notes
>>
>> `SkipBytes` has gone through a major facelift.
>>
>> - Past EOF `getFilePointer()` check was broken, fixed in f15ccfaf.
>> - `new RandomAccessFile("input.txt" , "rw")` was not working at all. File
>> needed to be passed via `new File(System.getProperty("test.src", "."),
>> "input.txt")`. Fixed in f4300dbb.
>> - `input.txt` doesn't need to be VCS-tracked. Removed it in f4300dbb.
>> - Extended the tests to reproduce the issue, see 049ae7c5. New cases are
>> cloned from 46d34868 delivered in [JDK-8394316].
>> - Migrated the tests to JUnit, see 02ac947c.
>>
>> [JDK-8394316]: https://bugs.openjdk.org/browse/JDK-8394316
>>
>> ---------
>> - [X] I confirm that I make this contribution in accordance with the
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> Volkan Yazici has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improve code style
Update the RAF.skipBytes looks good, just a few minor comments on the test to
make it easier to read.
test/jdk/java/io/RandomAccessFile/SkipBytes.java line 52:
> 50: // Populate the file
> 51: var fileNamePrefix =
> "RAF-%s-%s-".formatted(SkipBytes.class.getSimpleName(), size);
> 52: Path filePath = createTempFileOfSize(fileNamePrefix, ".dat",
> size);
Can you move this to the test method? There shouldn't be any need for this to
be test parameter.
test/jdk/java/io/RandomAccessFile/SkipBytes.java line 102:
> 100: @ParameterizedTest(name = "{0}, {2}, {3}")
> 101: @MethodSource("args")
> 102: void test(long size, Path path, long expectedPosition0, int
> requestedSkip) throws IOException {
Maybe rename expectedPosition0 to something like initialPosition as that it
what the parameter is.
test/jdk/java/io/RandomAccessFile/SkipBytes.java line 131:
> 129: } else {
> 130: expectedPosition1 = size;
> 131: }
This seems a complicated way to set expectedPosition1. Is there a reason to not
to use initialPosition + skip ?
test/jdk/java/io/RandomAccessFile/SkipBytes.java line 137:
> 135: int read = raf.read();
> 136: assertEquals(
> 137: position1 >= size, read < 0,
I think something this would be easier to read.
if (position1 >= size) {
assertEquals(-1, read, "Expected EOF");
} else {
assertNotEquals(-1, read, "Expected a byte");
}
-------------
PR Review: https://git.openjdk.org/jdk/pull/33649#pullrequestreview-5753533212
PR Review Comment: https://git.openjdk.org/jdk/pull/33649#discussion_r4455742634
PR Review Comment: https://git.openjdk.org/jdk/pull/33649#discussion_r4455753409
PR Review Comment: https://git.openjdk.org/jdk/pull/33649#discussion_r4455787777
PR Review Comment: https://git.openjdk.org/jdk/pull/33649#discussion_r4455823487
