Test Java12 MethodHandle changes #4689

theresa-m · 2019-02-11T21:43:17Z

Signed-off-by: Theresa Mammarella Theresa.T.Mammarella@ibm.com

ChengJin01 · 2019-02-11T22:08:56Z

test/functional/Jsr292/src/com/ibm/j9/jsr292/FilterArgumentsWithCombinerTest.java

+    /* test filterArgumentsWithCombiner for data type byte */
+    @Test(groups = { "level.extended" })
+	public void test_filterArgumentsWithCombiner_Byte() throws WrongMethodTypeException, Throwable {
+        MethodHandle result;


Better declare when using it in the test.
e.g. MethodHandle result = (MethodHandle)fawcMethod.invoke(...)
result = ...

ChengJin01 · 2019-02-11T22:11:35Z

test/functional/Jsr292/src/com/ibm/j9/jsr292/FilterArgumentsWithCombinerTest.java

+    }
+
+    /* combiner returns highest byte value */
+    public static byte combinerByte(byte v1, byte v2) {


All test methods like this should be moved to a separate file as this file only contains test cases.

I'm not sure I agree with that as it introduces another helper file. Tests are already marked with an annotation. What's the rationale for moving the helper methods?

DanHeidinga · 2019-02-12T20:27:26Z

test/functional/Jsr292/src/com/ibm/j9/jsr292/FilterArgumentsWithCombinerTest.java

+    private final static boolean constBoolMin = false;
+
+    private final static String MHClassName = "java.lang.invoke.MethodHandles";
+    private static Class<?> mhClazz;


private static Class<?> mhClazz = MethodHandles.class;

test/functional/Jsr292/src/com/ibm/j9/jsr292/FilterArgumentsWithCombinerTest.java

DanHeidinga · 2019-02-12T20:41:22Z

test/functional/Jsr292/src/com/ibm/j9/jsr292/FilterArgumentsWithCombinerTest.java

+        Double d1 = new Double (constDouble1);
+        Double d2 = new Double (constDouble2);
+        Double dMax = new Double (Double.MAX_VALUE);
+		Double dMin = new Double (Double.MIN_VALUE);


test/functional/Jsr292/src/com/ibm/j9/jsr292/FilterArgumentsWithCombinerTest.java

DanHeidinga · 2019-02-12T20:43:04Z

test/functional/Jsr292/src/com/ibm/j9/jsr292/FilterArgumentsWithCombinerTest.java

+    }
+
+    /* combiner returns highest byte value */
+    public static byte combinerByte(byte v1, byte v2) {


I'm not sure I agree with that as it introduces another helper file. Tests are already marked with an annotation. What's the rationale for moving the helper methods?

DanHeidinga · 2019-02-12T21:09:31Z

test/functional/Jsr292/src/com/ibm/j9/jsr292/FilterArgumentsWithCombinerTest.java

+
+    /* return the lowest value char */
+    public static char targetChar(char v1, char v2, char v3) {
+       if ((v1 <= v2) & (v1 <= v3)) {


This can be written in fewer comparisons and ends up with simpler to read logic

public static char targetChar(char v1, char v2, char v3) { char lowest = v1; if (v2 < lowest) { lowest = v2; } if (v3 < lowest) { lowest = v3; } return lowest; }

I've found for tests like this, that sometimes it more clear what is happening if the test either returns a string that concats all the arguments into a list or it returns an array of the arguments. It helps with debugging the tests when there is a problem.

that definitely seems more straightforward. I will do something your suggestion.

test/functional/Jsr292/playlist.xml

theresa-m · 2019-02-13T21:49:19Z

thanks for the reviews @ChengJin01 and @DanHeidinga I've addressed your comments

DanHeidinga

lgtm. @llxia Can you or a delegate give this a once over to validate the way the tests are being added?

llxia · 2019-02-14T00:01:06Z

test/functional/Jsr292/playlist.xml

+		</groups>
+		<subsets>
+			<subset>12+</subset>
+		</subsets>


Could you please add impls? Thanks

<impls> <impl>openj9</impl> <impl>ibm</impl> </impls>

these changes should not be j9 specific.

Ideally try running these tests against a hotspot build, have you been able to verify that?
If so and they run fine, then you can leave out the <impls> block, and impl specifiers, so that these tests will get run against all implementations?

the NoOptions run passes with hotspot but the Mode195 test does not. I'll add the impls

So presumably a hotspot build chokes on the option -XX:RecreateClassfileOnload which Mode195 contains.

makes sense. its added now

In this case, should we break this test into two? one for NoOptions which is for all impls, and one for Mode195 which is openj9 and ibm specific?

I was not going to suggest it, because it has implications for all other targets (as test team already discussed, and have deferred decision), we may want to consider other options than nearly duplicate test targets for the multi-impl test approach.

but yes, it could broken it in 2.

Eventually, we will need to do a pass of all playlists, when we do that, we may not want to break all the test targets into 2 to achieve multi-impl testing. We may want a rule that says NoOptions gets run on all impls, or some back-end check for options that strips out modes that are not applicable.

llxia

LGTM

DanHeidinga · 2019-02-19T18:00:03Z

Jenkins test sanity xlinux jdk12

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>

theresa-m · 2019-02-21T16:40:32Z

rebased to include build fix

pshipton · 2019-02-24T12:37:31Z

@DanHeidinga can this be merged before the 0.13 code split?

pshipton · 2019-02-24T12:37:49Z

Jenkins test sanity plinux jdk12

pshipton · 2019-02-24T16:31:09Z

Jenkins test extended plinux jdk12

pshipton · 2019-02-24T19:58:57Z

The reviewers have approved and the testing is passing, except for expected sanity failures. Merging.

pshipton added comp:test jdk12 labels Feb 11, 2019

ChengJin01 suggested changes Feb 11, 2019

View reviewed changes

DanHeidinga reviewed Feb 12, 2019

View reviewed changes

theresa-m force-pushed the working_12_test_mh branch from ee687bb to 0099bd7 Compare February 13, 2019 21:47

DanHeidinga approved these changes Feb 13, 2019

View reviewed changes

DanHeidinga self-assigned this Feb 13, 2019

DanHeidinga requested a review from llxia February 13, 2019 22:20

llxia reviewed Feb 14, 2019

View reviewed changes

theresa-m force-pushed the working_12_test_mh branch 2 times, most recently from 9e37f65 to bfc25c9 Compare February 14, 2019 18:15

llxia approved these changes Feb 19, 2019

View reviewed changes

Test Java12 MethodHandle changes

77e1f32

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>

theresa-m force-pushed the working_12_test_mh branch from bfc25c9 to 77e1f32 Compare February 21, 2019 16:39

pshipton added this to the Release 0.13.0 (Java 12) milestone Feb 24, 2019

pshipton merged commit 72bf491 into eclipse-openj9:master Feb 24, 2019

theresa-m deleted the working_12_test_mh branch February 25, 2019 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test Java12 MethodHandle changes #4689

Test Java12 MethodHandle changes #4689

theresa-m commented Feb 11, 2019

ChengJin01 Feb 11, 2019

ChengJin01 Feb 11, 2019

DanHeidinga Feb 12, 2019

DanHeidinga Feb 12, 2019 •

edited

Loading

DanHeidinga Feb 12, 2019

DanHeidinga Feb 12, 2019

DanHeidinga Feb 12, 2019

DanHeidinga Feb 12, 2019

theresa-m Feb 13, 2019

theresa-m commented Feb 13, 2019

DanHeidinga left a comment

llxia Feb 14, 2019

theresa-m Feb 14, 2019

smlambert Feb 14, 2019

theresa-m Feb 14, 2019 •

edited

Loading

smlambert Feb 14, 2019

theresa-m Feb 14, 2019

llxia Feb 15, 2019

smlambert Feb 15, 2019

smlambert Feb 15, 2019

llxia left a comment

DanHeidinga commented Feb 19, 2019

theresa-m commented Feb 21, 2019

pshipton commented Feb 24, 2019

pshipton commented Feb 24, 2019

pshipton commented Feb 24, 2019

pshipton commented Feb 24, 2019

Test Java12 MethodHandle changes #4689

Test Java12 MethodHandle changes #4689

Conversation

theresa-m commented Feb 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theresa-m commented Feb 13, 2019

DanHeidinga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theresa-m Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llxia left a comment

Choose a reason for hiding this comment

DanHeidinga commented Feb 19, 2019

theresa-m commented Feb 21, 2019

pshipton commented Feb 24, 2019

pshipton commented Feb 24, 2019

pshipton commented Feb 24, 2019

pshipton commented Feb 24, 2019

DanHeidinga Feb 12, 2019 •

edited

Loading

theresa-m Feb 14, 2019 •

edited

Loading