On Thu, 12 Jan 2023 03:02:51 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: > > Changed variable names and consolidated redundant code Changes requested by iklam (Reviewer). test/lib/jdk/test/lib/artifacts/ArtifactManager.java line 31: > 29: public interface ArtifactManager { > 30: public Path resolve(Artifact artifact) throws > ArtifactResolverException; > 31: Path resolve(String name, Map<String, Object> artifactDescription, > boolean unpack) throws ArtifactResolverException; For consistency, `public` should be added, similar to the existing interface method. Also, ArtifactManager is a public interface and some JDK developers may have written custom classes of this interface. If we add a new interface method, we may break such custom classes. For compatibility, the new method should be declared with a default implementation: default public Path resolve(String name, Map<String, Object> artifactDescription, boolean unpack) throws ArtifactResolverException { throw new ArtifactResolverException("not implemented"); } Note that it throws the exception instead of returning null. This will be consistent with the existing interface method, which throws ArtifactResolverException in case of any error. Otherwise, the caller of the new method would need to handle both the null return and the ArtifactResolverException, which would be too clumsy. test/lib/jdk/test/lib/artifacts/DefaultArtifactManager.java line 48: > 46: @Override > 47: public Path resolve(String name, Map<String, Object> > artifactDescription, boolean unpack) { > 48: return null; This should be removed as we already have a default implementation. ------------- PR: https://git.openjdk.org/jdk/pull/11852
