On Tue, 15 Aug 2023 19:13:31 GMT, Christoph <d...@openjdk.org> wrote:
>> Add new test case with sample modules that contains some >> requires/exports/uses/provides. >> >> We are just unsure if and how we should add some last step of verificaiton >> with the extracted and decompiled class. >> >> Follow up task from https://github.com/openjdk/jdk/pull/14408 > > Christoph has updated the pull request incrementally with two additional > commits since the last revision: > > - Add another required transitive desktop > > Assert number of module description generated sub modules > > Co-authored-by: Oliver Kopp <kopp....@gmail.com> > - Add copyright header and apply some formatting > Revert unnecesary CompilerUtil Looks good. Since the image is created with batchSize = 1, it means that one `subX` method is created for each module linked in the image. So the test can count the number of modules in the image and the number of `subX` methods should match the module count. I include the suggested code to check that. test/jdk/tools/jlink/JLinkDedupTestBatchSizeOne.java line 38: > 36: /* > 37: * @test > 38: * @summary Make sure that modules can be linked using jlink and > deduplication works correctly when creating sub methods please wrap this long line. test/jdk/tools/jlink/JLinkDedupTestBatchSizeOne.java line 94: > 92: .addMods("m1") > 93: .addMods("m2") > 94: .addMods("m2") `m2` is added twice. The duplicated line should be removed. test/jdk/tools/jlink/JLinkDedupTestBatchSizeOne.java line 116: > 114: > 115: extractJImage(image); > 116: decompileWitJavap(image); No needed. These 2 lines along with the methods can be deleted. test/jdk/tools/jlink/dedup/src/m1/p1/AInterface.java line 1: > 1: package p1; missing copyright header. test/jdk/tools/jlink/dedup/src/m4/p4/Main.java line 43: > 41: } > 42: var moduleClass = > Class.forName("jdk.internal.module.SystemModules$all"); > 43: long subMethodCount = > Arrays.stream(moduleClass.getDeclaredMethods()).filter(method -> > method.getName().startsWith("sub")).count(); nit: wrap long line Suggestion: long subMethodCount = Arrays.stream(moduleClass.getDeclaredMethods()) .filter(method -> method.getName().startsWith("sub")) .count(); test/jdk/tools/jlink/dedup/src/m4/p4/Main.java line 47: > 45: if (subMethodCount != MODULE_SUB_METHOD_COUNT) { > 46: throw new AssertionError("Difference in generated sub module > methods count! Expected: " + MODULE_SUB_METHOD_COUNT + " but was " + > subMethodCount); > 47: } Suggestion: // one subX method per each module is generated as the image is linked with // --system-modules=batchSize=1 var moduleCount = ModuleFinder.ofSystem().findAll().stream().count(); if (subMethodCount != moduleCount) { throw new AssertionError("Difference in generated sub module methods count! Expected: " + moduleCount + " but was " + subMethodCount); } ------------- PR Review: https://git.openjdk.org/jdk/pull/15234#pullrequestreview-1579273111 PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1295023900 PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1295023665 PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1295022781 PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1295024085 PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1295029909 PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1295025060
