-
Notifications
You must be signed in to change notification settings - Fork 28
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
Tags 3.0 TCK Challenge against tests that include space character before AM/PM #255
Comments
@scottmarlow why the additional set of test failures in Glassfish? Did those tests pass successfully on Wildfly? |
Is it possible to get the server.log file? |
The server.log is not currently archived. The generated JSTL tck contains a copy of the https://github.com/jakartaee/platform-tck/blob/master/docker/jstltck.sh script. I'm not sure if we would need to modification of that or the Jenkins job running the TCK.
When GlassFish is running against the JSTL standalone TCK, we see the following test stats: With WildFly running the JSTL tests in EE 10 Platform TCK, we see the following test stats: The Standalone JSTL TCK test count includes one more test as that is the signature test: |
Hey @scottmarlow , my question was more focused on why are the above 11 tests failing in GlassFish but not WildFly. Looking at the output:
The Since these 11 tests passed on WildFly but failed on GlassFish, I believe these failures are potentially not Jakarta Tags / TCK issues. If we can see the stack of the exception would give additional context but I didn't see To me, it looks like the two sets of tests referenced here are two different problems. |
Interesting find, the source is in a different folder https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/jstl/common/tags/ExceptionCheckTag.java#L174 |
@scottmarlow Given this information these 11 tests would only fail where Pages 4.0 / Jakarta EE11 were being tested. Do we need another challenge for this issue or could the TCK just be cleaned up to not call methods removed in Jakarta EE11? It would be good to keep these problems separate IMO. |
Good question, I don't think these 11 tests need a TCK challenge created for them. I think instead we could clean up https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/jstl/common/tags/ExceptionCheckTag.java#L174 to call JspException.getCause() which I agree should be captured by a separate Tags issues. |
I'm going to create the Tags issue for reporting the problem with https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/jstl/common/tags/ExceptionCheckTag.java#L174 I'll also remove the following 11 tests for GlassFish that I had included in this TCK challenge description:
|
@scottmarlow why is the remaining issue with the space character before AM/PM a challenge but #256 is not? These both seem to stem from running the Tags 3.0 TCK on a Java version / Features for Jakarta EE11. Could the TCK be updated for these tests to check the java version and then act accordingly? |
Great question to raise, thanks!
issues/256 is caused by Java SE 21 breaking backward compatibility via the JDK-8304925 change. #256 is caused by the planned removal of JspException.getRootCause() which the Tags 3.0 TCK depends on. In one case, the TCK failure cause is a breaking change in an EE Specification and in the other, the TCK Failure cause is a breaking change in Java SE 21. So which of these issues should be TCK challenges? Pasting from the TCK Process guidance on
It seems to me that the last point (
Yes, Flavia suggested something like that on Zulip a few days ago. I'm not really sure though about the idea of converting the string based on the Java version (quoting that suggestion from the WildFly Zulip discussion) but I don't really know enough what is the best approach. So what would be the best way to act accordingly? Have different copies of certain JSP files that work on Java 21 and conditionally use them when running on the Java 21? Or some way to convert the date values in the JSP source before the JSP files are processed. To help answer ^, I'm pasting from the Tags 3.0 spec:
Can Tags 3.0 implementations be required to automatically convert the date values in the JSP source when running on Java 21 (perhaps as per the above pasted Tags 3.0 Spec text)? Or do we need a new Tags Spec release to add an additional requirement? I ask as I think currently we need to do something for EE 11 but at the same time, I also want EE 10 implementations to also be able to pass the Jakarta EE 10 Platform TCK which runs against the same Tags tests. Also for EE 11, we have a lot of work ahead to refactor the Platform TCK which will include running the Tags TCK tests via Maven + Arquillian, whatever we do to address the challenge for EE 10, will hopefully help with running the EE 11 Platform TCK Tags tests on Java 21 as well as Java 17. |
I think we should accept this challenge. The issue may not be with JSTL itself / EE11 Specs, but I think we still have to accommodate any changes made in the underlying technology (i.e Java). @pnicolucci What do you think? |
I believe the best path forward here is to update the TCK to leverage Java to generate the Date/Time String that is Currently, the TCK has a static String: If the TCK were to use Java to generate the Date/Time values to be tested, then the values would be parsable by Java as they were generated by Java. The TCK could be updated and then the same TCK would work regardless of the JDK version being tested (11/17/21). When running on Java 21 a NNBS would be output, when running on Java 17 a standard space would be output. Take this code for example:
The As for the golden files a different file could be used depending on the version of Java being used so that the correct space character could be checked. |
Posted to the mailing list the latest status: https://www.eclipse.org/lists/jstl-dev/msg00138.html I plan to accept this challenge at the end of the week if there are no objections. @scottmarlow does the above solution seem reasonable to you? |
Well, I collided with this today when I executed tags TCK for any GF 7.0.x version. Finally I have found that by mistake I executed these tests with JDK21 - that breaks 30 tests of 542. Part of the issue is that TCK is sensitive to the default locale, but that can be resolved by With JDK11 and JDK17 GF7 passed all tests of JEE10 TCK. I know this issue is rather for JEE11, but I would like to have a firm ground first and have everything repeatable and solid in GF7 too. That is why I created eclipse-ee4j/glassfish#24925 - meanwhile I am preparing builds with TCK (private on OmniFish infrastructure as we have more powerful and reliable hw) and documenting the state of GF8 and JEE11 (here, I started by copy pasting JEE 10 and I am slowly going through the list). In GF master branch you can reproduce it by running this:
For GF8 I have to build the TCK first, but the tckrefactor branch build fails because of missing snapshot dependencies. But this problem is the same. Would be a solution just not depend on default system locale? |
@dmatej If you set LC_ALL=C it passes on SE 21? I think that's what you're saying, but it's not clear to me. |
No, it doesn't, but it fixes some issues when I have cs_CZ.UTF-8 set on my system. I believe the tck should ensure it. I did not find any way to pass on JDK21. |
@pnicolucci it seems reasonable to me but I'd like to hear from others in the community in the next day or so if anyone else is for/against the stated solution. I think that we will need a pull request for the https://github.com/jakartaee/platform-tck/tree/10.0.x branch so that we can apply the stated solution. If anyone later challenges the solution as not being valid and that later challenge is accepted, the tests would be excluded instead. |
@pnicolucci a day has passed and no feedback yet. Regarding the above solution that you mentioned, do you want to include that for a future EE release? Or for Jakarta EE 10? My preference is to exclude the tests as I know that could happen this week. |
We will merge the jakartaee/platform-tck#1219 change as soon as possible that will exclude the failing tests. |
Signed-off-by: Gurunandan Rao <gurunandan.rao@oracle.com>
… JSTL challenges that excludes tests for jakartaee/tags#256 + jakartaee/tags#255 Signed-off-by: Scott Marlow <smarlow@redhat.com>
Signed-off-by: Gurunandan Rao <gurunandan.rao@oracle.com>
…that excludes tests for jakartaee/tags#256 + jakartaee/tags#255 and also release JSTL 3.0.1 TCK Signed-off-by: Scott Marlow <smarlow@redhat.com>
@scottmarlow how long do you think it would actually take to update the tests rather than exclude them? What sort of time window is available to make test updates? |
The current TCK Process suggests the time window should be quick but if we exclude every TCK test, we will end up with zero tests so I think there should be a balance. Also, if we think that most potential contributors are likely not going to have time, that could be a reason to accept the excluding of the tests. Having said this, I think you as lead of the Tags project should decide whether the tests should be fixed or excluded. You could make that choice with other Tags committers or whomever you like. It is perfectly fine for you to make that decision as well. For TCK challenge resolution, I try to delegate to the respective Spec API lead for the Below is some guidance from the TCK Process. Only the first paragraph is relevant here but I included the other details which covers making test changes in response to a challenge:
|
@scottmarlow I don't have any experience with running the TCK at all, is there any guidance there? What percentage of tests does this challenge account for? |
We plan to make the process of running TCK tests easier for Jakarta EE 11. Some Jakarta EE implementations (e.g. GlassFish + Tomitribe have open sourced their TCK porting kit but I don't think anyone else has so for Open Liberty you likely would need bits that are behind the IBM firewall (same for WildFly currently as well). If you (or someone else) comes up with a Platform TCK pull request, the Platform TCK team can run tests against your branch. We would build your branch and stage a Platform TCK for testing. We could also stage a Standalone Tags TCK for testing as well. We would likely post on the pull request or here with the TCK results of running the Tags tests with Java 21.
Currently, with WildFly I am seeing that the Jakarta EE 10 Platform TCK runs 541 tests. If we exclude the 29 tests then there would be 29 fewer tests run but I think the decision might take into account how important the 29 tests are in terms of whether they are testing Tags Specification requirements that are hard to implement correctly. The |
Just to note, I'll probably build/stage a https://www.eclipse.org/downloads/download.php?file=/jakartaee/platform/10/jakarta-jakartaeetck-10.0.5.zip tomorrow with a Persistence TCK test exclusion that has been outstanding for a while. So if the decision for Tags becomes to exclude the 29 tests by tomorrow, I'll include those changes as well. No pressure either way, just wanted to mention my plans. :-) |
Actually, I will wait to release the Platform TCK until we have a fix for #255 merged in. I will instead just release the Standalone Persistence TCK today. |
I'll try running again with |
-Djava.locale.providers=COMPAT didn't help. |
@pnicolucci do you know of anyone that has started coding a fix for addressing this issue? |
Hi @scottmarlow , I was able to get the TCK running this morning after a couple of days of working with it. Let me try to get something in place now regarding a solution to this problem. I'm sure I'll have TCK-specific questions so I'll post those here as I have them. |
@scottmarlow @pnicolucci - We don't have a release planned for Tags in EE 11. As per TCK process, the service release of TCK [ https://github.com/jakartaee/platform-tck/pull/1219 ] allow for exclusion of tests or adjust the test validation logic to “expand” the validation check.I don't believe we are expanding validation with the fix for the challenge here. |
@gurunrao please see my comment on jakartaee/platform-tck#1219 (comment) This issue is for tags 3.0 for EE 10. It is fine if you respond here. I'll update the jakartaee/platform-tck#1219 (comment) discussion to link to this comment. |
@scottmarlow being unfamiliar with the TCK, can you give me a quick primer on how to build the specific buckets? If I make code changes to the Tags (JSTL) tests what's the easiest way to get them reflected in the test artifacts locally to validate in my dev environment? |
Certainly, it should likely fit in easily with how you are running the existing Jakarta EE 10 Platform TCK in your local environment. The Platform TCK binary archive includes the source that is buildable with a few minor changes to how you are running the jstl tests. Your changes need to be made to the $TS_HOME/src/com/sun/ts/tests/jstl folder before you do the following. Update your script or commands that run the Platform TCK:
The above fragment would be added in your script before the |
@pnicolucci do you need much more time for testing your changes? I would like to at least see other implementations have access to your test changes soon so others can also test. |
@scottmarlow I have one test working with Java 21, it took much longer than anticipated to get the TCK up and running to a state where I can make changes and test. I'm working on this as I have time. If we need a decision right now I'd say our only option is to exclude the tests. If I have something viable for testing soon we can have the discussion as to if we have time or not to include it for Jakarta EE11. I assume we could take some technical debt and update this TCK for Jakarta EE12 if it can't be done for EE11 so we can enable the tests again? |
I agree that could work. I'm personally away from the computer next week but others should be around still if you conclude which direction works best for Tags (we need to release a new Tags 3.0.x TCK + Platform TCK with the changes or tests excluded). I think that we should plan on having the updated Tags 3.0.x/Platform 10.0.x TCKs by June if not sooner. |
@scottmarlow see progress here: jakartaee/platform-tck#1312 I'm working to finish up the remaining sub buckets. |
@scottmarlow can we close this issue? |
Thank you for your help @pnicolucci! |
The Java rules have changed as per https://bugs.openjdk.org/browse/JDK-8304925 (Some date/time strings created with JDK <=19 can not be parsed since JDK 20) + https://bugs.openjdk.org/browse/JDK-8284840 (Update CLDR to Version 42.0) which means that the Tags 3.0 TCK tests are now seeing failures like
Caused by: java.text.ParseException: Unparseable date: "Nov 21, 2000, 3:45:02 AM"
.https://github.com/jakartaee/platform-tck/blob/10.0.x/src/web/jstl/compat/onedotzero/positiveFormatLocalizationContextI18NTest.jsp#L32 contains
<c:set var="dt" value="Nov 21, 2000, 3:45:02 AM"/>
which I do not think can be updated to work on Java 21 (e.g. to use non-breaking space) as that would mean the test would fail on Java 11/17 (which needs a breaking space).Are there other possible fixes that we should consider making?
Note that the https://github.com/jakartaee/platform-tck/blob/10.0.x/src/web/jstl files that contain
PM
are:Note that the https://github.com/jakartaee/platform-tck/blob/10.0.x/src/web/jstl files that contain
AM
are:The good news is that if we do a service release for jakarta-tags-tck-3.0.1.zip that excludes the tests that include ^ breaking space before AM/PM, we will use the same jakarta-tags-tck-3.0.1.zip for validating Standalone Tags 3.0 implementations for Jakarta EE 10 + 11.
We also need to update the EE 11 Platform TCK (tckrefactor branch) to be able to pass (or exclude) the same tests on Java 17 + 21+.
Please see comment jakartaee/platform-tck#1219 (comment) for more information on the failure.
This TCK challenge is for the following 29 tests that fail on Java 21 with WildFly:
We also need to challenge the same ^ 29 tests for GlassFish.
The text was updated successfully, but these errors were encountered: