Update TCK for changes in WebSocket 2.1

[markt-asf]

---

name: Pull Request
about: Create a pull request for a Platform TCK change
title: 'WebSocket 2.1 updates'
labels: ''
assignees: ''

---

Fixes Issue
None

Related Issue(s)
None.

Describe the change
Updates to TCK for changes in WebSocket 2.1 specification

Additional context

CC @alwin-joseph @anajosep @arjantijms @cesarhernandezgt @dblevins @m0mus @edbratt @gurunrao @jansupol @jgallimore @kazumura @kwsutter @LanceAndersen @bhatpmk @RohitKumarJain @shighbar @gthoman @brideck @scottmarlow

[markt-asf]

This PR is now at the point where a WebSocket 2.1 compliant container should pass all tests.
There are four more changes that I need to review (2 clarifications and 2 new methods). I'm expecting new tests for the new methods. The clarifications may or may not require changes depending on the coverage of the existing tests.

[markt-asf]

Force push was just a rebase to keep things in sync

[markt-asf]

Rebased and added commit to download the new WebSocket client JAR

[jansupol]

The tests under spec folder usually check the assertions required by the Spec document (pdf). This test seems to check this new API method. Should it be in the ee test folder then?

[scottmarlow]

Just for quick reference, the websocket test folder contains subfolders::

- api
- common
- ee
- negdep
- platform
- spec

I'm not sure about the purpose of the spec folder myself versus ee but typically, I thought ee is for the Jakarta EE Full Platform + Web Profile specific tests.

@LanceAndersen do you know the answer to Jan's question in the comment above?

Having a README.me file in the websocket folder that mentions what each folder is for could be helpful.

[jansupol]

It depends on the TCK. Some TCK uses ee for end-to-end tests (WebSocket+JAX-RS), some use ee for enterprise tests (I think JAX-WS, JPA) as opposed to SE tests. Platform folder contains tests for which a full platform is required, for instance for interoperability with CDI, i.e. tests that are only full CTS tests, and are not part of the standalone TCKs.

[scottmarlow]

It depends on the TCK. Some TCK uses ee for end-to-end tests (WebSocket+JAX-RS), some use ee for enterprise tests (I think JAX-WS, JPA) as opposed to SE tests. Platform folder contains tests for which a full platform is required, for instance for interoperability with CDI, i.e. tests that are only full CTS tests, and are not part of the standalone TCKs.

Thanks @jansupol, I had no idea of ^. If there are rules about which folders can include different types of tests, those rules should be documented somewhere, otherwise the pull request reviewers and contributors will have no idea (like me :-).

Personally, I don't yet fully understand the purpose of each of the above mentioned folders. Perhaps api should instead be called javadoc (it seems to only have assertions for javadoc). In the ee (end-to-end) folder, I see both javadoc + spec tests (same in other folders as well).

This PR is now blocking the Platform TCK from building (we are getting build failures) which means we cannot run Platform TCK testing to see where we are. IMO, this needs to be solved so that EE 10 Platform implementations can test if they want to.

CC @gurunrao @alwin-joseph

[markt-asf]

I assume you mean you need to merge this PR to fix the platform TCK build. If that is the case I'd suggest:

• merge this PR now
• if there is an issue with the locations of the tests I've added open an issue and assign it to me
• I'll provide a PR to put the new tests in the right place

[jansupol]

Perhaps api should instead be called javadoc

These tests do not require an implementation, those are tests that verify the Javadoc just using the API jar. Of course only a little can be tested without the implementation, depending on how much functionality the API classes provide.

[jansupol]

@markt-asf The test application war is ws_spec_upgradehttp_web.war. So it creates context path ws_spec_upgradehttp_web for the endpoints. The TCKTestServer endpoint gets deployed when the war is deployed.
But then, ServerEndpointConfig.Builder.create(WSTestServer.class, "/TCKTestServer").build() creates the endpoint config with the /TCKTestServer path and it is not matched with /ws_spec_upgradehttp_web/TCKTestServer. Should the path here contain the context path?

[markt-asf]

No. As per the Javadoc for Builder, the path is relative to the "web socket root" which is, as per section 6.5 of the WebSocket spec, the Servlet context root of the web application.

[alwin-joseph]

Hi @markt-asf ,

We have 3 test failures in websocket TCK for tests fixed/added in this PR.
Could you take a look if these are related to the changes here.

FAILED........com/sun/ts/tests/websocket/ee/jakarta/websocket/server/serverendpointconfig/WSClient.java#getUserPropertiesTest_from_standalone
FAILED........com/sun/ts/tests/websocket/ee/jakarta/websocket/session/WSClient.java#getRequestURITest_from_standalone
FAILED........com/sun/ts/tests/websocket/spec/servercontainer/addendpoint/WSClient.java#getRequestURITest_from_standalone

