On Tue, 7 Apr 2026 15:03:51 GMT, Eirik Bjørsnøs <[email protected]> wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> rename test method
>
> test/jdk/java/util/jar/JarEntry/GetMethodsReturnClones.java line 74:
>
>> 72: */
>> 73: @BeforeAll()
>> 74: static void beforeAll() throws Exception {
>
> I'm mostly used to this such methods being named `setup`. The current name
> does not provide anything other than what the annotation does. Maybe a matter
> of personal preference.
Hello Eirik, I prefer `beforeAll` for the method name because then I don't have
to think whether it gets run every time for each test method or whether it runs
once. Of course, there's the annotation for it one line above which would serve
the same purpose, so it isn't a big deal in this test. I've renamed it in the
updated PR.
> test/jdk/java/util/jar/JarEntry/GetMethodsReturnClones.java line 116:
>
>> 114: assertNotNull(certs, "JarEntry.getCertificates() returned
>> null for " + je.getName());
>> 115: // verify that the newly returned array doesn't have the
>> overwrittent value
>> 116: assertNotNull(certs[0], "Internal certiticates array was
>> modified");
>
> Typo:
>
> Suggestion:
>
> assertNotNull(certs[0], "Internal certificates array was
> modified");
Fixed
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30602#discussion_r3050667388
PR Review Comment: https://git.openjdk.org/jdk/pull/30602#discussion_r3050667895
