On Mon, 25 Nov 2024 13:55:30 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Alan's review - rename variable and drop final from test
>> - Alan's review - no need for throws in method signature
>
> test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java line 71:
>
>> 69: // if we can't create a directory with an emoji in its path
>> name,
>> 70: // then skip the entire test
>> 71: assumeTrue(false, "Skipping test since emoji directory
>> couldn't be created: "
>
> Perhaps `Assumptions.abort(message)` would be more clear here.
Hello Eirik, it looks like this is a new API introduced in 5.9 of jupiter API.
Looking at jtreg 7.4 that's used in JDK mainline, the jupiter API we use is
5.10.x. So yes, `abort()` would be more appropriate here. I've updated
accordingly.
> test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java line 78:
>
>> 76:
>> 77: ASCII_DIR = Files.createTempDirectory(SCRATCH_DIR,
>> "test-urlclasspath");
>> 78: Files.writeString(ASCII_DIR.resolve(RESOURCE_NAME), "hello");
>
> The contents of the resources seems not to matter for the test currently.
>
> Consider using just `Files::createFile` here, or make unique content for each
> resource such that you can verify the correct content was found in the tests.
Done - I've updated the test to use `Files.createFile` instead of
`writeString()`.
> test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java line 174:
>
>> 172: }
>> 173:
>> 174: private static String[] getClassPathElements() {
>
> This could perhaps be `addClassPathElemements(URLClassPath)` instead for
> cleaner call sites?
>
> Or perhaps even `URLClassPath makeClassPath()` since both tests seem to have
> identical class paths?
I prefer keeping the classpath construction through `URLClassPath.addFile()`
closer to the test code itself. So I've left this part as-is.
> test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java line 180:
>
>> 178: return new String[]{
>> 179: // non-existent dir
>> 180: ASCII_DIR.resolve("non-existent").toString(),
>
> Just an observation: This will not be recognized as a directory, so this will
> not create a `FileLoader`, but a `JarLoader` (which will throw a
> `FileNotFoundException`).
>
> For good measure, you could consider adding an actual non-existing directory
> URL, but I think you'd need to use `URLClassPath::addURL` for that instead of
> `addFile`.
>
> PS: It's rather weird that a non-exisiting `FileLoader` is added to the class
> path, while a non-existing JarLoader is ignored. Thorny code indeed.
The use of the "non-existent" path was intentional to trigger the exceptional
code path. But I agree that the code comment which said `non-existent dir` was
inaccurate. So I've updated that comment to say "non-existent path"
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22351#discussion_r1857894000
PR Review Comment: https://git.openjdk.org/jdk/pull/22351#discussion_r1857898071
PR Review Comment: https://git.openjdk.org/jdk/pull/22351#discussion_r1857897288
PR Review Comment: https://git.openjdk.org/jdk/pull/22351#discussion_r1857895549
