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

[Lance Andersen]

On Tue, 14 Feb 2023 17:46:21 GMT, Eirik Bjorsnos <duke 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

Hi Eirik,

Thank you for  your suggested changes to this test.

I think if we are going to re-work this test, we should go further including improving the comments for future maintainers

I made a quick pass and some initial thoughts are below

Best
Lance

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

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

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

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

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

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

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

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

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

PR Review: https://git.openjdk.org/jdk/pull/12563#pullrequestreview-1316410758
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119295384
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119297073
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119298405
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119299133
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119300038
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119300500
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119316956
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1119310517