Remove Jdk9SigTestDriver #623
Conversation
brideck
force-pushed
the
RemoveJdk9SigTestDriver
branch
from
e8deaba
to
e6a4843
Compare
|
Should we set a default option for jimage.dir so even if missed to set it, there wont be test failures. |
Yes, I should have pointed that out in the TO DOs, too. I didn't know the default env enough to presume what a sensible value would be. |
|
I would humbly suggest something like |
Hi Brian, yes the above does make sense. Other possible options I feel are ${java.home}/extract/classes , ${ts.home}/tmp/jdk-bundles/ |
${JAVA_HOME}/extract/classes/java.base${pathsep}${JAVA_HOME}/extract/classes/java.naming${pathsep}${JAVA_HOME}/extract/classes/java.rmi${pathsep}${JAVA_HOME}/extract/classes/java.sql The above added to sigTestClasspath should resolve it for platform TCK . I think the same can be used for standalone TCKs too to resolve most of the required packages. |
|
Okay, I think I'm going to use the Updates to the PR on their way. |
|
I've updated all of the ts.jte.jdk11 files with new values. What do we need to do to merge and/or test out these changes? |
I ran some of the tests using this change in the CI Looks like we get below error , maybe we need to create the directory/ set permissions before ? [javatest.batch] 03-02-2021 21:03:45: jimage extract --dir=/home/jenkins/agent/workspace/_tck-alw_RemoveJdk9SigTestDriver/annotations-tck/bin/xml/../../tmp/jdk-bundles /opt/jdk-11.0.7/lib/modules You can also test caj tck in local with this change.
|
|
Hrm. I do create the jimage.dir ahead of time, so I don't think that's it. It's not actually executing jimage at all, otherwise I think the output would look different. I think it's complaining that it doesn't know what/where jimage is. Perhaps I need to pass the full path to jimage as the command? Curiously, I did not need that when testing locally in my OL environment. |
|
@alwin-joseph |
brideck
force-pushed
the
RemoveJdk9SigTestDriver
branch
from
d498f17
to
d43bfd0
Compare
Early results of annotations show pass https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck-alw/job/RemoveJdk9SigTestDriver/9/. |
Excellent, thanks. Those are looking pretty darn good, if I don't say so myself. |
|
Some of these failures imply that these weren't tested for EE 9, right? I mean, jakarta.authentication-api.jar being missing for jaspic seems like a pretty fundamental problem unrelated to the changes in this PR. But sure, let's try to fix them all. Changes forthcoming... |
For 9.0 we used ts.jte which has the right CP. We probably copied the ts.jte.jdk11 from ts.jte before adding the jar. |
- Update to use jimage for JDK 11+ runs - Create new jimage.dir property for use in sig tests Signed-off-by: Brian Decker <bmdecker@us.ibm.com>
Signed-off-by: Brian Decker <bmdecker@us.ibm.com>
brideck
force-pushed
the
RemoveJdk9SigTestDriver
branch
from
d43bfd0
to
92d5ebd
Compare
Ah, of course. It's apparently too early in the morning to think of such things. I've fixed jaspic, saaj, jsp, & jstl, and I've rebased to try to fix jpa, if you want to retry those. Not sure what's going on with jta. It looks like the test applications aren't getting deployed. |
jsp : I think jakarta.el.jar is missing in the CP saaj : looks like I was wrong, webservices-api-osgi.jar is already there in the CP. I suspect java.xml module is needed. |
Signed-off-by: Brian Decker <bmdecker@us.ibm.com>
|
What is hopefully the last set of fixes has been pushed. Fixes for jsp & saaj, as well as updates to ts.jte for jsp/jstl since we are no longer writing out a new value for sigTestClasspath. |
| // on the testcase's classpath. | ||
| Properties sysProps = System.getProperties(); | ||
| String version = (String) sysProps.get("java.version"); | ||
| if (version.startsWith("11")) { |
There was a problem hiding this comment.
Would it be better to just do this for Java 9 and above to future proof this and not require a future change for later JDKs.
if (!version.startsWith("1.")) {
There was a problem hiding this comment.
Until we solve #156 (stop checking JDK signatures) we are dependent on specific Java SE versions. Although whether all future JDKs will work with this code change, I'm not sure.
My current thinking is that we will need to switch to checking if the Java version is equal/greater than the current base Java SE version (after we solve jakartaee-tck/issues/156). If that makes sense, we will be saying that the TCK can work with any later JDK version as long as the later JDK version doesn't break compatibility with what the TCK depends on. In summary, I think that is the best possible and might mean that a particular TCK version only really works with the base Java SE version + the next N JDK versions until some (later JDK) breaking change causes breakage. So, that later JDK version will not work with the earlier TCK until a new TCK (for a new Jakarta EE release) is released that works with the new JDK version. This is just my current thinking and the community, you, others need to buy into it when we actually look to resolve jakartaee-tck/issues/156. Sorry for the interruption and tangent on this pr. :-)
There was a problem hiding this comment.
I've gone ahead and made Jared's proposed changes. Even if we move forward with changes related to 156, making this change would remove the need for us to update the driver code for this particular scenario again.
| // on the testcase's classpath. | ||
| Properties sysProps = System.getProperties(); | ||
| String version = (String) sysProps.get("java.version"); | ||
| if (version.startsWith("11")) { |
There was a problem hiding this comment.
Same comment as the previous one to do ! startsWith "1."
There was a problem hiding this comment.
There are failures with JDK8 + Glassfish 6 + (PR changes) combination. jimage.dir property not set in ts.jte error.
Details can be found here -
https://ci.eclipse.org/jakartaee-tck/job/guru/job/jakartaee-tck-gtest/job/RemoveJdk9SigTestDriver/2/
Good catch. I think a default value should be set for jimage.dir in all ts.jte files also or via java source code. |
Interesting. I'm not sure exactly what drives the property checking exception being reported here. Would it also just work to move |
It does look like it's the call to getProperty() in SigTestData that is triggering it, per the code here: https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/lib/deliverable/AbstractPropertyManager.java#L211
This doesn't seem to be 100% correct, and I can't quite figure out the code flow at play here. I'll just try a simpler more specific change for the first pass fix attempt. |
Signed-off-by: Brian Decker <bmdecker@us.ibm.com>
|
I've pushed a fix that works with Java 8 for me locally, if you could verify through another attempt that would be great. |
@brideck - thanks for helping us with fixes, I have triggered new build here - https://ci.eclipse.org/jakartaee-tck/job/guru/job/jakartaee-tck-gtest/job/RemoveJdk9SigTestDriver/3/ |
|
Oof. That apparently did not actually work. Digging some more in an attempt to avoid editing dozens of ts.jte files to add an unused property. |
Signed-off-by: Brian Decker <bmdecker@us.ibm.com>
|
Once more, please. I tested the previous attempt incorrectly, but this one really does seem to fix it locally. |
|
@brideck have restarted the build - https://ci.eclipse.org/jakartaee-tck/job/guru/job/jakartaee-tck-gtest/job/RemoveJdk9SigTestDriver/4/ |
Tests are passing now with JDK8 + GF 6 + PR. |
Excellent. If there are no more objections at this point, I think this PR could safely be merged by someone. |
|
Thanks @brideck for making this change! I'll soon start a build via https://ci.eclipse.org/jakartaee-tck/job/9.1/job/eftl-jakartaeetck-build-910/ and then later start a test run via https://ci.eclipse.org/jakartaee-tck/job/9.1/job/eftl-jakartaeetck-run-910/ |
The Jdk9SigTestDriver class has a lot of unimplemented function to the point where it is broken for many use cases when testing with Java 11 (see #618 for details). This PR will remove that class and amend everywhere that uses it.
In its place, I'm adding calls to jimage to extract the modules needed for the sig tests and updating sigTestClasspath to match.
TO DO:
CC @alwin-joseph @anajosep @arjantijms @cesarhernandezgt @dblevins @m0mus @edbratt @gurunrao @jansupol @jgallimore @kazumura @kwsutter @LanceAndersen @bhatpmk @RohitKumarJain @gthoman