Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

scottmarlow
Copy link
Contributor

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

Copy link

netlify bot commented Jul 8, 2024

Deploy Preview for jakartaee-specifications ready!

Name Link
🔨 Latest commit bb42e6a
🔍 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 scottmarlow changed the title Please promote https://download.eclipse.org/jakartaee/servlet/6.0/jakarta-servlet-tck-6.0.2.zip Please promote Servlet TCK https://download.eclipse.org/jakartaee/servlet/6.0/jakarta-servlet-tck-6.0.2.zip Jul 8, 2024
@scottmarlow scottmarlow marked this pull request as draft July 8, 2024 20:07
@scottmarlow
Copy link
Contributor Author

@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 jakartaee/platform-tck#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
Copy link
Contributor Author

@markt-asf
Copy link
Contributor

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:

  • the change from a 2.2 DTD to a 5.0 schema for servlet_compat_LeadingSlash_With_web.xml won't break the test
  • the other changes don't impact the Servlet TCK

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

@scottmarlow
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants