hocuspocus: Add provider/server wire-protocol integration test with CI version-skew guard - #23755
Conversation
ae91175 to
5abea44
Compare
|
Warning Flaky specs
|
489b07a to
bb450fd
Compare
5abea44 to
25dec9b
Compare
8ed13d1 to
a12237e
Compare
43fca17 to
5d4e6d8
Compare
There was a problem hiding this comment.
Pull request overview
Adds a safety net against future client/server Hocuspocus version drift by (1) continuously validating the real provider↔server wire-protocol handshake across an allowed one-major skew window, and (2) surfacing a non-blocking PR warning when the client/server majors diverge.
Changes:
- Add a Vitest integration test that boots the Hocuspocus server in-process and verifies connect/auth/load/sync using both the current and previous provider major.
- Add a GitHub Actions workflow + script that compares
@hocuspocus/provider(frontend) vs@hocuspocus/server(extension) major versions and posts/removes a sticky PR warning comment on skew. - Add
@hocuspocus/providerand a pinned “previous-major” npm alias (@hocuspocus/provider-prev) to the extension devDependencies.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
script/ci/hocuspocus_version_skew.sh |
Reads dependency ranges via jq, extracts majors, and exports outputs used by the PR-commenting workflow. |
extensions/op-blocknote-hocuspocus/test/integration/provider-server-sync.test.ts |
New provider↔server handshake integration test with a two-major provider matrix and installed-version guard. |
extensions/op-blocknote-hocuspocus/package.json |
Adds current provider plus a previous-major alias for the integration matrix. |
extensions/op-blocknote-hocuspocus/package-lock.json |
Lockfile updates to include the added provider packages and their dependency trees. |
.github/workflows/hocuspocus-version-skew.yml |
New workflow that runs on relevant package.json changes and posts/deletes a sticky warning comment when majors skew. |
Files not reviewed (1)
- extensions/op-blocknote-hocuspocus/package-lock.json: Generated file
5d4e6d8 to
ff65e01
Compare
brunopagno
left a comment
There was a problem hiding this comment.
Nice job flexing the specs & CI skills
I vote for shipping it 🚢 🚢 🚢
| "@eslint/js": "^9.35.0", | ||
| "@eslint/json": "^1.2.0", | ||
| "@hocuspocus/provider": "^4.2.0", | ||
| "@hocuspocus/provider-prev": "npm:@hocuspocus/provider@^3", |
There was a problem hiding this comment.
I think this line is the only thing I don't like, as it's a bit obscure to whoever is observing.
We could call it @hocuspocus/provider-test-only to make it a bit more specific. Curse JSON for not allowing comments
There was a problem hiding this comment.
We could call it @hocuspocus/provider-test-only to make it a bit more specific. Curse JSON for not allowing comments
Good idea- happy to rename. For context, this is only added to the dev/test bundle of extentions/op-blocknote-hocuspocus to run compatibility tests in the event the provider version is bumped before the server catches up. Given @myabc 's feedback - perhaps all this can be simplified via dependabot dependency grouping?
There was a problem hiding this comment.
@akabiru I've done some digging into this and it looks like Dependabot can't keep server and client in lock-step in this case.
- What it can do: group updates for a dependency of the same name across multiple manifests (introduced relatively recently – see blog article). Unfortunately this doesn't help us since
@hocuspocus/providerlives in/frontendand@hocuspocus/serverin/extensions/op-blocknote-hocuspocus, i.e. different names in different dirs. - What it can't do: one PR for the whole
@hocuspocus/*group across both directories.
That said, I've created a PR to consolidate Dependabot npm entries and add a grouping for @hocuspocus/*. It's mostly a DRY-ness improvement; it also couples the server packages (@hocuspocus/server + extension-logger) within the extension's PR. Client and server still get separate PRs, each whenever its own update lands.
There was a problem hiding this comment.
Thanks Alex, as per our chat- will look into whether re-using/extending the existing script #23755 (review) is feasible (before merge)
a12237e to
3d9bfd7
Compare
myabc
left a comment
There was a problem hiding this comment.
@cbliard added a script a while back to help keep Primer version in lock-step. At the time I remember suggesting something rather over-engineered (reading package-lock.json and Gemfile.lock by their respective Bundler APIs). It may be that this simple shell script also does the trick.
script/check_same_primer_view_components_version_everywhere.
| PROVIDER_RANGE=$(jq -r '.dependencies["@hocuspocus/provider"] // empty' frontend/package.json) | ||
| SERVER_RANGE=$(jq -r '.dependencies["@hocuspocus/server"] // empty' extensions/op-blocknote-hocuspocus/package.json) |
There was a problem hiding this comment.
probably moot, but should we parse package.json or package-lock.json? script/check_same_primer_view_components_version_everywheredoes the latter.
The provider (frontend) and server (extension) live in separate dependabot ecosystems and bump independently, so they can silently drift across a major. A soft-warning sticky PR comment surfaces a major mismatch for review without blocking, since Hocuspocus supports a one-major skew in both directions.
The existing extension specs exercise the server hooks with a mocked API but never a real client, so nothing proves the @hocuspocus/provider actually speaks the wire protocol to the server. This boots the server in-process and connects a real provider, asserting the connect/authenticate/load/sync handshake for both the current and previous provider majors — making the one-major skew the version-skew guard tolerates a standing assertion.
ff65e01 to
761efb2
Compare
The npm alias was @hocuspocus/provider-prev; JSON can't carry a comment saying it exists solely to pin the previous major for the skew test. Renamed to @hocuspocus/provider-prev-test-only so the intent is on the key.
I had a look and the main difference is that for hocuspocus we compare only majors which would be identical in package.json and package-lock.json; the primer script is comparing exact versions across files which is not directly needed here. |
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
|
@copilot The following spec(s) were detected as flaky on PR #23755:
Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to investigate the root cause and apply a fix. Open a separate pull request with the change and request a review from @akabiru. |
https://community.openproject.org/wp/STC-834 · Follows #23754 (Hocuspocus v4.2.0 lockstep bump)
The Hocuspocus client and server live in separate dependabot ecosystems and ship as independently deployed artifacts, so they can drift apart in version on future bumps — with nothing testing that the real
@hocuspocus/provider↔ server wire protocol still holds across that drift.This adds a failsafe on two fronts: a vitest integration test that boots the server in-process and connects a real provider, asserting the connect/auth/load/sync handshake across adjacent provider majors so the supported skew stays proven; and a soft-warning CI check that flags whenever the client and server majors diverge, since only a single major of drift is supported (two or more is not).
The check never blocks the build, and the test runs in the existing
npm testwith no workflow change.