On Sat, 19 Sep 2020 02:42:27 GMT, Alexander Matveev <almat...@openjdk.org> wrote:
> https://bugs.openjdk.java.net/browse/JDK-8231591 > > - Added MultiLauncherTwoPhaseTest which uses predefine app image with > multiple launcher and tests to make sure installer > will create shortcuts for all launchers. > - Fixed Linux DesktopIntegration to create shortcuts for additional launcher > if we using pre-define app image. How about testing of other jpackage command line options in two phase mode? Like "--name", "--version"? Any plans to add them? src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/DesktopIntegration.java line 549: > 547: private final List<LinuxFileAssociation> associations; > 548: > 549: private static boolean initAppImageLaunchers = true; What is the logic behind static variable? test/jdk/tools/jpackage/helpers/jdk/jpackage/test/WindowsHelper.java line 140: > 138: cmd.verifyIsOfType(PackageType.WINDOWS); > 139: this.cmd = cmd; > 140: this.name = name; I'd suggest to have the following initialization of "name" field: `this.name = (name == null ? cmd.name() : name)` This will help to avoid multiple `(name == null ? cmd.name() : name)` in the code. test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java line 54: > 52: public class MultiLauncherTwoPhaseTest { > 53: > 54: public static void main(String[] args) throws Exception { Please replace main() function with dedicated test function. You can use SimplePackageTest as a template, This would give better control on how to run the test in debug mode. test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java line 94: > 92: launcher2.verify(cmd); > 93: launcher2.verifyPackageInstalled(cmd); > 94: }); Looks like a duplicate of verify logic added with the preceding addInstallVerifier() call. test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java line 80: > 78: }) > 79: .addInstallVerifier(cmd -> { > 80: launcher1.verify(cmd); There is no need in explicit call to AdditionalLauncher.verify() as this function is scheduled for the call from AdditionalLauncher.applyTo() call. Thus there is no need to change access to this function from "private" to "public" in your patch to AdditionalLauncher.java. test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java line 56: > 54: public static void main(String[] args) throws Exception { > 55: TKit.run(args, () -> { > 56: Path appimageOutput = TKit.workDir().resolve("appimage"); Please use TKit.createTempDirectory() to create directories for intermediate data of test runs. This allows clean multiple runs of the test in the same work directory. test/jdk/tools/jpackage/helpers/jdk/jpackage/test/AdditionalLauncher.java line 271: > 269: } else { > 270: TKit.assertPathExists(appInstallDir, false); > 271: } I don't understand what is going on here and why branching is required. Also seems like verifyPackageUninstalled() function is never called. test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageTest.java line 559: > 557: && !cmd.isPackageUnpacked( > 558: "Not verifying desktop integration")) { > 559: new WindowsHelper.DesktopIntegrationVerifier(cmd, > null); I'd rather add a loop calling DesktopIntegrationVerifier() for all launchers than copy/paste the code in AdditionalLauncher.java ------------- Changes requested by asemenyuk (Committer). PR: https://git.openjdk.java.net/jdk/pull/263
