On Fri, 25 Apr 2025 07:19:32 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Use the `@requires` tag instead of obtaining the operating system name from >> the `os.name` property and then exiting if the test is not run on that >> operating system. > > test/jdk/java/io/File/MacPathTest.java line 28: > >> 26: * @summary Tests paths on macOS >> 27: * @requires (os.family == "mac") >> 28: */ > > The missing `@test` on this looked odd, since that would mean that this test > wasn't being run at all so far. > > I went back and looked at the original RFR which introduced this test for > https://bugs.openjdk.org/browse/JDK-7130915. The RFR is here > https://mail.openjdk.org/pipermail/core-libs-dev/2012-June/010621.html. Going > through the webrevs posted there, it appears that this was initially a shell > test and had `@test` declaration. > > Then in https://bugs.openjdk.org/browse/JDK-8181912 we refactored it to be a > java jtreg test. The RFR for that is here > https://mail.openjdk.org/pipermail/core-libs-dev/2017-June/048225.html. Going > through this refactor RFR, the final webrev that was settled upon and > integrated appears to be this > https://mail.openjdk.org/pipermail/core-libs-dev/2017-June/048319.html. So > what that refactoring did was > (https://cr.openjdk.org/~mli/8181912/webrev.01/) it moved the `@test` > declaration to a new file `test/java/io/File/MacPath.java` which is what then > launches this `test/jdk/java/io/File/MacPathTest.java`'s main method using > `ProcessBuilder`. So this `MacPathTest.java` isn't really the jtreg `@test`. > > Given this, i think we shouldn't be adding this `@test` declaration here and > `MacPath.java` already has the necessary `@requires (os.family == "mac")`. > > What we should probably do (if you prefer in a different issue/PR), is > perhaps add a comment to this file that it gets launched through > `MacPath.java` and also maybe remove the `os.name` checks in the `main()` > method of this class. Thanks, @jaikiran, for the detailed investigation. I went partway down that road but clearly not far enough. I'll update it. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24860#discussion_r2060554322
