RFR: 8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to testNG

[Eirik Bjorsnos]

On Mon, 27 Feb 2023 20:47:42 GMT, Lance Andersen <lancea at openjdk.org> wrote:

>> CorruptedZipFiles could benefit from some spring cleaning and a conversion to testNG:
>> 
>> - The actual tests are moved into their own `@Test` methods, given more meaningful names and a Javadoc comment explaining the constraint being verified
>> - The setup code is moved to a `@Before` method, slightly modernized and rewritten to take advantage of `assertEquals` 
>> - `checkZipExceptionImpl` is updated to take advantage of `expectThrows`
>> - A bunch of constants copied over from `ZipFile` can be deleted since JDK-6225935 has long been fixed
>
> test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 53:
> 
>> 51:     // Copy of the template for modification in tests
>> 52:     private byte[] bad;
>> 53:     // Some well-known locations in the golden ZIP
> 
> Would be a good opportunity to provide better naming

I went for "template" and "copy" here.

> test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 58:
> 
>> 56:     @BeforeTest
>> 57:     public void setup() throws IOException {
>> 58:         // Make a ZIP with a single entry
> 
> Please change either this method name or the other `setup()` method.
> 
> I would also add a Files.delete(zip) here

Renamed the `@BeforeMethod` one to `makeCopy`

> test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 70:
> 
>> 68:         good = Files.readAllBytes(zip);
>> 69:         Files.delete(zip);
>> 70: 
> 
> I would add a `cleanup` method in addition to deleting earlier on

This one I did not fully understand. In any case, there is really no need to write the template ZIP to disk, so I opted for a ByteArrayOutputStream instead. I added a cleanup method to delete the ZIP file produced by each test.

> test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 71:
> 
>> 69:         Files.delete(zip);
>> 70: 
>> 71:         // Set up some well-known offsets
> 
> please expand the comments

Done

> test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 90:
> 
>> 88:      */
>> 89:     @BeforeMethod
>> 90:     public void setUp() {
> 
> As mentioned above, please rename this method or the other

Yes, renamed to `makeCopy`

> test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 95:
> 
>> 93: 
>> 94:     @Test
>> 95:     public void corruptedENDSIZ() {
> 
> Please add a comment to this and the other tests describing the purpose of the test

I have given each `@Test` method a more meaningful name and added comments explaining the constraint or error condition being tested.

> test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 164:
> 
>> 162:     }
>> 163:     static int uniquifier = 432;
>> 164: 
> 
> I am not sure how many people would know what a `uniquifier` is though it is an interesting phrase which I know from SQL Server.  Perhaps consider renaming this variable

I'm not sure we need unique file names for this test. Opted for a single ZIP Path instead and removed this variable.

> test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 184:
> 
>> 182:                     "Unexpected ZipException message: " + ex.getMessage());
>> 183: 
>> 184:         } catch (IOException e) {
> 
> Another option is to throw the IOException and add IOException to the test method signature as we are going to fail anyways for the IOException

Changed to propagate the `IOException`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119742719
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119743179
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119744775
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119744908
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119745171
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119746099
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119747423
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119746553