On Fri, 16 May 2025 11:02:54 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> src/java.base/share/classes/sun/launcher/resources/launcher.properties line
>> 283:
>>
>>> 281: make inner class static or move inner class out to separate source
>>> file
>>> 282: java.launcher.cls.error8=\
>>> 283: Error: abstract class {0} can not instantiated\n\
>>
>> Nit - should this use a comma instead of the newline?
>
> The other errors here provide some advice on the second line, so I kept that
> pattern for this error as well. So, the newline is intentional.
Sounds fine to me then.
>> test/jdk/tools/launcher/Arrrghs.java line 635:
>>
>>> 633: tr.contains("Error: abstract class Foo can not instantiated");
>>> 634: if (!tr.testStatus)
>>> 635: System.out.println(tr);
>>
>> Hello Jan, I think this is pre-existing, but it looks like we don't fail the
>> test even when `tr.testStatus` indicates a failure? We seem to only write
>> out to `System.out` and return from the test method. Am I misreading the
>> test code?
>
> `tr.contains` marks the test as failing, and eventually `TestHelper.main`
> will fail the test. It is probably not how I would write the test from
> scratch, but re-writing an already existing test also does not seem like a
> good option.
Thank you for that. I didn't even realize that this was a "main()" test. I saw
that there was a `@Test` annotation on these test methods and assumed it was
either a testng or junit test. Turns out it's just another internal annotation
of the `TestHelper`.
> It is probably not how I would write the test from scratch, but re-writing an
> already existing test also does not seem like a good option.
What you have in the current PR is fine with me.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25256#discussion_r2092893716
PR Review Comment: https://git.openjdk.org/jdk/pull/25256#discussion_r2092892219
