On Thu, 14 Dec 2023 11:39:22 GMT, Lance Andersen <lan...@openjdk.org> wrote:
>> The scope of this PR has now expanded to removing uses of the `input.zip` >> and `input.jar` files, updating any test using them to produce their own >> test vectors, and convert affected tests to JUnit. >> >> I'm marking the PR ready for review again. Before looking too closely at the >> code, it would be useful to discuss the following tests: >> >> - `Available.java`: This test has no jtreg header. I've added one and >> converted the test. Is this worthwhile, or should we rather remove it? >> - `CopyJar.java`: The concern tested seems to have superior coverage in the >> test `zip/CopyZipFile.java`. Should we retire `CopyJar.java` instead of >> coverting it? >> - `DirEntry.java`: There is duplication between this test and >> `ReadZip.readDirectoryEntry()`. Should we retire one of these? > >> The scope of this PR has now expanded to removing uses of the `input.zip` >> and `input.jar` files, updating any test using them to produce their own >> test vectors, and convert affected tests to JUnit. >> >> I'm marking the PR ready for review again. Before looking too closely at the >> code, it would be useful to discuss the following tests: >> >> * `Available.java`: This test has no jtreg header. I've added one and >> converted the test. Is this worthwhile, or should we rather remove it? > > This could be moved into ReadZip. I do not believe we have a specific test > and it is trivial > >> * `CopyJar.java`: The concern tested seems to have superior coverage in the >> test `zip/CopyZipFile.java`. Should we retire `CopyJar.java` instead of >> coverting it? > > Yes CopyZipFile already exercises Zipfile.ZipInputStream so it is safe to > retire CopyJar (though CopyZipFile could use a junit conversion ;-) >> * `DirEntry.java`: There is duplication between this test and >> `ReadZip.readDirectoryEntry()`. Should we retire one of these? > > I believe you meant GetDirEntry.java not DirEntry.java? > > Having a test that specifically validates we can read META-INF is not a bad > thing, but I suspect we have a test that already does that if not in the > java/util/zip tests or java/util/jar tests. If not we should keep it but > merge it as you suggest @LanceAndersen Thanks for your guidance! I moved `Available` into `ReadZip`, deleted `CopyJar` and merged `GetDirEntry` into ReadZip.readDirectoryEntries (adding a 'META-INF/' directory just in case) ------------- PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1855753641
