On Tue, 19 Jan 2021 23:12:02 GMT, Johannes Kuhn 
<github.com+652983+dasbr...@openjdk.org> wrote:

>> Simple fix - one line change: 
>> https://openjdk.github.io/cr/?repo=jdk&pr=2000&range=00#sdiff-0.
>> 
>> Most of the changes come from an additional test that fails without this fix:
>> 
>>      Error: Unable to load main class somelib.test.TestMain in module somelib
>>      java.lang.IllegalAccessError: superclass access check failed: class 
>> somelib.test.TestMain (in module somelib) cannot access class 
>> java.lang.Object (in module java.base) because module java.base does not 
>> export java.lang to module somelib
>> 
>> As described in JDK-8259395.
>> You can view the output of the test without patch here: 
>> https://github.com/DasBrain/jdk/runs/1668078245
>> 
>> Thanks to @AlanBateman for pointing out the right fix.
>
> Johannes Kuhn has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix comment, and missing newline in module-info.java

Thanks for expanding the test to cover the initial module and automatic module 
on the module path cases. I think we are nearly good to me, just a few 
suggestions to make the test easier to maintain in the future.

test/jdk/tools/launcher/modules/patch/automatic/PatchTest.java line 2:

> 1: /*
> 2:  * Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights 
> reserved.

Minor nit, should be 2021 only.

test/jdk/tools/launcher/modules/patch/automatic/PatchTest.java line 34:

> 32:  * @run testng PatchTest
> 33:  * @bug 8259395
> 34:  * @summary Runs tests that make use of automatic modules

Can we change the summary to say that it tests patching an automatic module?

test/jdk/tools/launcher/modules/patch/automatic/PatchTest.java line 122:

> 120: 
> 121:     @Test
> 122:     public void modulePathExtend() throws Exception {

I think it would be useful for future maintainers if there was a comment on 
each of the 4 tests. I'd probably rename them them too, e.g. 
testExtendAutomaticModuleOnModulePath, 
testExtendAutomaticModuleAsInitialModule, so that it's a bit clear what each 
one does.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2000

Reply via email to