Add RTC y-websocket-server tests#78179
Merged
Merged
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: 0 B Total Size: 7.93 MB ℹ️ View Unchanged
|
maxschmeling
approved these changes
May 11, 2026
|
Flaky tests detected in b936e57. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25698802191
|
…ests' of github.com:WordPress/gutenberg into add/rtc-y-websocket-tests
Contributor
|
Just following up on your ping with the caveat that I'm not familiar with Gutenberg or RTC, I asked my AI fuzzing harness to evaluate this vs. the other PR. Of course this PR is the right way way to go overall, but it flagged a few things that are useful for fuzzing surface area that are in the hacky/low quality AI fuzzing PR I submitted that this is missing (which could go into another PR):
Controls is maybe a bit funny when presented in a list without context, but it means this thing that's passed in that's a way to send some flags to the WS provider to get it to do custom stuff for testing/fuzzing. It also flagged this:
For the fuzzing setup, it has a bunch of machinery to try to reject false positives which isn't applicable and wasn't applied here, so this is more raw than the normal output of the fuzzer. A 2nd pass flagged these things (again, with the caveat that the normal false positive rejection machinery isn't set up for this); (2) is the same as the above:
|
Contributor
Author
Thanks for testing the fuzzer against this PR! Agreed those are useful for fuzzing surface area, I hadn't considered that. I think a follow-up PR might be the right home for this, or in the fuzzer-specific harness. I think #77920 is doing two jobs: behavior testing for CI (in the near future), and fuzzer instrumentation. This PR pulls behavior testing onto the real Also, many of the listed items don't have a direct analogue in If the fuzzer wants its own provider with whatever controls help it triage, I'd probably prefer a second Does that all sound reasonable to you? I'll address the other correctness bullet points separately, and probably push up some more commits for those. |
…ced before running tests
Contributor
Author
|
@danluu I've implemented most of your review results above, excluding this one:
A full reset is a good idea, but between the two suggestions in there there's a big chunk of code would need changing. This is recommended as a "Follow-up", and I'll leave it out for now. |
Contributor
Yeah, that all sounds reasonable and good to me! |
Comment on lines
+10
to
+11
| // eslint-disable-next-line import/no-extraneous-dependencies -- declared in test/e2e/package.json, only loaded by the WS e2e harness. | ||
| import { WebsocketProvider } from 'y-websocket'; |
Member
There was a problem hiding this comment.
It wouldn't have hurt to add y-websocket to devDependencies in packages/e2e-tests/package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Based on @danluu's changes in #77920, but with the official
@y/websocket-serveras a backend.Replaces that PR's hand-rolled JSON server and client with a minimal harness on top of
@y/websocket-server(the official Yjs WebSocket broker) andy-websocket(the matching client). The test surface, fixtures, and spec files are unchanged.Note that #77920 added a new failing test
collaboration-same-user-title-reload-loss.spec.tsthat appears to be unrelated to the PR. I've removed that test from this PR, but it should be added in an appropriate PR related to that bug.Why?
The previous test harness invented a JSON envelope that base64-wrapped Yjs
updateV2payloads and replayed the entire update log to every joiner. It works as a transport, but it bypassed some built-in transport features: sync step negotiation, binary frames, y-protocols awareness, and some other minor differences. A fuzzer running against that harness can find bugs in the editor'ssync.providersintegration but will miss any differences in the WebSocket broker.vip-real-time-collaboration(a Gutenberg RTC deployment) uses the official protocol, and likely other implementations will use the official Yjs server, so let's test against that one.How?
Replace the custom server implementation with a shim around
@y/websocket-server.Also replaces #77920's test mirroring config, e.g.:
In test/e2e/specs/editor/collaboration/websocket/collaboration-presence.spec.ts:
with configuration to run the same set of e2e tests from the
websocket/folder so we don't risk missing future e2e test additions.Testing Instructions
npm installnpm run wp-env startnpm run test:e2e:rtc-websocketNote that #77920 and this PR show different results.
collaboration-same-user-title-reload-loss.spec.tswas added in #77920, but appears unrelated, and has been removed in this version of the PR. Here's the difference in results between the two branches:In #77920 (hand-rolled sync server):
This PR:
Overall, the test performance is roughly the same, with one removed test in this PR and one additional failure of
three users concurrently edit a large post with diverse blocksincollaboration-stress.spec.ts.Given the results, #77924 is still likely to address valid bugs and we may have uncovered another one.
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Used for: Migration to Yjs packages, PR draft.