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