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.

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

Add tests for Math max and min #2973

Merged
merged 1 commit into from
Sep 27, 2018
Merged

Add tests for Math max and min #2973

merged 1 commit into from
Sep 27, 2018

Conversation

dhong44
Copy link
Contributor

@dhong44 dhong44 commented Sep 21, 2018

Add tests for Math max and min.

Signed-off-by: Daniel Hong daniel.hong@live.com

@fjeremic fjeremic self-assigned this Sep 21, 2018
fjeremic
fjeremic requested changes Sep 24, 2018
View reviewed changes
Copy link
Contributor

@fjeremic fjeremic left a comment

Choose a reason for hiding this comment

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

@dhong44 I think we'll need to modify playlist.xml or testing.xml to add some JIT options to these tests. I want to make sure they are deterministic, i.e. with count=1,disableAsyncCompilation on the test classes so that we make sure we JIT compile and run the tests correctly.

@dhong44 dhong44 force-pushed the maxMinTests branch 3 times, most recently from 28db887 to a486aa2 Compare September 24, 2018 22:54
@dhong44 dhong44 changed the title WIP: Add tests for Math max and min Add tests for Math max and min Sep 26, 2018
@dhong44
Copy link
Contributor Author

dhong44 commented Sep 26, 2018

@fjeremic I've updated the tests as you recommended in #2973 (review). Have also confirmed that these tests do expose corner cases such as the one patched in eclipse-omr/omr#2967 (comment). This is now ready for review.

fjeremic
fjeremic reviewed Sep 26, 2018
View reviewed changes
test/functional/Java8andUp/src/org/openj9/test/java/lang/Test_Math.java Outdated
@@ -25,7 +25,7 @@
import org.testng.annotations.Test;
import org.testng.Assert;

@Test(groups = { "level.sanity" })
@Test(groups = { "level.sanity" }, invocationCount = 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need 10 invocations with count=1? Shouldn't 2 do just fine and cut down on test execution time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2e5aa80

test/functional/Java8andUp/playlist.xml Outdated
<test>
<testCaseName>JCL_TEST_MathMethods</testCaseName>
<variations>
<variation>-Xjit:count=1,disableAsyncCompilation</variation>
Copy link
Contributor

@fjeremic fjeremic Sep 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Could we perhaps limit down to the Test_Math class? This is unnecessarily going to synchronously compile several hundred methods before we ever get to executing the tests which will slow down all the other testing.

i.e.

-Xjit:disableAsyncCompilation,{*Test_Math*}(count=1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a7cc1a9

fjeremic
fjeremic reviewed Sep 26, 2018
View reviewed changes
test/functional/Java8andUp/src/org/openj9/test/java/lang/Test_Math.java Outdated
@@ -0,0 +1,68 @@
package org.openj9.test.java.lang;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this below the copyright to stay consistent with the other files in the codebase?

Copy link
Contributor Author

@dhong44 dhong44 Sep 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The tests in this folder did not follow a consistent formatting, in 088b765 all have been changed to follow the formatting in https://github.com/eclipse/openj9/blob/088b765f32c38597096ad0f626e099cd865edeaf/test/functional/Java11andUp/src/org/openj9/test/nestmate/NestmateTest.java#L1-L29

Formatting being, copyright at the beginning of the file with no empty lines, followed package declaration, an empty line and then import statements.

@fjeremic
Copy link
Contributor

Jenkins test sanity xlinux,zlinux JDK8,JDK11

fjeremic
fjeremic requested changes Sep 26, 2018
View reviewed changes
test/functional/Java8andUp/src/org/openj9/test/java/lang/Test_RuntimePermission.java Outdated
import org.testng.annotations.BeforeMethod;
import org.testng.AssertJUnit;

package org.openj9.test.java.lang;
Copy link
Contributor

Choose a reason for hiding this comment

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

@dhong44 some test failures because of this. The package is already defined. Looks like a copy/paste error. Can you make the appropriate fixes and I'll relaunch the builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1a2c592

@fjeremic
Copy link
Contributor

Jenkins test sanity xlinux,zlinux JDK8,JDK11

@AdamBrousseau
Copy link
Contributor

Jenkins line endings check

@fjeremic
Copy link
Contributor

fjeremic commented Sep 26, 2018 •
edited
Loading

@dhong44 some issues with the builds:

15:59:56 if [ $? -eq 0 ] ; then echo ""; echo "JCL_TEST_MathMethods_0""_PASSED"; echo ""; else echo ""; echo "JCL_TEST_MathMethods_0""_FAILED"; echo ""; fi; } 2>&1 | tee -a "/home/jenkins/jenkins-agent/workspace/PullRequest-Sanity-JDK8-linux_390-64_cmprssptrs-OpenJ9/openj9/test/TestConfig/scripts/testKitGen/../../../TestConfig/test_output_15379904224680/TestTargetResult";
15:59:56 /bin/sh: 1: Syntax error: "(" unexpected (expecting "}")
15:59:56 autoGen.mk:1020: recipe for target 'JCL_TEST_MathMethods_0' failed

Can you please take a look? Might want to try running in a Docker image to see what is wrong and get it working.

@dhong44
Copy link
Contributor Author

dhong44 commented Sep 26, 2018

Seems like it was a problem with the parentheses in the variation options from a7cc1a9, fixed in
18064ce.

@fjeremic
Copy link
Contributor

@dhong44 can we use single quotes to avoid having to escape anything (now and in the future). Here is an example:
https://github.com/eclipse/openj9/pull/2665/files#diff-be4413663bce2480d3a88638578a34b8R77

@dhong44 dhong44 force-pushed the maxMinTests branch 2 times, most recently from 0a98d62 to 711c5ee Compare September 27, 2018 04:37
@dhong44
Add tests for Math max and min
9cd678b 
Add tests for Math max and min.

Signed-off-by: Daniel Hong <daniel.hong@live.com>
@dhong44
Copy link
Contributor Author

dhong44 commented Sep 27, 2018

I've changed the command line options to use single quotes. Since the changes have already
been approved, I've done a squash and rebase as well.

@fjeremic
Copy link
Contributor

Jenkins test sanity xlinux,zlinux JDK8,JDK11

@fjeremic fjeremic merged commit 2aedb8a into eclipse-openj9:master Sep 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjeremic fjeremic fjeremic approved these changes

Assignees

@fjeremic fjeremic

Labels
comp:test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhong44 @fjeremic @AdamBrousseau