RFR: 8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs [v3]

[Jaikiran Pai]

On Mon, 25 Nov 2024 13:55:30 GMT, Eirik Bjørsnøs <eirbjo at 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