Separate tests that rely on SecurityManager (Jsr292) #14580
Conversation
EricYangIBM
force-pushed
the
sm_Jsr292
branch
2 times, most recently
from
246ee5c to
7b9cb6c
Compare
test/functional/Jsr292/build.xml
Outdated
| <javac srcdir="${srcpath}" excludes="${commonExcludes}" destdir="${build}" debug="true" fork="true" executable="${compiler.javac}" includeAntRuntime="false" encoding="ISO-8859-1" classpathref="build.cp"> | ||
| <!-- exclude tests that depend on SecurityManager --> | ||
| <exclude name="com/ibm/j9/jsr292/MethodTypeTests_SM.java"/> | ||
| <exclude name="com/ibm/j9/jsr292/bootstrap/Test_CallerSensitive.java"/> |
There was a problem hiding this comment.
What error for this test? assuming it compiles even the security manager APIs are removed.
There was a problem hiding this comment.
Test_CallerSensitive depends on CallerSensitiveClass which extends SecurityManager
There was a problem hiding this comment.
It doesn't appear that SecurityManager is required by CallerSensitiveClass, can the dependency be removed?
There was a problem hiding this comment.
Tried removing it and seems like it is ok
test/functional/Jsr292/testng.xml
Outdated
| @@ -1,7 +1,7 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
|
|
|||
| <!-- | |||
| Copyright (c) 2016, 2020 IBM Corp. and others | |||
| Copyright (c) 2016, 2022 IBM Corp. and others | |||
There was a problem hiding this comment.
This file has only copyright year change which is no needed.
test/functional/Jsr292/testng_sm.xml
Outdated
| <class name="com.ibm.j9.jsr292.MethodTypeTests_SM"/> | ||
| </classes> | ||
| </test> | ||
| </suite> |
There was a problem hiding this comment.
A new line is required at the end of a file.
|
jenkins test extended xlinux jdk8,jdk11,jdk17,jdk18,jdknext |
test/functional/Jsr292/playlist.xml
Outdated
| @@ -89,8 +89,8 @@ | |||
| --add-opens=java.base/java.lang=ALL-UNNAMED \ | |||
| -Djava.security.policy=$(Q)$(TEST_RESROOT)$(D)java.policy$(Q) \ | |||
| -cp $(Q)$(TEST_RESROOT)$(D)jsr292test.jar$(P)$(RESOURCES_DIR)$(P)$(TESTNG)$(P)$(LIB_DIR)$(D)asm-all.jar$(Q) \ | |||
| org.testng.TestNG -d $(REPORTDIR) $(Q)$(TEST_RESROOT)$(D)testng.xml$(Q) \ | |||
| -testnames jsr292Test,jsr292Test_optional \ | |||
| org.testng.TestNG -d $(REPORTDIR) $(Q)$(TEST_RESROOT)$(D)testng_sm.xml$(Q) \ | |||
There was a problem hiding this comment.
We don't support 9, 10 any more so this test can just be deleted.
| <impl>openj9</impl> | ||
| <impl>ibm</impl> | ||
| </impls> | ||
| </test> |
There was a problem hiding this comment.
Instead of add -testnames jsr292Test_SM in different test targets , could we just have one jsr292Test_SM target? And add <version>8</version> and <variation>-Xjit:count=0</variation> in this jsr292Test_SM target? Did I miss anything?
There was a problem hiding this comment.
It seems like jsr292Test_jdk8 and jsr292Test_jdk8_special are the same (with variation changes) but jsr292Test_JitCount0_jdk8 has an extra testname jsr292Test_optional and platform requirement ^arch.arm. The first two could probably be merged but why are they separate to begin with? If I merge the new SM tests then the non SM tests should be merged as well.
There was a problem hiding this comment.
The first two could probably be merged but why are they separate to begin with?
I am not sure the details, but I think these tests were added in the early days. And the test framework (TKG) might have limited ability back then, which resulted in separated targets.
If I merge the new SM tests then the non SM tests should be merged as well.
I was thinking to keep this PR simple and only deal with SM tests. And we can have a separate PR for cleaning up non SM tests later. But it is up to you. I am ok either way.
There was a problem hiding this comment.
The github diff was messy when I tried to merge everything so I merged just the SM tests instead
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]>
|
@llxia ping |
|
jenkins test extended xlinux jdk8,jdk11,jdk17,jdk18,jdknext |
|
testSCCacheManagement_0 failure is a known issue #8949. Added comment #8949 (comment) |
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]