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

Tests added for the option -XX:[+/-]ShareAnonymousClasses and -XX:[+/-]ShareUnsafeClasses #7278

Merged
merged 1 commit into from
Oct 8, 2019
Merged

Tests added for the option -XX:[+/-]ShareAnonymousClasses and -XX:[+/-]ShareUnsafeClasses #7278

merged 1 commit into from
Oct 8, 2019

Conversation

doomerxe
Copy link
Contributor

@doomerxe doomerxe commented Sep 30, 2019 •
edited
Loading

Added the tests for options

-XX:+ShareAnonymousClasses
-XX:-ShareAnonymousClasses
-XX:+ShareUnsafeClasses
-XX:-ShareUnsafeClasses

for Java 11 and up

Signed-off-by: Jiahan Xi doomerXe@gmail.com

@doomerxe
Copy link
Contributor Author

Hi @hangshao0! Could you please review this change? Thank you so much!

@doomerxe doomerxe marked this pull request as ready for review September 30, 2019 14:59
@hangshao0
Copy link
Contributor

You may want to update this line as well:

https://github.com/eclipse/openj9/blob/master/test/functional/cmdLineTests/shareClassTests/SCCMLTests/ShareClassesCMLTests-1.xml#L27

@doomerxe
Copy link
Contributor Author

@hangshao0 I have made the changes, please re-review, thank you!

@pshipton
Copy link
Member

pshipton commented Sep 30, 2019 •
edited
Loading

Tests are fine, but only check basic functionality. Some other things to check:

  • the class is loaded from the shared cache, and works as expected
  • a modified implementation of the same anonymous and unsafe class works properly with the cache. i.e. the correct implementation is loaded and works as expected

@doomerxe doomerxe force-pushed the optiontest branch 4 times, most recently from 9694d79 to e02a814 Compare October 1, 2019 19:25
@doomerxe
Copy link
Contributor Author

doomerxe commented Oct 1, 2019

Hi @pshipton, I have added some new tests, could you please review them, thank you!

@doomerxe doomerxe force-pushed the optiontest branch 2 times, most recently from b4cd343 to 7faa65d Compare October 2, 2019 21:15
@doomerxe
Copy link
Contributor Author

doomerxe commented Oct 2, 2019

Hi @pshipton, I have updated the test, please review them, thanks!

@doomerxe doomerxe force-pushed the optiontest branch 3 times, most recently from 23d9818 to 5d913b0 Compare October 4, 2019 21:31
@doomerxe
Copy link
Contributor Author

doomerxe commented Oct 5, 2019

Hi @pshipton , I have changed the files and tested the changes. Could you please review? Thank you so much!

@pshipton
Copy link
Member

pshipton commented Oct 7, 2019

jenkins test sanity plinux jdk8,jdk11

@pshipton
Copy link
Member

pshipton commented Oct 7, 2019

@doomerxe please see the jdk8 test build failure

@doomerxe
Copy link
Contributor Author

doomerxe commented Oct 7, 2019

@doomerxe please see the jdk8 test build failure

The tests I made are for jdk 11 and up. I forgot to check the jdk version on the build.xml. I will fix it!

@doomerxe doomerxe force-pushed the optiontest branch 2 times, most recently from c2d51c1 to f49fd52 Compare October 8, 2019 15:32
@doomerxe
Copy link
Contributor Author

doomerxe commented Oct 8, 2019

Hi @pshipton , I have fixed the issue and tested it on jdk8. It seems to be fine now. Could you please re-run the test? Thank you!

@pshipton pshipton requested a review from llxia October 8, 2019 15:43
@pshipton
Copy link
Member

pshipton commented Oct 8, 2019

jenkins test sanity plinux jdk8,jdk11

@pshipton
Copy link
Member

pshipton commented Oct 8, 2019

The jdk11 testing passed, the failure is a jenkins uploading to artifactory issue.

@pshipton
Copy link
Member

pshipton commented Oct 8, 2019

@llxia can you please take a look at the test framework changes.

llxia
llxia approved these changes Oct 8, 2019
View reviewed changes
Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

LGTM

@pshipton pshipton self-assigned this Oct 8, 2019
@pshipton
Copy link
Member

pshipton commented Oct 8, 2019

@doomerxe one more tweak. Please update the commit comment to include the details you've added in the description of this PR.

@doomerxe
Tests added for the -XX:[+/-]ShareAnonymousClasses and -XX:[+/-]Share…
1489b4c 
…UnsafeClasses

Added the tests for options:

-XX:+ShareAnonymousClasses
-XX:-ShareAnonymousClasses
-XX:+ShareUnsafeClasses
-XX:-ShareUnsafeClasses

for Java 11 and up

Signed-off-by: Jiahan Xi <doomerXe@gmail.com>
@doomerxe
Copy link
Contributor Author

doomerxe commented Oct 8, 2019

@doomerxe one more tweak. Please update the commit comment to include the details you've added in the description of this PR.

Hi @pshipton , I have updated the comment, please take a look, thank you!

@pshipton
Copy link
Member

pshipton commented Oct 8, 2019

PR testing passed.

@pshipton pshipton merged commit 207a9c0 into eclipse-openj9:master Oct 8, 2019
@pshipton
Copy link
Member

pshipton commented Oct 8, 2019

Forgot to approve the review officially, but I do.

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

@pshipton pshipton pshipton left review comments

@hangshao0 hangshao0 hangshao0 approved these changes

@llxia llxia llxia approved these changes

Assignees

@pshipton pshipton

Labels
comp:test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@doomerxe @hangshao0 @pshipton @llxia