On Mon, 27 Feb 2023 20:47:42 GMT, Lance Andersen <lan...@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
