On Fri, 15 Dec 2023 20:38:19 GMT, Eirik Bjorsnos <d...@openjdk.org> wrote:
> The test is shaping up nicely. Since it's a new test it should use JUnit 5. > > Also included a couple suggestions, I'll stop now, promise! :) No prob - they're all reasonable suggestions :) > test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 30: > >> 28: */ >> 29: >> 30: import org.junit.Test; > > Let's use JUnit 5: > Suggestion: > > import org.junit.jupiter.api.Test; Should be fixed in cf457eff38c. > test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 36: > >> 34: import java.util.zip.*; >> 35: >> 36: import static org.junit.Assert.assertArrayEquals; > > Let's use JUnit 5: > > Suggestion: > > import static org.junit.jupiter.api.Assertions.assertArrayEquals; Should be fixed in cf457eff38c. > test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 53: > >> 51: byte[] compressedN = repeat(compressed1, NUM_COPIES); >> 52: >> 53: // (a) Read back copied compressed data from a stream where >> available() is accurate and verify > > Suggestion: > > // (a) Read back inflated data from a stream where available() is > accurate and verify Should be fixed in cf457eff38c. > test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 54: > >> 52: >> 53: // (a) Read back copied compressed data from a stream where >> available() is accurate and verify >> 54: byte[] readback1 = new GZIPInputStream(new >> ByteArrayInputStream(compressedN)).readAllBytes(); > > These readback lines got rather long. Perhaps consider extracting the GZIP > reading into a method taking the source InputStream as a parameter? > > Suggestion: > > byte[] readback1 = inflateFrom(new ByteArrayInputStream(compressedN)); Should be fixed in cf457eff38c. > test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 57: > >> 55: assertArrayEquals(uncompressedN, readback1); >> 56: >> 57: // (b) Read back copied compressed data from a stream where >> available() always returns zero and verify > > Suggestion: > > // (b) Read back inflated data from a stream where available() always > returns zero and verify Should be fixed in cf457eff38c. > test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 58: > >> 56: >> 57: // (b) Read back copied compressed data from a stream where >> available() always returns zero and verify >> 58: byte[] readback2 = new GZIPInputStream(new >> ZeroAvailableStream(new ByteArrayInputStream(compressedN))).readAllBytes(); > > Suggestion: > > byte[] readback2 = inflateFrom(new ZeroAvailableStream(new > ByteArrayInputStream(compressedN))); Should be fixed in cf457eff38c. ------------- PR Comment: https://git.openjdk.org/jdk/pull/17113#issuecomment-1858489270 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428486646 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428487198 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428486708 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428487465 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428486783 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428486924
