Repository navigation
Conversation
Type of changeTest only. How to testExpected: |
Push is enabled for the whole test application with @Push, over WebSocket. The view updates the page from a background thread after the response to a click, so the update can only arrive over the push connection.
7a8c966 to
50ff07f
Compare
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 1 comment.
| Finding | |
|---|---|
| 🧪 | The test never checks which transport delivered the update, so it can't show that WebSocket works under the strict policy. |
|
|
||
| $(NativeButtonElement.class).id("start").click(); | ||
|
|
||
| waitUntil(ExpectedConditions.textToBe(By.id("pushed"), |
There was a problem hiding this comment.
🧪 The test never checks which transport delivered the update, so it can't show that WebSocket works under the strict policy.
If the WebSocket upgrade fails without a policy violation, Atmosphere quietly falls back to long polling and the update still arrives. One way this happens is when the websocket starter is dropped or not picked up. The test passes either way, even though the README row and the pom comment both claim WebSocket coverage.
Assert the transport the client actually used, as PushConfigurationWebSocketIT does in test-root-context. That way, losing WebSocket fails the test.
PushIT.java:42 · tests
There was a problem hiding this comment.
Done. The server now records the transport of every push request, the view shows it with the pushed update, and PushIT asserts it is only websocket. A fallback to long polling fails the test. PushConfigurationWebSocketIT reads the configured transport, not the one used, so I did not copy it.
|
@heruan this "chore:"/"test:"/"refactor:" PR has been approved automatically, but auto merge has not been enabled for it. You can enable the auto merge by yourself, or ask another member for another review. [Message is sent from bot] |
The server records the transport of every push request, and the push views show it with the pushed update. The WebSocket test fails when the browser falls back to long polling, and a second view pushes over long polling.
heruan
changed the title
|
@heruan this "chore:"/"test:"/"refactor:" PR has been approved automatically, but auto merge has not been enabled for it. You can enable the auto merge by yourself, or ask another member for another review. [Message is sent from bot] |
|
Follow-up to #26975, Part of #26154, Part of #26155, Part of vaadin/platform#9488
test · flow-tests · the strict csp test module; no application
Push was on the list of things still to verify with
csp=strict, and loading a lazy chunk had no test in the strict module. The module now covers push over WebSocket, push over long polling, and navigation to a view whose frontend module is in a lazy chunk. All pass onmainwith no policy violation, so no fix is needed in Flow.Risks:
❓
@Pushis on the application class, so every scenario of the module now runs with a push connection open. This is how a real application uses push, but it changes the setup of the existing tests.❓ The long polling view cannot just set the transport when it is attached: the browser still opened a WebSocket. The view closes push on attach and opens it again over long polling on the click. I did not look into why; it is not related to the policy.
✅ A test module only: no API, behavior, security, serialization, threading, performance or migration impact
Added
push.PushViewandPushIT: a click starts a background task that updates the page 500 ms later throughUI.access, after the response to the click is sentAdded
push.LongPollingPushViewandLongPollingPushIT, the same over long pollingAdded
PushTransportRecorder, a servlet filter that records the transport of every push request of a session; the views show it with the update and the tests assert it, so a silent fallback from WebSocket to long polling failsPushITApplicationimplementsAppShellConfiguratorwith@PushAdded
spring-boot-starter-websocket, without it push falls back to long polling in an embedded serverAdded
chunk.LazyChunkView,chunk.LazyChunkStartViewandLazyChunkIT: the test checks that the module of the target view is not loaded with the page, then navigates to itAdded the scenarios to the module README