On Fri, 13 Jun 2025 20:56:49 GMT, Johannes Döbler <[email protected]> wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address review - Convert to JUnit, correct comment typo, remove 'Infra'
>> methods
>
> test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java line 57:
>
>> 55: static void setup() throws IOException {
>> 56: var bytes = ClassFile.of().build(ClassDesc.of("fo o"), _ -> {});
>> 57: try (JarOutputStream jos = new JarOutputStream(new
>> FileOutputStream(JAR_PATH.toFile()))) {
>
> Suggestion: The original test used testclasses.jar to provide two class files
> with invalid names packaged in a jar. Since you now dynamically construct the
> class files, there is no need to write them to disk as a jar. Why not have a
> Map<String,byte[]), mapping a resource path to bytes, to store all used class
> files in memory.
Thanks, I think we can make this simplification because the original version of
this test used to test directly against a "fo o.class" binary file, and changed
overtime to adapt the jar. (Thanks @jaikiran for confirming). I updated it such
that we just read the class file bytes directly, and all JAR related logic is
dropped. I did not put it in a map, since the second class is commented out,
and we have been testing only "fo o" for quite some time now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25703#discussion_r2150503122
