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.

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

Setup for Java12andUp tests (jep334) #4104

Closed
wants to merge 1 commit into from

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Dec 18, 2018

see #4195 for dependencies

  • including playlist.xml entry for jep334 tests

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

@theresa-m
Copy link
Contributor Author

update copyright, merge conflict

@pshipton pshipton added this to the Release 0.13.0 (Java 12) milestone Jan 28, 2019
@pshipton pshipton added the jdk12 label Jan 28, 2019
@theresa-m theresa-m changed the title Setup for Java12andUp tests Setup for Java12andUp tests (jep334) Jan 28, 2019
@DanHeidinga
Copy link
Member

@llxia Can you or a delegate review this?

@theresa-m theresa-m force-pushed the 12_test_frame branch 4 times, most recently from 32e9477 to af14ebb Compare February 14, 2019 20:54
@theresa-m
Copy link
Contributor Author

updated to resolve conflicting files

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

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

If its expected the Jep334Tests target work for jdk13, and on, then a minor change to subset to use 12+, otherwise LGTM

<group>functional</group>
</groups>
<subsets>
<subset>12</subset>
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing these tests would be for 12+ subsets/versions.

As discussed in separate review, this can stay implementation-agnostic, until someone adds a variant/mode that contains a command line option that hotspot doesn't recognize. With NoOptions, its good as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it should be 12+. I will fix that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated with that change

- including playlist.xml entry for jep334 tests

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@smlambert
Copy link
Contributor

Launched Grinder: https://ci.eclipse.org/openj9/view/Test/job/Test-Grinder/272
to verify that testng doesn't complain if no test classes specified.

@theresa-m - have you tried running this test target locally ?

@theresa-m
Copy link
Contributor Author

theresa-m commented Feb 14, 2019

ooh just tried the test frame work does not like that its empty. I can lump this commit in with one of my prs that actually adds a test so there's no error. I made this separate because I wasn't sure which one would get merged first. sorry I didn't think that would be an issue

@theresa-m
Copy link
Contributor Author

Adding this commit to #4105 instead to avoid the compilation problem.

@theresa-m theresa-m closed this Feb 14, 2019
@smlambert
Copy link
Contributor

no worries, we could have additionally added a dummy class in this PR, but looks like you have a good approach to shift it anyway :) Thanks for strengthening the jdk12 testing story, very much appreciated!!

@theresa-m theresa-m deleted the 12_test_frame branch February 25, 2019 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants