RFR: 8304013: Add a fast, non-manual alternative to test/jdk/java/util/zip/ZipFile/TestTooManyEntries

[Eirik Bjorsnos]

On Tue, 31 Jan 2023 11:38:07 GMT, Lance Andersen <lancea at openjdk.org> wrote:

>> The TestTooManyEntries test was originally added to validate that ZIP64 files with CEN sizes exceeding what ZipFile  supports are rejected with a ZipException. The test does this by creating a large ZIP file (several gigabytes) with many enties. Because this is resource intensive, the test is currently tagged as manual.  (See #6927)
>> 
>> It would be useful to have a test which asserts the CEN size enforcement, but without the CPU, disk, memory and run time requirements of TestTooManyEntries.  Such a fast test can run non-manual,  without the @requires and manual tags as found in TestTooManyEntries.
>> 
>> This PR adds the EndOfCenValidation test which creates sparse test ZIPs where the CEN is "inflated" such that is matches the size declared in the "End of central directory" records.
>> 
>> While thee sparse files look large, they consume very little disk space on file systems supporting sparse files:
>> 
>> 
>> 16 -rw-r--r--  1   2147483702 Feb  6 18:54 bad-cen-offset.zip
>> 16 -rw-r--r--  1   2147483703 Feb  6 18:54 cen-size-too-large.zip
>>  8 -rw-r--r--  1          132 Feb  6 18:54 invalid-zen-size.zip
>> ``` 
>> 
>> For good measure, two new test methods are added to excercise the remaining ZipExceptions which ZipFile may throw during validation of the END record .
>
> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 47:
> 
>> 45: import static org.testng.Assert.fail;
>> 46: 
>> 47: public class CenSizeTooLarge {
> 
> I would add a comment which also points to the original test

Added a comment referring to TestTooManyEntries explaining why this test exists.

> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 52:
> 
>> 50:     private static final int CEN_SIZE_OFFSET = 12; // Offset of CEN size field within ENDHDR
>> 51:     private Path huge; // ZIP file with CEN size exceeding limit
>> 52:     private Path big;  // ZIP file with CEN size exactly on the limit
> 
> You might consider more meaningful names for the Paths

I've removed these fields now and instead produce the files in the test methods directly, removing the need to name the fields.

> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 92:
> 
>> 90:     private void assertRejected(Path zip, String expectedMsg) throws IOException {
>> 91:         try (ZipFile zf = new ZipFile(zip.toFile())) {
>> 92:             fail("Expected ZipFile to throw ZipException");
> 
> You could consider using expectThrows and then validation the value of getMessage()

Replaced with expectedExceptions and expectedExceptionsMessageRegExp

> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 107:
> 
>> 105:      * to test that the large CEN size is rejected
>> 106:      */
>> 107:     private Path zipWithCenSize(String name, int cenSize) throws IOException {
> 
> You could speed this up a bit more by just creating the array representing the original Zip 1 time and modify it each pass.(though it might not speed things up that much)

I introduced the templateZip() method which returns a byte[]. While the speed is insignificat here, I think it makes it more explicit that we're producing ZIPs from a template.

> test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 113:
> 
>> 111:             try (ZipFile zf = new ZipFile(zip.toFile())) {
>> 112:             }
>> 113:         });
> 
> You can omit the try block

I removed the try/catch block with a quivering lower lip :-)

> test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 177:
> 
>> 175:                                           Path zip) throws IOException {
>> 176: 
>> 177:         Files.deleteIfExists(zip);
> 
> Is this needed?  I think I would just call the cleanup method from the setup method

Sparse files need to be created as such. The purpose of this delete exists to make this explicit. I moved this closer to the FileChannel.open and added a comment to explain the delete.

> test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 219:
> 
>> 217:     }
>> 218: 
>> 219:     // Produce a byte array of a ZIP with a single entry
> 
> Minor nit, I would use the same comment format for the methods as above and probably add an @throws as needed so javadoc tag checks won't fail

Changed to block comment style and added @throws.

> test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 223:
> 
>> 221:         ByteArrayOutputStream bout = new ByteArrayOutputStream();
>> 222:         try (ZipOutputStream zout = new ZipOutputStream(bout)) {
>> 223:             zout.putNextEntry(new ZipEntry("duke.txt"));
> 
> Perhaps clarify why you are not writing out any data...

No particular reason, so I opted to output some data instead of explaining.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1094495050
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1094491416
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1094495538
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1094493992
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1147233013
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1147234584
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1147233518
PR Review Comment: https://git.openjdk.org/jdk/pull/12231#discussion_r1147234962