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

Separate tests that rely on SecurityManager (JLM_Tests) #14562

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

EricYangIBM
Copy link
Contributor

SecurityManager will be removed in a future jdk. Separate tests that
rely on it so that they can be easily disabled in the future.

Issue: #14412
Signed-off-by: Eric Yang [email protected]

@EricYangIBM
Copy link
Contributor Author

TestSharedClassMemoryMXBean.java and TestRegression.java are disabled (commented out) in testng and don't look like they will be fixed so I disabled their compilation for jdk19+

<compilerarg line='${addExports} ${TestUtilitiesExports}' />
<!-- exclude tests that depend on SecurityManager -->
<exclude name="org/openj9/test/java/lang/management/TestSharedClassMemoryMXBean.java" />
<exclude name="org/openj9/test/java/lang/management/TestRegression.java" />
Copy link
Member

Choose a reason for hiding this comment

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

These two tests are still valuable if the security manager related APIs are separated, pls create two new classes w/o APIs to be removed.

bean.setMaxHeapSize(size + 1024);
if (size + 1024 != bean.getMaxHeapSize()) {
throw new RuntimeException("not equal");
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this a duplicated sub-test somewhere, otherwise, pls keep original version, and create one w/o System.setSecurityManager(new SecurityManager()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this class is used at all

@JasonFengJ9
Copy link
Member

A separated testng_sm.xml might be required for this PR as well.

@EricYangIBM
Copy link
Contributor Author

I don't think a new testng file is needed here since I didn't add any java test files to the existing testng. Jdk19 will not fail to find the new compilation-ignored _SM.java files because they are not listed in playlist and testng

@JasonFengJ9 JasonFengJ9 requested review from llxia and pshipton March 3, 2022 20:25
@pshipton
Copy link
Member

pshipton commented Mar 3, 2022

jenkins test sanity alinux64 jdk18,jdknext

@pshipton
Copy link
Member

pshipton commented Mar 4, 2022

Pls add the _SM files into the testng.xml comment for the excluded tests, so if anyone fixes the tests they will realize these files exist.

SecurityManager will be removed in a future jdk. Separate tests that
rely on it so that they can be easily disabled in the future.

Issue: eclipse-openj9#14412
Signed-off-by: Eric Yang <[email protected]>
Copy link
Member

@JasonFengJ9 JasonFengJ9 left a comment

Choose a reason for hiding this comment

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

LGTM

@JasonFengJ9
Copy link
Member

@llxia ping

@llxia
Copy link
Contributor

llxia commented Mar 30, 2022

sorry, this slipped under my radar

@llxia
Copy link
Contributor

llxia commented Mar 30, 2022

jenkins test sanity alinux64 jdk18,jdknext

@llxia llxia merged commit 5f38d87 into eclipse-openj9:master Mar 30, 2022
@EricYangIBM EricYangIBM deleted the sm_JLM_Tests branch March 30, 2022 19:40
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.

4 participants