8350013: Add a test for JDK-8150442 #23825

alexeysemenyukoracle · 2025-02-27T14:17:30Z

Add a test to verify jpackage is using a custom MSI condition blocking package installation depending on the version of Windows where the package installer runs. Support for this MSI condition was added in JDK-8150442.

The test adds an unconditionally failing OS-version MSI condition to the resource directory. MSI installer using this condition should always fail. The exit code of the failed installation would be 1603. Extended error information can be dug in the MSI log file. To make the test work, the following changes to jpackage test lib have been made:

Support non-0 exit code from MSI install handler. Support for non-0 exit codes was added to install handlers of all other types too. Added PackageTest.setExpectedInstallExitCode(int) method to configure the expected exit code of a package installation;
Support using msi log files when MSI and EXE packages get installed, unpacked, or uninstalled. Added PackageTest.createMsiLog(boolean) to enable or disable creation of msi log files in a PackageTest instance and Optional<Path> JPackageCommand.winMsiLogFile() to access the current log file from the callbacks registered with the PackageTest instance.

Added tests for PackageTest class (PackageTestTest).

Additionally, improved the code in WindowsHelper detecting paths to Start Menu, Desktop, and other common paths. Previously, it relied on reading these paths from the registry. On some machines, required registry keys are not available. The code now will call .NET Environment.GetFolderPath() function through powershell if a registry key is missing.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8350013: Add a test for JDK-8150442 (Enhancement - P4)

Reviewers

