8297141: Fix hotspot/test/runtime/SharedArchiveFile/DefaultUseWithClient.java for 8u #181
Conversation
|
👋 Welcome back zzambers! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Shouldn't we just amend the condition on lines 41-45 to something like:
boolean is32BitWindowsClientVm = (Platform.isWindows() && Platform.is32bit() && Platform.isClient());
if (!is32BitWindowsClientVm) {
System.out.println("Test only applicable on 32-bit Windows Client VM. Skipping");
return;
}
|
@jerboaa Oh, that is indeed cleaner. I have not checked Platform class and I did not know it is capable of that. I'll update my PR. Thanks |
|
Even though, I should test that VM launched with "-client" uses Client VM (maybe case where it has Client VM but it is not default). I would then probably need to run test in othervm and with -client arg as well. |
|
Updated detection code. Tested on both default build [5] (without Client VM) and build with Client VM [6] with windows-x86 testing enabled. Passed both. [5] https://github.com/zzambers/jdk8u-dev/actions/runs/3482764139 |
|
@zzambers This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: 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 5 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jerboaa) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@jerboaa Thanks for the review (but still needs jdk8u-fix-yes) |
|
/integrate |
|
/sponsor |
|
Going to push as commit 5a32484.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
Following test (from hotspot/tier1) currently fails on Windows x86:
hotspot/test/runtime/SharedArchiveFile/DefaultUseWithClient.javaTest output:
Problem:
Test is Windows 32-bit only, only applies to Client VM and checks default behaviour of shared archive feature. Problem is, that default build of 32-bit windows JDK does not include Client VM, so Server VM is used (so -client arg does nothing). With Server VM tests fails. I tried to make build which has Client VM and then this test passes (It breaks other tests which expect default to be Server VM though).
Fix:
Test runs java with
-versionargument, which can print something similar to:This output can be used to check if Client VM and skip other checks, if Client VM is not used. Fix is JDK 8 only as test has been removed on newer JDKs by JDK-8154204 by [2]. But I decided to fix it for JDK 8.
Testing:
With this change test passes on Windows 32-bit. (both with Server [3] and Client [4] Vms)
[1] https://github.com/zzambers/jdk8u-dev/actions/runs/3438556844
[2] https://bugs.openjdk.org/browse/JDK-8154204
[3] https://github.com/zzambers/jdk8u-dev/actions/runs/3462926725
[4] https://github.com/zzambers/jdk8u-dev/actions/runs/3462887672
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev pull/181/head:pull/181$ git checkout pull/181Update a local copy of the PR:
$ git checkout pull/181$ git pull https://git.openjdk.org/jdk8u-dev pull/181/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 181View PR using the GUI difftool:
$ git pr show -t 181Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/181.diff