Full logs : https://ci.eclipse.org/jakartaee-tck/blue/rest/organizations/jenkins/pipelines/jakartaee-tck/branches/master/runs/1662/nodes/45/steps/56/log/?start=0
TCK bundle : https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck/job/master/1662/artifact/standalone-bundles/jakarta-websocket-tck-2.1.0.zip
Test were run with Glassfish 7.0.0-M2 from https://jakarta.oss.sonatype.org/content/repositories/staging/org/glassfish/main/distributions/glassfish/7.0.0-M2/.

@jansupol @gurunrao @scottmarlow

[markt-asf]

The two getRequestURI() failures are Glassfish bugs. Issue 228 clarified that the full URI must be returned. As per the provided logs, the expected value was ws://localhost:8080/ws_session_web/TCKTestServer?test1=value1&test2=value2&test3=value3

The root cause of the getUserProperties() failure isn't so obvious. I'll need a little longer to look into that.

[markt-asf]

I can't be 100% without access to the Glassfish logs and I'm not set up to run the TCK with Glassfish locally but the getUserProperties() issue looks like it is another Glassfish bug. issue 235 clarified the expected behaviour for user properties.

[alwin-joseph]

I can't be 100% without access to the Glassfish logs and I'm not set up to run the TCK with Glassfish locally but the getUserProperties() issue looks like it is another Glassfish bug. issue 235 clarified the expected behaviour for user properties.

Glassfish server logs from my local run : server.log

[markt-asf]

Tx. Confirmed as a Glassfish issue:

java.lang.IllegalStateException: User properties map has [0] entries when 3 are expected

[jansupol]

serverendpointconfig/WSClient.java#getUserPropertiesTest seems to be wrong to me anyway:
The client creates two requests.

• The first one is fine: SEC_USER_PROPERTIES is set, UserPropertiesConfigurator checks there are two properties, it removes one, adds two. The WsProgramaticUserPropertiesServer checks there are three properties.

• For the second request, the static SEC_USER_PROPERTIES are not recreated, there are three properties, and UserPropertiesConfigurator fails with three properties.

[markt-asf]

The behaviour in this area has been clarified for WebSocket 2.1 as there were differences in behaviour between containers. The clarifications, including the associated commit, can be found in WebSocket issue 235.
The expected behaviour is as follows:

• ServerEndpointConfig.getUserProperties() provides the possibly empty initial set of properties
• During the execution of ServerEndpointConfig.Configurator.modifyHandshake() the user properties obtained from ServerEndpointConfig.getUserProperties() must:
  a) Be a per connection (a.k.a session) copy of the user properties. In terms of implementation this might mean wrapping the ServerEndpointConfig but other solutions may be applicable
  b) This copy - including any modifications - must be used as the initial contents of Session.getUserProperties()

What this means for this test for request 1 is:

• At the start of ServerEndpointConfig.Configurator.modifyHandshake() the user properties must be a per connection/session shallow copy of the return value from ServerEndpointConfig.getUserProperties() so it should contain 2 entries, KEY_1 and KEY_2.
• modifyHandshake() removes KEY_2 and adds KEY_3 and KEY_4 so at the point the method exits user properties should contain 3 entries.
• When the initial WebSocket message is processed the session user properties should be a shallow copy of the user properties at the end of modifyHandshake() - i.e. contain 3 properties.
• The message handler removes one property (KEY_4) and adds another (KEY_5). These actions are essentially a NO-OP but there are there mainly to make sure those changes don't appear anywhere they shouldn't.

For request 2, the expected behaviour is exactly the same. None of the changes made to user properties during request 1 should be visible to request 2.

From the description above it sounds like modifyHandshake() is being passed the original properties Map rather than the required shallow copy and request 2 is seeing the modifications made by request 1.

[jansupol]

@markt-asf Thanks for the reply.

In terms of implementation this might mean wrapping the ServerEndpointConfig but other solutions may be applicable

I very much disagree:

• the javadoc mandates that a implementation of ServerEndpointConfig.Configurator.modifyHandshake() should ensure that ServerEndpointConfig.getUserProperties() is per connection. But UserPropertiesServerEndpointConfig is a user code that should behave as mandated by the javadoc. The implementation should not band the user code.
• Wrapping the ServerEnpointConfig is wrong. The user passes the implementation in and expects that instance to be accessible. The user might require additional methods he would have implemented, but the wrapped class would not provide these methods. Effectively, there is no good way to ensure the user code gets modified to wrap getUserProperties().
• The user might deliberately want to modify the properties each time modifyHandshake is invoked.
• When the modifyHandshake() takes place, there is no Session, yet. As per Javadoc, a Session is created as the WebSocket handshake completes successfully. A copy of the Session properties at the time of modifyHandshake() sounds dodgy.

[jansupol]

I have challenged the test: #873

[markt-asf]

Since this PR has already been merged, I'll respond in #873.