On Sat, 28 Sep 2024 14:32:31 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Henry Jen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address review feedbacks
>
> test/jdk/tools/jar/ExtractFilesTest.java line 241:
>
>> 239: if (rc != 0) {
>> 240: String s = baes.toString();
>> 241: if (s.startsWith("java.util.zip.ZipException: duplicate
>> entry: ")) {
>
> This method runs any arbitrary `jar` "command". Is there any significance why
> we are checking for a "duplicate entry" in the exception message? Maybe we
> could just remove this entire if block and just throw a `new IOException(s)`?
> As far as I can see in this test, we don't do any exception type checks on
> the thrown exception from this method.
I copied the method from another test that do the create jar, so it does that.
Some other comments also related to copied code, I can do a round of clean up.
> test/jdk/tools/jar/MultipleManifestTest.java line 67:
>
>> 65:
>> 66: @TestInstance(Lifecycle.PER_CLASS)
>> 67: @TestMethodOrder(MethodName.class)
>
> Is the use of these annotations intentional? Especially the method ordering?
I had some test failure earlier so I added those to ensure not the race
condition or left over even though I don't think that's the case.
I later figured out the real cause, so I think I can remove those.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21141#discussion_r1781337548
PR Review Comment: https://git.openjdk.org/jdk/pull/21141#discussion_r1781333864
