On Thu, 14 Dec 2023 14:55:08 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> This PR suggests we retire the binary test vectors `ZipFile/input.zip`, >> `ZipFile/input.jar` and `ZipFile/crash.jar` >> >> Binary test vectors are harder to analyze, and sharing test vectors across >> unrelated tests increases maintenance burden. It would be better to have >> each test produce its own test vectors independently. >> >> While visiting these dusty tests, we should take the opportunity to convert >> them to JUnit, add more comments and perform some mild modernization and >> cleanups where appropriate. We should also consider whether any test are >> duplicated and can be retired or moved into other test files as separate >> methods. See comments below. >> >> To help reviewers, here are some comments on the updated tests: >> >> `Available.java` >> This test currently has no jtreg `@test` header, so isn't currently active. >> After discussion, we decided to merge this test into `ReadZip.java`. I added >> some checks to verify that reading from the stream reduces the number of >> available bytes accordingly, also checking the behavior when the stream is >> closed. >> >> `CopyJar.java` >> The concern of copying entries seems to now have better coverage in the test >> `zip/CopyZipFile`. We decided to retire this test rather than convert it and >> instead convert `CopyZipFile` to JUnit. >> >> `EnumAfterClose.java` >> To prevent confusion with Java Enums, I suggest we rename this test to >> `EnumerateAfterClose`. >> >> `FinalizeInflater.java` >> The code verified by this test has been updated to use cleaners instead of >> finalizers. Still, this test code relies on finalizers. Not sure if this is >> an issue, but this test will need to be updated when finalizers are finally >> removed. >> >> `GetDirEntry.java` >> We decided to merge this test into `ReadZip.readDirectoryEntries` rather >> than keeping it as a separate test. >> >> `ReadZip.java` >> Nothing exciting here, the single main method was split into multiple JUnit >> methods, each focusing on a separate concern. A new test case `noentries()` >> was added, resolving >> [JDK-8322830](https://bugs.openjdk.org/browse/JDK-8322830) >> >> `ReleaseInflater.java` >> Nothing exciting, tried to add some comment to help understanding of what is >> tested. >> >> `StreamZipEntriesTest.java` >> This test was using TestNG so was converted to JUnit for consistency. Added >> some comments to help understanding. >> >> `ReadAfterClose.java` >> This uses the binary test vector `crash.jar`. The test is converted to JUnit >> and moved to `ReadZip.readAfterClose´. The test is expanded to exercise more >> ... > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Merge the ReadAfterClose test into ReadZip, converting it to JUnit. test/jdk/java/util/zip/ZipFile/ReadZip.java line 233: > 231: URI uri = URI.create("jar:" + zip.toUri()); > 232: Map<String, Object> env = Map.of("create", "true", > "forceZIP64End", "true"); > 233: try (FileSystem fs = FileSystems.newFileSystem(uri, env)) { No Need to use a URI here test/jdk/java/util/zip/ZipFile/ReadZip.java line 243: > 241: } > 242: // Read using ZipFileSystem > 243: try (FileSystem fs = FileSystems.newFileSystem(uri, Map.of())) { Same comment as above regarding URI test/jdk/java/util/zip/ZipFile/ReadZip.java line 249: > 247: > 248: /** > 249: * Read a zip file created via "echo hello | zip dst.zip -", This was created using info-zip so I would clarify in the comments test/jdk/java/util/zip/ZipFile/ReadZip.java line 257: > 255: @Test > 256: public void readZip64EndZipProcess() throws IOException, > InterruptedException { > 257: if (Files.notExists(Paths.get("/usr/bin/zip"))) { We should address this as the test won't run on Windows. It would be better to store the zip as a byte array so that it can be processed on all platforms and by removing ProcessBuilder, the test run will speed up a bit ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17038#discussion_r1430574058 PR Review Comment: https://git.openjdk.org/jdk/pull/17038#discussion_r1430574583 PR Review Comment: https://git.openjdk.org/jdk/pull/17038#discussion_r1430575196 PR Review Comment: https://git.openjdk.org/jdk/pull/17038#discussion_r1430578476
