-
Notifications
You must be signed in to change notification settings - Fork 109
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
Jakarta EE 10 standalone TCK support for JDK 21 runs. #1219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Pasting one of the Tags failures for easier viewing:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the failing tags tests should be challenged for both this pull request and https://issues.redhat.com/browse/WFLY-18900 as well (see https://issues.redhat.com/secure/attachment/13112138/jstlfailures.txt).
Tags tests are failing with JDK21 and Glassfish 8 [ https://ci.eclipse.org/jakartaee-tck/job/guru/job/jakartaee-tck-build-run/job/jdk21-run/39/testReport/], similar failures having |
In case it helps, the problem is the breaking space before 'AM/PM'. Apparently the CLDR used in SE 21 wants a non-breaking space there. I have no idea what to do with this information. :)
Outputs:
The answer on https://stackoverflow.com/questions/77225936/java-21-problem-with-dateformat-getdatetimeinstance-formatnew-date gives some context into what likely lead to this string no longer being ok in SE 21. |
@scottmarlow: Glassfish 8 is under development, Please raise challenge for https://issues.redhat.com/browse/WFLY-18900 failures. |
@gurunrao This might need a TCK challenge, not sure yet. I would like to see what happens if we update https://github.com/jakartaee/platform-tck/blob/10.0.x/src/web/jstl/compat/onedotzero/positiveFormatLocalizationContextI18NTest.jsp#L32 to use value="Nov 21, 2000, 3:45:02\u202FAM"/> which I did locally but I'm not sure of the magic secret to rebuild the jsp prior to running the jstl/compat folder. I tried:
Assume that testFolder=jstl/compat/onedotzero But it seems that even after running the above positiveFormatLocalizationContextI18NTest.jsp didn't get rebuilt. Do you remember which ant command we should use to rebuild the https://github.com/jakartaee/platform-tck/blob/10.0.x/src/web/jstl/compat/onedotzero/positiveFormatLocalizationContextI18NTest.jsp + the dist/** archives for onedotzero? |
I might just start a 10.0.x test branch with changes to the failing jsp sources to use \u202F instead of regular space before AM/PM and build a test platform tck. If someone has the right |
Hello,
I recall working on this space problem in Faces in our Open Liberty tests
(see OpenLiberty/open-liberty#24009).
In the issue I wrote:
Java 20 Updated to CLDR 42
<https://cldr.unicode.org/index/downloads/cldr-42> which includes the
following change:
- Large-scale normalization of different kinds of spaces (see Migration)
- https://unicode-org.atlassian.net/browse/CLDR-14032
…____
My solution is not very elegant -- I checked for a different character
based on which Java level was used:
"\u202F" for Java 20 and above and "\u0020" for anything below.
Something similar may be needed in the TCK unless there's another way?
Hope it helps,
Volodymyr
On Fri, Mar 8, 2024 at 9:43 AM Scott Marlow ***@***.***> wrote:
Seems like the failing tags tests should be challenged for both this pull
request and https://issues.redhat.com/browse/WFLY-18900 as well (see
https://issues.redhat.com/secure/attachment/13112138/jstlfailures.txt).
@scottmarlow <https://github.com/scottmarlow>: Glassfish 8 is under
development, Please raise challenge for
https://issues.redhat.com/browse/WFLY-18900 failures.
@gurunrao <https://github.com/gurunrao> This might need a TCK challenge,
not sure yet.
I would like to see what happens if we update
https://github.com/jakartaee/platform-tck/blob/10.0.x/src/web/jstl/compat/onedotzero/positiveFormatLocalizationContextI18NTest.jsp#L32
to use value="Nov 21, 2000, 3:45:02\u202FAM"/> which I did locally but I'm
not sure of the magic secret to rebuild the jsp prior to running the
jstl/compat folder. I tried:
if [[ "$testFolder" == testFolder=jstl* ]]; then
cd $TS_HOME/src/com/sun/ts/tests/$testFolder
ant compile package build.all compile
cd $TS_HOME/src/web ant compile build package
fi
Assume that testFolder=jstl/compat/onedotzero
But it seems that even after running the above
positiveFormatLocalizationContextI18NTest.jsp didn't get rebuilt. Do you
remember which ant command we should use to rebuild the
https://github.com/jakartaee/platform-tck/blob/10.0.x/src/web/jstl/compat/onedotzero/positiveFormatLocalizationContextI18NTest.jsp
+ the dist/** archives for onedotzero?
—
Reply to this email directly, view it on GitHub
<#1219 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNIZZRBICZZPMSKDLRCH43YXHE77AVCNFSM6AAAAABCSJ76NCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBVHAYTKNRWGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Pasting from ^ issue:
Does Java 11 treat the narrow no break space (U+00A0) the same as (U+0020) so that the test could be changed to use (U+00A0)? |
From my local testing, \u202F does not work with Java 11 that same as a (U+0020). I think the EE 10 tests with this problem should be challenged and possibly excluded for EE 10. |
I think the tests are problematic for Jakarta EE 11 as well due to the Java changes. Clearly applications will need to be updated that hit this problem on Java 19+. |
👍 , I'll create a TCK challenge for Tags 3.0 |
@gurunrao jakartaee/tags#255 is the TCK challenge issue. |
@scottmarlow: Based on challenge jakartaee/tags#255, will add following tests to JSTL exclude list: com/sun/ts/tests/jstl/compat/onedotzero/JSTLClient.java#positiveFormatLocalizationContextI18NTest For challenge jakartaee/tags#256 following tests have been excluded: com/sun/ts/tests/jstl/spec/core/urlresource/importtag/JSTLClient.java#negativeImportRequestDispatcherServletExceptionTest |
I believe this is OK as of TCK 4.0.1 published quite recently
Can this one be completed, please? XML WS depends on this... |
jakartaee/tags#255 challenge will be accepted today as per comment - jakartaee/tags#255 (comment), I will be able to proceed with test. |
@scottmarlow - Please review. |
install/jstl/bin/ts.jtx
Outdated
@@ -16,4 +16,46 @@ | |||
|
|||
# | |||
# $Id$ | |||
# | |||
# https://github.com/jakartaee/tags/issues/255 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated:
jakartaee/tags#255 (comment) discusses a possible code change to address the issue for EE 10+. I'm not yet sure if the suggested code change to change the test based on which Java version is used, will help but it sounds like a worthwhile idea to try.
The only issue that I see is that the Platform TCK team doesn't have enough time to do everything that we are being asked to do. We should respond on the jakartaee/tags#255 issue that we do not have time to make a test change but see if someone else wants to. Otherwise we will merge this change and the other EE 10 changes that are needed for Java 21 support.
Forget what I said about EE 11 as this pr is for the 10.0.x branch. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated: jakartaee/tags#255 (comment) discusses a possible code change to address the issue for EE 10+. I'm not yet sure if the suggested code change to change the test based on which Java version is used, will help but it sounds like a worthwhile idea to try.
The only issue that I see is that the Platform TCK team doesn't have enough time to do everything that we are being asked to do. We should respond on the jakartaee/tags#255 issue that we do not have time to make a test change but see if someone else wants to. Otherwise we will merge this change and the other EE 10 changes that are needed for Java 21 support.
Forget what I said about EE 11 as this pr is for the 10.0.x branch. :-)
Thanks Scott for the comments, As you rightly said the changes are applicable for complete release of Specification. The PR is for service release, where test changes are not possible. Please let me know, if further changes are need in the PR or PR can be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting from https://jakarta.ee/committees/specification/tckprocess:
A consensus that a test produces invalid results will result in the exclusion of that test from certification requirements, and an immediate update and release of an official distribution of the TCK including the new exclude list. The associated challenge issue MUST be closed with an accepted label to indicate it has been resolved.
The specification project may approve (user) workarounds for an accepted TCK challenge (as an alternative to excluding TCK tests).
As another alternative to excluding a challenged test, it may be possible to adjust the test validation logic to “expand” the validation check. E.g. if a test expects a value “A1” for a specific variable “x”, but a challenge is raised arguing that the specification language actually allows for either values “A1” and “A2” (but no other values) to be valid values for “x”, then it could be a valid course of action to modify the challenged test to allow either “A1” OR “A2” for “x”.
Since this line of thinking might be applied to cases that aren’t quite as straightforward as this example, care should be taken when using this approach. A particular danger is that an implementation that has already demonstrated compliance before the challenge was raised might actually not pass the new, modified test.
To limit the confusion and additional work such a scenario would cause, if there is already at least one certified compatible implementation before the challenge, the new, modified TCK should be run against at least one such implementation (and ideally all of them) before the changes are published, released, and finalized.
If such a change were released, and it was later found to cause a previously-certified implementation to fail the new, modified test, then excluding the test would likely be the only option, and this would require yet another, additional service release.
More specifically, the text it may be possible to adjust the test validation logic to “expand” the validation check
mentions a specific use case that doesn't involve new releases of Java breaking compatibility but that is what broke the jstl tests. To "expand" the (TCK) validation check we have to support both Java 11/17 (as before) but also "expand" the check to also work on Java 21. To do that, I think there will be some separate files for the different Java versions and some test changes to use different expressions in the jstl tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gurunrao feel free to respond on the related EE 10 TCK challenge issue jakartaee/tags#255 (comment)
@scottmarlow would it make sense to merge this as is and resolve pending issue/tags tck through a different PR to unblock 4-5 different TCKs unrelated to the issue being observed? |
I think that we are prepared to merge the Tags change as both jakartaee/tags#255 + jakartaee/tags#256 are accepted. We probably should separate the changes for the other specs unless they already have TCK challenges (I don't see a challenge issue under https://github.com/jakartaee/connectors/issues)?
|
Actually, this pull request only is changing how we run the TCKs for the other specs which do not require a TCK challenge, so yes, we can merge this and I will after reading through the change once more.
|
@@ -22,7 +22,7 @@ | |||
<import file="../../bin/xml/ts.common.props.xml"/> | |||
|
|||
<property name="deliverable.version" value="2.1"/> | |||
<property name="deliverable.tck.version" value="2.1.0"/> | |||
<property name="deliverable.tck.version" value="2.1.1"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, guess we are looking to release new service releases with the GlassFish TCK script changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to release new TCK service releases we do need to follow the TCK process so we might want to separate the JSTL change since that is ready to merge but it would be nice to only have to release the platform tck once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the TCK process:
Process for releasing a point revision of a TCK
The process for releasing a point revision of a TCK entails filing an issue in the jakartaee/specifications repository with the following details:
- Link to the TCK release to be published.
- Updated TCK links in the specification’s _index.md file.
- A high-level description of what tests were excluded from the TCK and why.
@lukasj as (just) mentioned above we need accepted TCK challenges for the other issues as well. @gurunrao I think we need to increment the platform tck version for the jstl test excludes. |
Actually, we only need to release the platform TCK with the jstl changes. I'll fork this change and create a separate pr for just the jstl changes. |
Thanks for the review @scottmarlow : The javatest jar has been built from master branch using github repository https://github.com/openjdk/jtharness.git. The javatest jar was built for PR #1163 and same javatest.jar has been used for current PR. javatest library built has option to bypass security manager and complete the test run. |
Signed-off-by: Gurunandan Rao <gurunandan.rao@oracle.com>
Signed-off-by: Gurunandan Rao <gurunandan.rao@oracle.com>
…ersion to support JDK21[without security manager] Signed-off-by: Gurunandan Rao <gurunandan.rao@oracle.com>
Signed-off-by: Gurunandan Rao <gurunandan.rao@oracle.com>
Signed-off-by: Gurunandan Rao <gurunandan.rao@oracle.com>
|
We should do this soon! |
7f74e4c
to
4e4dba8
Compare
@scottmarlow @edburns - I have removed the JSTL changes from the PR. |
release/tools/jta.xml
Outdated
@@ -22,7 +22,7 @@ | |||
<import file="../../bin/xml/ts.common.props.xml"/> | |||
|
|||
<property name="deliverable.version" value="2.0"/> | |||
<property name="deliverable.tck.version" value="2.0.2"/> | |||
<property name="deliverable.tck.version" value="2.0.3"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this before but looks like https://www.eclipse.org/downloads/download.php?file=/jakartaee/transactions/2.0/jakarta-transactions-tck-2.0.1.zip was the last Transactions TCK to be promoted. So I think we can just stick with the release/tools/jta.xml deliverable.tck.version = 2.0.2 value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything else looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reverted the version increment for JTA, also
found JTA 2.0.2 TCK bundle service release at - https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/promoted/eftl/
Signed-off-by: Gurunandan Rao <gurunandan.rao@oracle.com>
Thanks @gurunrao! |
Support JDK21 runs with security manager disabled for SPEC which do not have new release during Jakarta EE 11.
Service release TCK's are:
Tags has 40 failures[date/timezone problem]- https://ci.eclipse.org/jakartaee-tck/job/guru/job/jakartaee-tck-build-run/job/jdk21-run/39/#showFailuresLink