On Thu, 25 Jul 2024 04:53:33 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> SendaoYan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add a word throw
>
> test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java line 216:
>
>> 214: } catch (IOException e) {
>> 215: if (e.getMessage().contains("Mount point not found")) {
>> 216: // We would like to skip the test with a cause with
>
> Hello @sendaoYan, it feels very specific and odd to be checking only for this
> exception message. This test method's goal appears to be to create a
> read-only directory into which it wants to write out the proxy classes and
> verify that it won't be able to do that. For that it first verifies that the
> underlying `FileStore` supports posix file attributes. If it's not able to
> ascertain that the underlying `FileStore` has posix support, then it skips
> the test.
>
> So I think we should just catch the `IOException` here when getting the
> FileStore and skip the test instead of checking for specific exception
> messages. While we are at it, we should throw a `org.testng.SkipException`
> (this is a testng test) from this method wherever we are currently skipping
> the test execution by writing out a System.out warning message and returning.
Thanks for your advice, the checking for specific exception messages has been
replaced to just catch the IOException when getting the FileStore, and all the
`System.out warning message and returning` has been replaced to
`org.testng.SkipException`.
Additional, the `org.testng.SkipException` seems do not work normally in
[jtreg](https://bugs.openjdk.org/browse/CODETOOLS-7903708) for now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19905#discussion_r1691168794
