-
Notifications
You must be signed in to change notification settings - Fork 43
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
TCK: WebSocket NegDep Challenge #465
Comments
Tagged as a challenge. @markt-asf @joakime can you take a look? |
The tests all pass with Tomcat and Eclipse Tyrus. I'm not adverse to improving the tests but this looks more like an issue with the Liberty / Arquillian integration at this point. |
Which part do you consider to be the issue here? That Liberty fails to start these applications? Or that the Liberty Arquillian plugin considers this to be an Arquillian deployment exception? Since the application doesn't start, there is no valid URL that could be supplied. The plugin could certainly fake one up, but it already knows at this point that no endpoints will exist so why bother? I'm surprised that other plugin implementations wouldn't treat this sort of case the same way. |
@pnicolucci What JDK do you use to run the TCK? Can you try with 21? |
17 & 21. Things work the same in both cases. |
Here is Tyrus script with some old glassfish that works with JDK 21. |
Right, so the fundamental difference here is that Glassfish seems to go ahead and deploy the application anyway:
The WebSocket spec (https://jakarta.ee/specifications/websocket/2.2/jakarta-websocket-spec-2.2#deployment-errors) states:
Liberty has chosen to fulfill "must halt the deployment of the application" as meaning that the application does not end up deployed at all. |
Is there any chance these tests could be excluded from this release? This would allow more time to investigate between the implementations and the Arquillian plugins. The application starting in Glassfish looks a bit concerning since should not start due to a deployment exception. As mentioned, Liberty fails to start the app, and this leads no URL provided error later on in Arqullian. Additionally, my original suggestion was to use |
Following up on my request to exclude the tests. Thanks! |
Looking at this is on my TODO list. I want to understand why the test passes with Tomcat before taking any action on this. |
The test currently expects the app to deploy, but for none of the endpoints to exist, which I believe is exactly how Tomcat currently performs (I know @volosied did a little investigation of Tomcat awhile back, so perhaps he can recall). That's definitely the case for Glassfish. Open Liberty doesn't deploy the app (therefore none of the endpoints exist), but that doesn't play nicely with the test, which does not expect a deployment failure. We actually worked around the app deployment failure in the old TCK by special casing these specific tests in our TCK porting package to ignore the exception and proceed, which is pretty broken. We don't want to do the same thing here because our Arquillian plugin is open source, available for anyone to use, and not specific to TCK testing. The old porting package is proprietary and specific to TCK testing, so that was a marginally acceptable solution there. |
I was pushing for excluding these tests more due to Glassfish's behavior (deploying the app even with an exception). As for Tomcat, here's the output below. It does report
|
I'm pretty sure that |
Challenged tests
Main Package: https://github.com/jakartaee/platform-tck/tree/main/websocket/spec-tests/src/main/java/com/sun/ts/tests/websocket/negdep
wsc_negdep_invalidpathparamtype_srv_onclose_web
wsc_negdep_invalidpathparamtype_srv_onerror_web
wsc_negdep_invalidpathparamtype_srv_onmessage_web
wsc_negdep_invalidpathparamtype_srv_onopen_web
wsc_negdep_malformedpath_web
wsc_negdep_multiplepaths_web
wsc_negdep_onclose_srv_duplicate_web
wsc_negdep_onclose_srv_toomanyarguments_web
wsc_negdep_onerror_srv_duplicate_web
wsc_negdep_onerror_srv_toomanyarguments_web
wsc_negdep_onmessage_pasrv_nomoreendpoints_web
wsc_negdep_onmessage_ppsrv_nomoreendpoints_web
wsc_negdep_onmessage_srv_binarybytebufferint_web
wsc_negdep_onmessage_srv_binaryinputstreamboolean_web
wsc_negdep_onmessage_srv_binaryinputstreamboolean_web
wsc_negdep_onmessage_srv_binarynodecoder_web
wsc_negdep_onmessage_srv_pongboolean_web
wsc_negdep_onmessage_srv_pongduplicate_web
wsc_negdep_onmessage_srv_textbigdecimal_web
wsc_negdep_onmessage_srv_textduplicate_web
wsc_negdep_onmessage_srv_textnodecoder_web
wsc_negdep_onmessage_srv_textreaderboolean_web
wsc_negdep_onmessage_srv_textstringint_web
wsc_negdep_onopen_srv_duplicate_web
wsc_negdep_onopen_srv_toomanyarguments_web
TCK Version
Main Branch of platform-tck/websocket
Description
This is partly a challenge and partly a request for improvement. These web socket tests listed above have moved to use the Arquillian framework. They invoke the application and then expect an exception to occur, as these are negative tests (some misconfiguration exists in the app).
While testing on Liberty, we have encountered various errors when trying to get these tests to run. By design, these apps trigger DeploymentException, which causes them to not start in the server. As a result, Arquillian and any related plugins we use cannot handle the app (since it doesn’t exist for us). After making a few fixes on our side, the last error we’ve encountered is related to the URL:
We’re unsure if any other implementations have been tested yet, but we would expect similar issues to occur. Arquillian is trying to do some Test enrichment, but due to the deployment exception, there is no available URL for these tests.
We propose to remove the invoke all in these tests:
and instead rely on the Arquillian’s ShouldThrowException annotation. This has the added bonus of being able to explicitly check for DeploymentException, instead of just catching Exception as is currently done. This is similar to the CDI TCK’s approach, as linked here and here.
Some extra handling would also be needed to verify that any previously deployed endpoints are removed from service. A draft PR can be provided.
Of if anyone has a workaround for the URL issue above, we’d be happy to accept other suggestions.
Additional context
None
The text was updated successfully, but these errors were encountered: