On Tue, 3 Nov 2026 14:05:58 GMT, Alan Bateman <[email protected]> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Improve code style
>
> 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.
I had kept it in the parameter generator to avoid unnecessarily creating
multiple files with the same content for 345 times. Moved the test file
generation to `test()` in c31cea56b3a.
> 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.
Renamed in f04b2b10ba2.
> 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 ?
Staring for too long to the same code? 🙈 Changed as requested in 29ffe6f1592.
> 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");
> }
Changed as requested in 090a8cd5f1b.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33649#discussion_r4468465455
PR Review Comment: https://git.openjdk.org/jdk/pull/33649#discussion_r4468458754
PR Review Comment: https://git.openjdk.org/jdk/pull/33649#discussion_r4468457107
PR Review Comment: https://git.openjdk.org/jdk/pull/33649#discussion_r4468454604