Alexander Matveev (@sashamatveev - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23825/head:pull/23825
$ git checkout pull/23825

Update a local copy of the PR:
$ git checkout pull/23825
$ git pull https://git.openjdk.org/jdk.git pull/23825/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23825

View PR using the GUI difftool:
$ git pr show -t 23825

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23825.diff

Using Webrev

Link to Webrev Comment

…m in IDE this way.

…upplied by JUnit

@test

…annotation if the class has a function with the given name and @test annotation

This reverts commit 45a2ab5.

…only in the test work directory but in the sub-directories too.

…teTempFile() tests

…createTempFile() methods

…st installation failures.

…s. Make WindowsHelper use PS as alternative to reading registry values to get paths of special folders

…They belong to different branch

bridgekeeper · 2025-02-27T14:18:27Z

👋 Welcome back asemenyuk! 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 · 2025-02-27T14:19:02Z

@alexeysemenyukoracle 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 details.

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

8350013: Add a test for JDK-8150442

Reviewed-by: almatvee

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 40 new commits pushed to the master branch:

3c72c04: 8350818: Improve OperatingSystemMXBean cpu load tests to not accept -1.0 by default
6b719ee: 8348596: Update FreeType to 2.13.3
a87dd1a: 8347139: [macos] Test tools/jpackage/share/InOutPathTest.java failed: "execution error: Finder got an error: AppleEvent timed out."
c7fa499: 8350118: Simplify the layout access VarHandle
fb659eb: 8350915: [JMH] test SocketChannelConnectionSetup failed for 2 threads config
3a7d986: 8350646: Calendar.Builder.build() Throws ArrayIndexOutOfBoundsException
197004f: 8350820: OperatingSystemMXBean CpuLoad() methods return -1.0 on Windows
e98df71: 8348028: Unable to run gtests with CDS enabled
d6c4be6: 8350758: G1: Use actual last prediction in accumulated survivor rate prediction too
2019f44: 8343832: Enhance test summary with number of skipped tests
... and 30 more: https://git.openjdk.org/jdk/compare/0f82268134df65bbc65ecda158d25f708f18d150...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 · 2025-02-27T14:19:35Z

⚠️ @alexeysemenyukoracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2025-02-27T14:19:48Z

@alexeysemenyukoracle 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 pull request command.

alexeysemenyukoracle · 2025-02-27T17:34:10Z

@sashamatveev PTAL

mlbridge · 2025-02-27T17:38:04Z

Webrevs

sashamatveev

Looks good with minor comments.

sashamatveev · 2025-02-27T22:53:13Z

test/jdk/tools/jpackage/helpers-test/jdk/jpackage/test/PackageTestTest.java

+        }
+
+        void configureUninstallVerifiers(PackageTest test, Consumer<Verifiable> verifiableAccumulator) {
+            for (final var verifier  : uninstallVerifiers) {


Extra space after verifier.

sashamatveev · 2025-02-27T23:48:19Z

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageTest.java

+                case SKIP_NO_PACKAGE_HANDLER -> {
+                    TKit.trace(String.format("No handler of [%s] action for %s command",
+                            action, cmd.getPrintableCommandLine()));
+                    return;


Do we really need return here and at line 495? Also, do you think it might be better to make switch consistent? For example this one uses -> and below :.

Do we really need return here and at line 495?

Yes. It should exit the function early to avoid jumping in the next switch block.

do you think it might be better to make switch consistent?

Totally!

Yes. It should exit the function early to avoid jumping in the next switch block.

With -> it should not fall into next case. Maybe I am missing something.

With -> it should not fall into next case. Maybe I am missing something.

Right, but there are two siwtch blocks in the function. Without return the control flow will get to the second one. return-s prevents this.

alexeysemenyukoracle added 30 commits February 13, 2025 08:07

Convert helpers-test tests to junit format. It is easier to debug the…

79719ee

…m in IDE this way.

Make junit pass

115f4ef

Manual merge

2d929b1

Fix AnnotationsTest to create test work directory in temp directory s…

c66c591

…upplied by JUnit

Fix bug in TestBuilder that created tests for functions without @test …

5b9d7e0

…annotation if the class has a function with the given name and @test annotation

Can use @ParameterSupplier annotation without a value.

eb36f4d

Undo unrelated change

45a2ab5

Merge branch 'decouple-junit' into decouple-fix-annotations

bd6018a

Revert "Undo unrelated change"

45eec0e

This reverts commit 45a2ab5.

Minor

c216296

Fix typo

292d8fb

Merge branch 'decouple-junit' into decouple-fix-annotations

c4215e4

Decouple Executor.Result and Executor classes

691c5f9

Bugfix Executor.createResult() and add Executor.getExecutable()

781100e

Rework TKit.createTempFile() to make it create unique file names not …

5922465

…only in the test work directory but in the sub-directories too.

Cleanup

a0d6625

Unified TKit.createTempFile() and TKit.createTempDirectory()

5b43d3f

Better shape TKitTest.testCreateTempDirectory() and TKitTest.testCrea…

12c810b

…teTempFile() tests

Remove "throws IOException" from TKit.createTempDirectory() and TKit.…

dcf393a

…createTempFile() methods

Allow install action exit with non-zero exit code. Can be handy to te…

87050e7

…st installation failures.

Added tests for PackageTest class

d26497f

Manual merge

929b356

Better test coverage for PackageTest

e60f42a

Add a test for OS-version condition

0f52bcf

Added WindowsHelper.lastMsiLogFile()

e6ba3db

Revisit msi log configuration. Decouple internals of PackageTest clas…

26fffa7

…s. Make WindowsHelper use PS as alternative to reading registry values to get paths of special folders

Rerun only unpack if exit code is 1603

7c59054

Improve PackageTest.PackageTypePipeline implementation

b5fb3bf

Remove TKit.assertEquals(boolean) and TKit.assertNotEquals(boolean). …

d0c815d

…They belong to different branch

Merge branch 'decouple-junit' into decouple-fix-annotations

399c15f

alexeysemenyukoracle added 13 commits February 13, 2025 10:53

Merge branch 'decouple-tkit' into decouple-os-condition

294136b

Remove redundant "return"

ae5ddc7

Merge branch 'master' into decouple-junit

2c39345

Merge branch 'decouple-junit' into decouple-fix-annotations

09cec2f

Fix javac error

1247c84

Merge branch 'decouple-fix-annotations' into decouple-executor-result

f7d104c

Merge branch 'decouple-executor-result' into decouple-tkit

937c46f

Test if the main jar is in the app image if specified.

74c2ac5

Merge branch 'master' into decouple-tkit

e6614c0

Better test description and add tests for roles with dots

01fa252

Better log message

e1a7d43

Merge branch 'decouple-tkit' into decouple-os-condition

56d6326

Merge branch 'master' into decouple-os-condition

9d24e48

openjdk bot added the core-libs [email protected] label Feb 27, 2025

Cleanup

a27cb52

alexeysemenyukoracle marked this pull request as ready for review February 27, 2025 17:33

openjdk bot added the rfr Pull request is ready for review label Feb 27, 2025

sashamatveev reviewed Feb 27, 2025

View reviewed changes

Cleanup

43ff870

openjdk bot removed the rfr Pull request is ready for review label Feb 28, 2025

Trailing whitespace cleanup

a38876d

openjdk bot added the rfr Pull request is ready for review label Feb 28, 2025

sashamatveev approved these changes Feb 28, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8350013: Add a test for JDK-8150442 #23825

8350013: Add a test for JDK-8150442 #23825

alexeysemenyukoracle commented Feb 27, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 27, 2025

openjdk bot commented Feb 27, 2025 •

edited

Loading

openjdk bot commented Feb 27, 2025

openjdk bot commented Feb 27, 2025

alexeysemenyukoracle commented Feb 27, 2025

mlbridge bot commented Feb 27, 2025 •

edited

Loading

sashamatveev left a comment

sashamatveev Feb 27, 2025

sashamatveev Feb 27, 2025

alexeysemenyukoracle Feb 28, 2025

sashamatveev Feb 28, 2025 •

edited

Loading

alexeysemenyukoracle Feb 28, 2025 •

edited

Loading

8350013: Add a test for JDK-8150442 #23825

Are you sure you want to change the base?

8350013: Add a test for JDK-8150442 #23825

Conversation

alexeysemenyukoracle commented Feb 27, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 27, 2025

openjdk bot commented Feb 27, 2025 • edited Loading

openjdk bot commented Feb 27, 2025

openjdk bot commented Feb 27, 2025

alexeysemenyukoracle commented Feb 27, 2025

mlbridge bot commented Feb 27, 2025 • edited Loading

Webrevs

sashamatveev left a comment

Choose a reason for hiding this comment

sashamatveev Feb 27, 2025

Choose a reason for hiding this comment

sashamatveev Feb 27, 2025

Choose a reason for hiding this comment

alexeysemenyukoracle Feb 28, 2025

Choose a reason for hiding this comment

sashamatveev Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

alexeysemenyukoracle Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

alexeysemenyukoracle commented Feb 27, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 27, 2025 •

edited

Loading

mlbridge bot commented Feb 27, 2025 •

edited

Loading

sashamatveev Feb 28, 2025 •

edited

Loading

alexeysemenyukoracle Feb 28, 2025 •

edited

Loading