RFR: 8357016: Candidate main methods not computed properly [v2]

[Jaikiran Pai]

On Fri, 16 May 2025 11:02:54 GMT, Jan Lahoda <jlahoda at 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