On Mon, 21 Sep 2020 14:39:33 GMT, Alexey Semenyuk <asemen...@openjdk.org> wrote:
> How about testing of other jpackage command line options in two phase mode?
> Like "--name", "--version"? Any plans to
> add them?
Plan was to file separate bugs for any additional options. I do not think we
should put everything in one issue. This
issue for desktop integration for additional launchers.
> 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.
Fixed.
> 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.
Fixed.
> 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.
Fixed.
> 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.
Fixed.
> 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
Fixed. I also removed code from AdditionalLaunchers.
-------------
PR: https://git.openjdk.java.net/jdk/pull/263
