RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v2]

[Archie Cobbs]

On Fri, 15 Dec 2023 14:09:12 GMT, Eirik Bjorsnos <duke at openjdk.org> wrote:

>> Archie Cobbs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address review comments.
>
> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 52:
> 
>> 50:         buf.reset();
>> 51:         for(int i = 0; i < 100; i++)
>> 52:             buf.write(gz);
> 
> Whitespace after `for`, braces are recommended even when optional in the language:
> 
> Suggestion:
> 
>         for (int i = 0; i < 100; i++) {
>             buf.write(gz);
>         }

Thanks - should be addressed in 074b8455b11.

> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 53:
> 
>> 51:         for(int i = 0; i < 100; i++)
>> 52:             buf.write(gz);
>> 53:         final byte[] gz32 = buf.toByteArray();
> 
> Drop final, consider finding a more expressive name:
> 
> Suggestion:
> 
>         byte[] concatenatedGz = buf.toByteArray();

Thanks - should be addressed in 074b8455b11.

> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 56:
> 
>> 54: 
>> 55:         // (a) Read it back from a stream where available() is accurate
>> 56:         long count1 = countBytes(new GZIPInputStream(new ByteArrayInputStream(gz32)));
> 
> This is the expected number of bytes to be read. This could be calculated directly from the uncompressed data. At least the name should express that this is the expected number of bytes:
> 
> Suggestion:
> 
>         long expectedBytes = countBytes(new GZIPInputStream(new ByteArrayInputStream(gz32)));
> 
> 
> Similarly, `count2` could be renamed `actualBytes`

Thanks - should be addressed in 074b8455b11.

> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 62:
> 
>> 60: 
>> 61:         // They should be the same
>> 62:         Assert.assertEquals(count2, count1);
> 
> This could be a static import. And for the assertion failure message to be correct, the expected value must come first:
> 
> Suggestion:
> 
>         assertEquals(expectedBytes, actualBytes);
> 
> 
> An alternative here could be to store the original uncompressed data and compare that to the full inflated  data read from GZIPInputStream using assertArrayEquals. The length alone is a bit of a weak proxy for equality.

Thanks - should be addressed in 074b8455b11.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428174716
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428175505
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428174894
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428175182