Ensure that sigtest -IgnoreJDKClass can ignore JDK classes (java.* and javax.transaction.xa.*) during signature check #30
Conversation
scottmarlow
force-pushed
the
IgnoreJDKClass_part2
branch
from
73b3a66
to
7e1cbdd
Compare
Signed-off-by: Scott Marlow <smarlow@redhat.com>
scottmarlow
force-pushed
the
IgnoreJDKClass_part2
branch
from
7e1cbdd
to
405644d
Compare
scottmarlow
changed the title
scottmarlow
changed the title
Signed-off-by: Scott Marlow <smarlow@redhat.com>
|
Will merge this soon so that we can start using the changes in EE 11 TCKs. |
| private static PackageGroup excludedJdkClasses = new PackageGroup(true); | ||
|
|
||
| public static void enable() { | ||
| excludedJdkClasses.addPackages(new String[] {"java", "javax.transaction.xa" }); |
There was a problem hiding this comment.
Should we also ignore things like javax.sql?
There was a problem hiding this comment.
Good point, I looked at the current https://github.com/jakartaee/platform-tck/blob/master/src/com/sun/ts/tests/signaturetest/signature-repository/jakarta.persistence.sig#L1840 file and do see that we should include javax.sql and others as well.
Perhaps we should exclude all of javax since all of the classes that we care about checking for (super-setting/sub-setting) changes should be in the jakarta package.
For reference, output of grep javax in signaturetest/signature-repository folder is attached to javax.txt
There was a problem hiding this comment.
^^ I think it would be appropriate to ignore all javax packages assuming that they are part of the JDK since this artifact is part of jakartaee tools. But maybe put a big warning somewhere in the README that we will ignore java and javax just in case :)
| if (JDKExclude.isJdkClass(baseAnnotList[requiredPos].getName()) || | ||
| JDKExclude.isJdkClass(testAnnotList[foundPos].getName())) { | ||
| // don't check excluded Annotation classes | ||
| return; |
There was a problem hiding this comment.
Should this be continue instead of return? Otherwise, you won't check the rest of the annotations.
There was a problem hiding this comment.
Nice catch! Thank you!
There was a problem hiding this comment.
Thanks again @KyleAure as this change led to a more correct checkAnnotations implementation as we made the suggested fix and also changed to validate JDK annotations by checking that the implementation uses the same JDK annotation class name.
… even if a JDK annotation is checked. checkAnnotations() should also validate JDK annotations by only comparing the JDK annotation class name. Signed-off-by: Scott Marlow <smarlow@redhat.com>
Work in progress for #20. This change eliminates the need for passing the often changing list JDK classes to exclude. Instead just set -IgnoreJDKClass