Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8231591: [TESTBUG] Create additional two phase jpackage tests #263

Closed
wants to merge 4 commits into from

Conversation

sashamatveev
Copy link
Member

@sashamatveev sashamatveev commented Sep 19, 2020

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.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8231591: [TESTBUG] Create additional two phase jpackage tests

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/263/head:pull/263
$ git checkout pull/263

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 19, 2020

👋 Welcome back almatvee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 19, 2020
@openjdk
Copy link

openjdk bot commented Sep 19, 2020

@sashamatveev The following label will be automatically applied to this pull request: core-libs.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

@openjdk openjdk bot added the core-libs label Sep 19, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 19, 2020

Webrevs

Copy link
Member

@alexeysemenyukoracle alexeysemenyukoracle left a comment

How about testing of other jpackage command line options in two phase mode? Like "--name", "--version"? Any plans to add them?

@@ -524,6 +546,7 @@ private static String stringifyShellCommands(List<String> commands) {

private final List<LinuxFileAssociation> associations;

private static boolean initAppImageLaunchers = true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the logic behind static variable?

Copy link
Member Author

@sashamatveev sashamatveev Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used to avoid infinite calls in DesktopIntegration(). I modified it by removing PREDEFINED_APP_IMAGE from launcherParams.

cmd.verifyIsOfType(PackageType.WINDOWS);
this.cmd = cmd;
this.name = name;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@sashamatveev sashamatveev Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


public class MultiLauncherTwoPhaseTest {

public static void main(String[] args) throws Exception {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@sashamatveev sashamatveev Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

.addInstallVerifier(cmd -> {
launcher1.verify(cmd);
launcher1.verifyPackageInstalled(cmd);
launcher2.verify(cmd);
launcher2.verifyPackageInstalled(cmd);
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a duplicate of verify logic added with the preceding addInstallVerifier() call.

"--win-menu-group", "MultiLauncherTwoPhaseTest");
})
.addInstallVerifier(cmd -> {
launcher1.verify(cmd);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@sashamatveev sashamatveev Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


public static void main(String[] args) throws Exception {
TKit.run(args, () -> {
Path appimageOutput = TKit.workDir().resolve("appimage");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@sashamatveev sashamatveev Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Path appInstallDir = cmd.appInstallationDirectory();
if (TKit.isLinux() && Path.of("/").equals(appInstallDir)) {
ApplicationLayout appLayout = cmd.appLayout();
TKit.assertPathExists(appLayout.runtimeDirectory(), false);
} else {
TKit.assertPathExists(appInstallDir, false);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what is going on here and why branching is required. Also seems like verifyPackageUninstalled() function is never called.

new WindowsHelper.DesktopIntegrationVerifier(cmd);
new WindowsHelper.DesktopIntegrationVerifier(cmd, null);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather add a loop calling DesktopIntegrationVerifier() for all launchers than copy/paste the code in AdditionalLauncher.java

Copy link
Member Author

@sashamatveev sashamatveev Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I also removed code from AdditionalLaunchers.

@sashamatveev
Copy link
Member Author

sashamatveev commented Sep 22, 2020

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.

@andyherrick
Copy link

andyherrick commented Sep 22, 2020

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.

Agree that separate bugs can be filed for additional options, can you file them ?
I am testing in conjunction with #271

@@ -618,6 +633,7 @@ private void verifyPackageUninstalled(JPackageCommand cmd) {
private Map<PackageType, Handler> handlers;
private Set<String> namedInitializers;
private Map<PackageType, PackageHandlers> packageHandlers;
private final List<String> launchersName = new ArrayList();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the variable name "launcherNames" be more accurate?

@@ -317,6 +317,11 @@ public PackageTest configureHelloApp(String javaAppDesc) {
return this;
}

public PackageTest addLauncherName(String name) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to make this function public and explicitly call it from tests.
It would be better to add call to this function from AdditionalLauncher.applyTo(PackageTest).

Copy link
Member Author

@sashamatveev sashamatveev Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But test calls AdditionalLauncher.applyTo(JPackageCommand cmd), which does not have reference to PackageTest. Do you know why we need two applyTo()?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just change the test code to call AdditionalLauncher.applyTo(PackageTest):
launcher1.applyTo(packageTest); launcher2.applyTo(packageTest)

AdditionalLauncher.applyTo(JPackageCommand) is for non-packaging tests.

Copy link
Member Author

@sashamatveev sashamatveev Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it will not work. I am running appImageCmd command before test executes to generate app image. If I add it to packageTest, jpackage will fail when trying to run command with app-image and launchers.

Copy link
Member Author

@sashamatveev sashamatveev Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep addLauncherName() inside test to add launchers manually and will also call it in applyTo(PackageTest) for future tests, that might use such functionality.

@openjdk
Copy link

openjdk bot commented Sep 25, 2020

@sashamatveev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for more details.

After integration, the commit message for the final commit will be:

8231591: [TESTBUG] Create additional two phase jpackage tests

Reviewed-by: asemenyuk, herrick

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 156 new commits pushed to the master branch:

  • b159e4e: 8253149: Building an installer from invalid app image fails on Window…
  • 0e855fe: 8252377: Incorrect encoding for EC AlgorithmIdentifier
  • 9150b90: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
  • 0187567: 8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 …
  • a75edc2: 8251188: Update LDAP tests not to use wildcard addresses
  • 1f5a033: 8253555: Make ByteSize and WordSize typed scoped enums
  • f62eefc: 8253469: ARM32 Zero: replace usages of __sync_synchronize() with OrderAccess::fence
  • 1b79326: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
  • dc1ef58: 8253631: Remove unimplemented CompileBroker methods after JEP-165
  • 27d0a70: 8253633: Remove unimplemented TieredThresholdPolicy::set_carry_if_neccessary
  • ... and 146 more: https://git.openjdk.java.net/jdk/compare/ac9d1b08ee4a9f54cd75a4115adffcc4c0fb105d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Sep 25, 2020
@sashamatveev
Copy link
Member Author

sashamatveev commented Sep 25, 2020

/integrate

@openjdk openjdk bot closed this Sep 25, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 25, 2020
@openjdk
Copy link

openjdk bot commented Sep 25, 2020

@sashamatveev Since your change was applied there have been 156 commits pushed to the master branch:

  • b159e4e: 8253149: Building an installer from invalid app image fails on Window…
  • 0e855fe: 8252377: Incorrect encoding for EC AlgorithmIdentifier
  • 9150b90: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
  • 0187567: 8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 …
  • a75edc2: 8251188: Update LDAP tests not to use wildcard addresses
  • 1f5a033: 8253555: Make ByteSize and WordSize typed scoped enums
  • f62eefc: 8253469: ARM32 Zero: replace usages of __sync_synchronize() with OrderAccess::fence
  • 1b79326: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
  • dc1ef58: 8253631: Remove unimplemented CompileBroker methods after JEP-165
  • 27d0a70: 8253633: Remove unimplemented TieredThresholdPolicy::set_carry_if_neccessary
  • ... and 146 more: https://git.openjdk.java.net/jdk/compare/ac9d1b08ee4a9f54cd75a4115adffcc4c0fb105d...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5a57945.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
3 participants