search

jakartaee / servlet

Jakarta Servlet
https://eclipse.org/ee4j/servlet
Other
261 stars 85 forks source link

TCK: DefaultMappingTests app is missing servlet classes #691

Closed brideck closed 1 month ago

brideck commented 1 month ago

Challenged Tests: servlet.tck.spec.defaultmapping.DefaultMappingTests

TCK Version: Servlet TCK 6.1.0

Tested Implementation: Open Liberty

Description: There is a Shrinkwrap app construction error in this test class. The deployment descriptor for the application defines five servlets with classes servlet.tck.spec.requestmap.TestServlet*, but the built app does not contain any of these classes from the requestmap package. The test class should be updated to import the referenced requestmap classes and then include them in the app.

Based on what the test is doing, it probably only actually needs one additional servlet in the app (TestServlet3, as currently written), instead of all five. It would also be a bit cleaner if there wasn’t a cross-package dependency like this at all. One or more servlet classes could be added to servlet.tck.spec.defaultmapping and be used instead of the ones from requestmap.

pnicolucci commented 1 month ago

@markt-asf @stuartwdouglas another TCK challenge, I've labeled it as such.

markt-asf commented 1 month ago

I agree that the test needs some work but I don't think a challenge is required since the test in its current form will pass on a specification compliant server. The issue is that it might not fail on a non-compliant server. Therefore, I see no need to exclude this test from a TCK run.

brideck commented 1 month ago

I wouldn't ask to exclude it, I would ask to fix it in response to this challenge. It was ported incorrectly from the Platform TCK.

I don't think it's really testing what it thinks it is without some additional servlets present, is it? Or is the mere presence of bogus servlet info in the descriptor enough to exercise the default mapping mechanism?

Note: Our particular problem in OL at present doesn't even have anything to do with servlets. Our Faces implementation is balking at the missing classes, and while we can potentially force some changes into that community for us, it's indicative of the complications others implementing the full EE stack might see due to having crippled apps as part of a TCK.

markt-asf commented 1 month ago

While I agree the test isn't behaving as intended, I think it is successfully testing the mapping. If the request is incorrectly mapped to one of the non-existent servlets I'd expect a 500 error due to a ClassNotFoundException.

That said, I also agree this needs to be fixed. I've already fixed it locally. I ran out of time yesterday and didn't get as far as committing it. There are a couple of other changes I want to wrap up with this but I expect to have this fixed in master and 6.1.x later today.

pnicolucci commented 1 month ago

@markt-asf since we made changes to the 6.1.x TCK for this issue we should tag it with TCK:accepted any objection?

markt-asf commented 1 month ago

I'd lean towards not on the grounds that: