search

jakartaee / specifications

Documentation base for Specification pages be published at jakarta.ee via Hugo and git submodules
Eclipse Public License 2.0
121 stars 79 forks source link

Please promote Servlet TCK https://download.eclipse.org/jakartaee/servlet/6.0/jakarta-servlet-tck-6.0.2.zip #761

Closed scottmarlow closed 1 month ago

scottmarlow commented 4 months ago

Please promote the Servlet 6.0.2 TCK for https://github.com/jakartaee/platform-tck/issues/1313 which addresses Platform TCK Challenge to update from optional web-app_2_3.dtd to (required) web-app_5_0.xsd

netlify[bot] commented 4 months ago

Deploy Preview for jakartaee-specifications ready!

Name Link
Latest commit bb42e6a44c7ff4e0cbd89e877776c642e76263de
Latest deploy log https://app.netlify.com/sites/jakartaee-specifications/deploys/668c30c55999140008ed9a15
Deploy Preview https://deploy-preview-761--jakartaee-specifications.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

scottmarlow commented 4 months ago

@markt-asf are you okay with releasing a new Servlet 6.0.2 TCK zip that has been changed to use a web-app_5_0.xsd for one test? The related discussion is on https://github.com/jakartaee/platform-tck/issues/1313 and doesn't involve Servlet specific TCK tests (as far as I know).

FYI, there will still be one test with web-app_2_3.dtd (this test is excluded from the EE 10 Platform TCK which cannot include web-app_2_3.dtd or web-app_2_2.dtd tests).

scottmarlow commented 4 months ago

https://www.eclipse.org/downloads/download.php?file=/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-servlet-tck-6.0.2.zip is the TCK to be staged when this pr is marked ready for review.

markt-asf commented 4 months ago

Not essential but I think the web.xml changes should also have been reverted for servlet_compat_LeadingSlash_With_web.xml. It shouldn't change the outcome of the test (which is excluded anyway) but I think the intention of the test is that this should use the 2.2 DTD.

The purpose of the other tests also looks to be testing compatibility of 2.2 DTDs in various scenarios. I don't think changing the deployment descriptor was correct there either.

These TCK tests are testing optional functionality. The changes made means the tests aren't performing the tests as intended. I think that is incorrect. If there is optional functionality there needs to be optional TCK tests OR these tests should be completely removed. I think leaving these tests in their current state is just going to cause confusion later as the current code comments are now not consistent with the test behaviour.

I'm not going to object to the release of a 6.0.2 Servlet TCK since:

but I do want to register my view that I think these changes are incorrect.

scottmarlow commented 4 months ago

Thanks @markt-asf! sounds like this draft pr should be closed and that a new Platform issue should be opened for the next Jakarta EE 10 Platform TCK update to re-examine why these tests aren't optional.