On Fri, 13 Jan 2023 17:05:10 GMT, Matias Saavedra Silva <matsa...@openjdk.org> wrote:
>> This is an enhancement of the test case in >> [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), which tests >> against an archive created by the "boot JDK", which is usually set as the >> previous official JDK release when building the JDK repo. >> >> If it's able to acquire previous valid JDK releases: >> - Download and install previous JDK versions (19 through N) >> where N == java.lang.Runtime.version​().major() - 1 >> - Test the interaction of the current JDK versus each of the previous >> releases >> >> If it's not able to find the previous releases revert to the existing logic >> in TestAutoCreateSharedArchiveUpgrade.java (use the test.boot.jdk or >> test.previous.jdk properties). Verified with tier1-4 tests. > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Added exceptions instead of default jdk Changes requested by iklam (Reviewer). test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java line 104: > 102: newJVM = TEST_JDK + FS + "bin" + FS + "java"; > 103: > 104: // Example path: > bundles/linux-x64/jdk-19_linux-x64_bin.tar.gz/jdk-19/bin/java It's not cleat what this comment refers to. I think it can be deleted. test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java line 172: > 170: > 171: // Fetch JDK artifact depending on platform > 172: // If the artifact cannot be found, default to the test.boot.jdk > property This comment about test.boot.jdk is no longer valid. If the artifact cannot be found, RuntimeException is thrown. test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java line 199: > 197: architecture = "x"; > 198: } else if (Platform.isAArch64()) { > 199: architecture = "aarch"; It's better to use "x64" and "aarch64" here. The comments below should be fixed, too. test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java line 207: > 205: // Ex: bundles/linux-x64/jdk-19_linux-x64_bin.tar.gz > 206: if (Platform.isWindows()) { > 207: jdkArtifactMap.put("file", "bundles/windows-x64/jdk-" + > version + "_windows-x64_bin.zip"); Instead of hard-coding "x64", `architecture` should be used. test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java line 225: > 223: try { > 224: path = ArtifactResolver.resolve("jdk", jdkArtifactMap, true) > + "/jdk-" + version; > 225: System.out.println("Boot JDK path: " + path); `String path` should be declared inside this `try` block and its value should be returned here. That way you don't need to read all the catch block below to find out what value is actually returned. test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java line 230: > 228: if (cause == null) { > 229: System.out.println("Cannot resolve artifact, " > 230: + "please check if JIB jar is present in > classpath."); If we come to here, the function would end up returning null. We shold always throw the RuntimeException when we come into this `catch` block. Also, I am not sure if we should assume that `e.getCause() == null` has any specific meaning. There could be multiple implementations of `ArtifactResolver.resolve()` and we can't predict what the `e.getCause()` would be. ------------- PR: https://git.openjdk.org/jdk/pull/11852
