-
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
Introduce alternative fix for accepted TCK challenge 1313 for 10.0.x branch #1335
Introduce alternative fix for accepted TCK challenge 1313 for 10.0.x branch #1335
Conversation
…allenge jakartaee#1313, old schema in test artifacts." This reverts commit b383a23.
@pnicolucci could you please review this change. |
Perhaps more changes are needed as I get errors testing locally. |
7743a11
to
449b218
Compare
I fixed one problem but am now seeing that some of the old web-app_2_3 fields are no longer supported like |
449b218
to
3ee3121
Compare
@pnicolucci @markt-asf The https://jakarta.ee/specifications/servlet/5.0/jakarta-servlet-spec-5.0#a-basic-example spec also has The purpose of this pull request is to avoid excluding Platform TCK tests for challenge 1313. CC @gurunrao |
@scottmarlow https://jakarta.ee/xml/ns/jakartaee/web-app_6_0.xsd includes https://jakarta.ee/xml/ns/jakartaee/web-common_6_0.xsd which contains context-param |
Thanks @pnicolucci! I suppose that we can either remove fields that are no longer supported or rename fields if the names have changed. If the removed fields are important to the tests, then we have to decide if the test can still exist in some form in EE 10 (as well as EE 9.1 which was also challenged). Also pending would be web-app_2_3.dtd -> web-app 5.0 change and web-app_2_2.dtd -> web-app 5.0. We also need to update from sun-application-client_5_0-0.dtd -> application_9.xsd. We already excluded the tests. If anyone has time to update these schemas, please speak up. |
@scottmarlow can you point to the logs that show I'm seeing an issue in the logs for |
…pp_5_0.xsd Signed-off-by: Scott Marlow <[email protected]>
3ee3121
to
0b45fc9
Compare
I do see the tck/vi/glassfish7/glassfish/domains/domain1/logs/server.log which only contains the |
…pp_5_0.xsd Signed-off-by: Scott Marlow <[email protected]>
This change doesn't work so more exploration would be needed to bring it in, perhaps in a future EE 10 release as other outstanding TCK challenges have been waiting for the 10.0.5 changes for too long now (really since last year for some). |
Hi @scottmarlow can you elaborate on what doesn't work? Do you have logs of the most recent change? |
@pnicolucci https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck-scottmarlow/job/issues_1313_10.0.x_part2/4/artifact/jstl-results.tar.gz contains
See attached server.log for the errors. |
Cool! I actually, should have a fix for this! See the changes here: 10.0.x...pnicolucci:jakartaee-tck:isELIgnored you can pull this commit in if you want and test it. I got good results locally. This is due to https://jakarta.ee/specifications/pages/3.0/jakarta-server-pages-spec-3.0#_Deactivating_EL_Evaluation where EL is ignored on the previous webapp 2.3 version that was being used. I updated the individual pages that I hit the problem with locally using the page directive rather than the entire application via the web.xml to make the set of changes as minimal as possible. |
As an alternative, you could try adding the following to the web.xml for the application:
I didn't test this route yet but if you prefer this over my current proposal I can try it. |
+1 thank you! I started a test build via https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck-scottmarlow/job/issues_1313_10.0.x_part2/5 which may also run the Platform TCK tests as well, will see how that goes.
Good catch! |
https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck-scottmarlow/job/issues_1313_10.0.x_part2/5/testReport/junit/com.sun.ts.tests.jstl.compat.onedotzero/JSTLClient/jakartaeetck_run___jstl___positiveCWOTest has a failure from the Full Platform TCK run I started with the 10.0.x...pnicolucci:jakartaee-tck:isELIgnored change:
It sounds like we need to try the I'll work on an update for this. |
@scottmarlow yes that web.xml change looks correct to me. I'm not sure why the page directive isn't working for you. I updated the jsp for the test that is failing in your most recent logs. Could you test the new application manually and send it to me to take a look at and test? Perhaps something isn't cleaned up in your environment or wasn't built correctly? |
Regarding two Servlet failures: https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck-scottmarlow/job/issues_1313_10.0.x_part2/5/artifact/servlet-results.tar.gz now contains the server.log with the following for the servlet failures:
|
In making the scottmarlow@69127c6 change, I noticed that I also need to update the src/com/sun/ts/tests/jstl/compat/onedotzero/jstl_1_0_compat_web.war which I didn't change before in my previous change. |
@scottmarlow Servlet failures look due to: https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/servlet/compat/LeadingSlash/WithLeadingSlash/servlet_compat_LeadingSlash_With_web.xml#L27 could try removing those and see if things clean up. |
Thanks, added 93532a6 to remove Description from servlet_compat_LeadingSlash_With_web.xml#L27 and after the current test runs completes, will start a new one. |
One servlet failure left that fails due to
This test previously passed with the older
I think we need to exclude the |
Will build and run just the servlet test this time. If that passes, we will do a full run against the staged TCK after this change is merged. |
@scottmarlow for the Jakarta Tags changes, I guess if we're going with the web.xml update and that is working you can remove the changes to the JSP files from this PR as well. |
Signed-off-by: Scott Marlow <[email protected]>
Signed-off-by: Scott Marlow <[email protected]>
…URLClient.WithoutLeadingSlashTest as it cannot work without 2.2 Servlet web.xml Signed-off-by: Scott Marlow <[email protected]>
c6bfb63
to
c3757e9
Compare
I'm going to build/run again to see what happens without the JSP file changes. |
…3.0 implementations can also run the TCK on Java 21 Signed-off-by: Scott Marlow <[email protected]>
5814db0
to
dbf31f3
Compare
…CKs so implementations can also run the TCK on Java 21 Signed-off-by: Scott Marlow <[email protected]>
Will also release Tags 3.0.1, Servlet 6.0.2, Server Pages 3.1.1, Connector 2.1.2 TCKs so implementations can also run the TCK on Java 21 |
https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck-scottmarlow/job/issues_1313_10.0.x_part2/8/ passed the testing. Just need a positive approval on the pull request from one of you @pnicolucci or @markt-asf or @gurunrao |
.../servlet/compat/LeadingSlash/WithoutLeadingSlash/servlet_compat_LeadingSlash_Without_web.xml
Outdated
Show resolved
Hide resolved
…/servlet/compat/LeadingSlash/WithoutLeadingSlash/servlet_compat_LeadingSlash_Without_web.xml Signed-off-by: Scott Marlow <[email protected]>
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
Thanks @scottmarlow for fixes of outdated test schema. |
Will stage TCKS today for EE 10 Platform TCK, Tags, Servlet, Connector, Persistence. |
Fixes Issue
#1313
Describe the change
Revert b383a23 and update http://java.sun.com/dtd/web-app_2_3.dtd to https://jakarta.ee/xml/ns/jakartaee/web-app_5_0.xsd