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.

Sign up for GitHub

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

Jump to bottom

JDK-8292066 Convert TestInputArgument.sh and TestSystemLoadAvg.sh to java version #9848

Closed

bwhuang-us wants to merge 6 commits into openjdk:master from bwhuang-us:JDK-8292066

Contributor

bwhuang-us commented Aug 11, 2022 •

edited by openjdk bot

Loading

This task converts 2 shell tests to java version.
test/java/lang/management/OperatingSystemMXBean/TestSystemLoadAvg.sh
test/java/lang/management/RuntimeMXBean/TestInputArgument.sh

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-8292066: Convert TestInputArgument.sh and TestSystemLoadAvg.sh to java version

Reviewers

Leonid Mesnik (@lmesnik - Reviewer)
Alex Menkov (@alexmenkov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9848

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

Using diff file

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

bwhuang-us added 3 commits

August 9, 2022 16:25


          Coverted TestSystemLoadAvg shell test to java version.

a3eb7a5


          Converted TestInputArgument.sh to java version.

ba8d832


          Updated copyright header.

41a4548

bwhuang-us changed the title ~~JDK-8292066~~ JDK-8292066 Convert TestInputArgument.sh and TestSystemLoadAvg.sh to java version

bridgekeeper bot commented Aug 11, 2022

👋 Welcome back bwhuang-us! 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 bot added the rfr label

openjdk bot commented Aug 11, 2022

@bwhuang-us The following labels will be automatically applied to this pull request:

core-libs
serviceability

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

openjdk bot added serviceability core-libs labels

mlbridge bot commented Aug 11, 2022 •

edited

Loading

Webrevs

Contributor

AlanBateman commented Aug 12, 2022

/label remove core-libs

openjdk bot removed the core-libs label

openjdk bot commented Aug 12, 2022

@AlanBateman
The core-libs label was successfully removed.

Contributor Author

bwhuang-us commented Aug 26, 2022

@lmesnik Could you please review this PR?

lmesnik reviewed

View reviewed changes

test/jdk/java/lang/management/OperatingSystemMXBean/GetSystemLoadAverage.java Outdated

-                      if (args.length == 0) {
+                  @Test(invocationCount = 5, timeOut = 300)
+                  void testSystemLoadAvg() throws Exception {
+                      if (!OS.contains("Win")) {

Member

lmesnik Aug 29, 2022

Check /test/lib/jdk/test/lib/Platform.java, it contains "Platform.isWindows()" which could be used for this.

test/jdk/java/lang/management/OperatingSystemMXBean/GetSystemLoadAverage.java Outdated

		@@ -54,25 +58,25 @@ public class GetSystemLoadAverage {
		// Allow some delta.
		private static double DELTA = 0.05;

Member

lmesnik Aug 29, 2022

Not a part of your fix but could you make it final?

test/jdk/java/lang/management/OperatingSystemMXBean/GetSystemLoadAverage.java Outdated


		if (args.length == 0) {
		@Test(invocationCount = 5, timeOut = 300)

Member

lmesnik Aug 29, 2022

I am not sure it is the correct replacement. Accordingly to TestNG doc the invocationCount = 5 means that TestNG calls the test 5 times. And test fails if any of the invocations fail while the bash script makes 5 attempts and passes if testcase passed in any of them.

Contributor Author

bwhuang-us Aug 29, 2022

Good catch! I've missed the exit statement in the original shell script.

bwhuang-us added 2 commits

August 29, 2022 16:19


          Implemented review comments.

8d17ede


          Added a blank line between fields and methods.

21e7ce8

lmesnik reviewed

View reviewed changes

test/jdk/java/lang/management/OperatingSystemMXBean/GetSystemLoadAverage.java Outdated

+                              System.out.println(
+                                      String.format("Run %d: TestSystemLoadAvg", i));
+                              if (!Platform.isWindows()) {
+                                  // On Linux or Solaris

Member

lmesnik Aug 29, 2022

Sorry, if missed the first time. It is Linux or Mac, or better something like *mix.

test/jdk/java/lang/management/OperatingSystemMXBean/GetSystemLoadAverage.java Outdated

+                                                      "failed %d runs",
+                                              i));
+                              if (i == MAX_RETRIES)
+                              {

Member

lmesnik Aug 29, 2022

please move move brace to the prev line


          Implemented more review comments.

9ae1663

lmesnik approved these changes

View reviewed changes

openjdk bot commented Aug 30, 2022 •

edited

Loading

@bwhuang-us 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:

8292066: Convert TestInputArgument.sh and TestSystemLoadAvg.sh to java version

Reviewed-by: lmesnik, amenkov

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

fa68371: 8292584: assert(cb != __null) failed: must be with -XX:-Inline
04d8069: 8230374: maxOutputSize, instead of javatest.maxOutputSize, should be used in TEST.properties
3d254d3: 8289510: Improve test coverage for XPath Axes: namespace
2d18dda: 8173605: Remove support for source and target 1.7 option in javac
7c2f299: 8293202: Document how to edit doc/testing, doc/building
07616de: 8175382: clhsdb pmap should print the end addresses of the load modules
9444a08: 8290709: Incorrect dominance error for unconditional pattern vs. null
6a1b0b5: 8293154: TemporalQueries java doc error
5204528: 8293201: Library detection in runtime/ErrorHandling/TestDwarf.java fails on some systems
2d10d4f: 8291651: CleanerTest.java fails with "Cleanable was cleaned"
... and 260 more: https://git.openjdk.org/jdk/compare/ae1a9a0b252fc9593b077f9626a431b0f06b3f51...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.

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 (@lmesnik, @alexmenkov) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk bot added the ready label

Contributor Author

bwhuang-us commented Aug 30, 2022

/integrate

openjdk bot added the sponsor label

openjdk bot commented Aug 30, 2022

@bwhuang-us
Your change (at version 9ae1663) is now ready to be sponsored by a Committer.

alexmenkov approved these changes

View reviewed changes

Member

lmesnik commented Sep 1, 2022

/sponsor

openjdk bot commented Sep 1, 2022

Going to push as commit 032be16.
Since your change was applied there have been 271 commits pushed to the master branch:

e393973: 8292990: Improve test coverage for XPath Axes: parent
fa68371: 8292584: assert(cb != __null) failed: must be with -XX:-Inline
04d8069: 8230374: maxOutputSize, instead of javatest.maxOutputSize, should be used in TEST.properties
3d254d3: 8289510: Improve test coverage for XPath Axes: namespace
2d18dda: 8173605: Remove support for source and target 1.7 option in javac
7c2f299: 8293202: Document how to edit doc/testing, doc/building
07616de: 8175382: clhsdb pmap should print the end addresses of the load modules
9444a08: 8290709: Incorrect dominance error for unconditional pattern vs. null
6a1b0b5: 8293154: TemporalQueries java doc error
5204528: 8293201: Library detection in runtime/ErrorHandling/TestDwarf.java fails on some systems
... and 261 more: https://git.openjdk.org/jdk/compare/ae1a9a0b252fc9593b077f9626a431b0f06b3f51...master

Your commit was automatically rebased without conflicts.

openjdk bot added the integrated label

openjdk bot closed this

openjdk bot removed ready rfr sponsor labels

openjdk bot commented Sep 1, 2022

@lmesnik @bwhuang-us Pushed as commit 032be16.

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

bwhuang-us deleted the JDK-8292066 branch

March 1, 2023 18:24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integrated serviceability