RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip
Eirik Bjorsnos
duke at openjdk.org
Thu Dec 14 12:25:10 UTC 2023
On Thu, 14 Dec 2023 11:39:22 GMT, Lance Andersen <lancea at 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
More information about the core-libs-dev
mailing list