Repository navigation
[lexical-y] Chore: Cover the schema version gate's remaining paths, and update stale test comments - #9537
Conversation
…nd update stale test comments ## Description An audit of the `@lexical/y` tests after the stack merged, with a v8 coverage run of `packages/lexical-y` and `packages/lexical-mark` (lines 94.8%, branches 90.4%). New tests in `SchemaVersion.test.ts`, for the schema gate paths no test reached, and the read-only status the gate holds: - A newer record that arrives while a local update runs, and that update then throws: the gate waited for a commit that never came, and the next update is refused (`schemaGatesWrite` dropping `schemaGateAfter`). - A newer record in a remote change while the app's listeners keep queuing local updates past the read's commit limit: the read gives up before its `before` step can gate, and only the recovery gates (the `recover` path of the remote read). Removing that gate makes this test fail with the binding still `'active'`. - A newer record in a remote change whose read throws: the editor is rebuilt from the document with the newer client's edit, and the binding is read only (gated before the read; the recovery gates again). - A binding the gate holds read only stays read only, never editable, and does not warn again when another client records a new layout. Comments that no longer matched the code: - `TableOnOpen.test.ts` said the binding reads the document when it registers; it starts in `YCollabExtension`'s `afterRegistration`, once every extension has registered. - Four tests pointed at "Known open issues" or "Known limits", sections of the #9400 PR description that the repository does not have; they now name #9400, the y.md limit, or the issue filed since (#9514). Every `test.fails` still fails on `main` (46 expected failures in the full unit run), and no test is skipped beyond the environment-gated perf and replay suites. ## Changeset None: tests only. ## Test plan ### Before ``` $ pnpm exec vitest --project unit --project scripts-unit --no-watch Test Files 585 passed | 3 skipped (588) Tests 18134 passed | 46 expected fail | 6 skipped (18186) ``` Coverage of `Binding.ts` before: `schemaGatesWrite`'s reset of `schemaGateAfter` (line 1754) and the remote read's recovery gate (line 2530) never ran. ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/SchemaVersion.test.ts Tests 25 passed (25) ```
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
lexical
@lexical/a11y
@lexical/clipboard
@lexical/code
@lexical/code-core
@lexical/code-prism
@lexical/code-shiki
@lexical/compiler
@lexical/devtools-core
@lexical/dragon
@lexical/eslint-plugin
@lexical/extension
@lexical/fast-check
@lexical/file
@lexical/hashtag
@lexical/headless
@lexical/history
@lexical/html
@lexical/internal
@lexical/link
@lexical/list
@lexical/mark
@lexical/markdown
@lexical/mdast
@lexical/offset
@lexical/overflow
@lexical/plain-text
@lexical/react
@lexical/rich-text
@lexical/selection
@lexical/table
@lexical/tailwind
@lexical/text
@lexical/utils
@lexical/y
@lexical/yjs
commit: |
…ures ## Description Three expected failures pointed at the list of known open issues in #9400's description. Each now has an issue, which the comment names: S4-8 (#9571), ProdUndoNodeSelection (#9572), and R16A-2, whose cause is #9568. Comments only. ## Test plan ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/FollowUps.test.ts packages/lexical-y/src/__tests__/unit/regressions/QuotesAndSlots.test.ts packages/lexical-y/src/__tests__/unit/regressions/Undo.test.ts Test Files 3 passed (3) Tests 8 passed | 8 expected fail (16) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/FollowUps.test.ts packages/lexical-y/src/__tests__/unit/regressions/QuotesAndSlots.test.ts packages/lexical-y/src/__tests__/unit/regressions/Undo.test.ts Test Files 3 passed (3) Tests 8 passed | 8 expected fail (16) ```
…ates to it ## Description #9537 updates the comments of the update listener, R16A-2, S4-8 and ProdUndoNodeSelection expected failures, and names their issues. This change no longer edits them, so the two pull requests do not conflict. Comments only. ## Test plan ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/Fuzz.test.ts packages/lexical-y/src/__tests__/unit/CompositionHoldBack.test.ts packages/lexical-y/src/__tests__/unit/CompositionHoldBackBlockIds.test.ts packages/lexical-y/src/__tests__/unit/UpdateErrors.test.ts packages/lexical-y/src/__tests__/unit/regressions/FollowUps.test.ts packages/lexical-y/src/__tests__/unit/regressions/Joins.test.ts packages/lexical-y/src/__tests__/unit/regressions/QuotesAndSlots.test.ts packages/lexical-y/src/__tests__/unit/regressions/Undo.test.ts packages/lexical-y/src/__tests__/unit/BlocksAndLines.test.ts Test Files 9 passed (9) Tests 2025 passed | 15 expected fail (2040) ``` ### After ``` $ pnpm exec vitest run --project unit (the same files) Test Files 9 passed (9) Tests 2025 passed | 15 expected fail (2040) ```
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the audit. The four new SchemaVersion tests pass, and the re-pointed comments match their targets at this head. Three optional suggestions on the tests, none blocking:
1. Pin the recovery path in the commit-limit test (packages/lexical-y/src/__tests__/unit/SchemaVersion.test.ts:473-475). The test reaches the recovery gate because 40 listener updates fall inside a window: with this scenario, 16-31 updates are all committed before the read (its before step gates), 32-47 make the read give up and the rebuild succeed, and 48 or more make the rebuild give up too, so the binding fails closed (UNKNOWN_NODE_TYPES_WARNING, reason 'unreadable'). At 48 every current assertion still passes, so a change in how many pending updates the read absorbs would move the test onto the fail-closed path without failing it. At 40 the warnings are exactly these two, so asserting them keeps the test on the path its name describes:
expect(codes(older)).toEqual([
RECOVERED_READ_WARNING,
UNSUPPORTED_SCHEMA_WARNING,
]);2. The neighbouring 20-update test still says it recovers (SchemaVersion.test.ts:408-447, comment at lines 417-418). With 20 listener updates its read commits them all and gates in before; no recovery runs, and the test keeps passing without the recovery gate at Binding.ts:2530. Since this pull request corrects stale test comments, renaming it (for example "a read that first commits many pending local updates still gates") and fixing that comment would complete the audit.
3. Binding.ts:1754 is executed rather than guarded. "a record that arrives while an update runs that then throws" is the only test that reaches schemaGatesWrite's reset of schemaGateAfter, but the file still passes with that line removed: the gateSchema() that follows at Binding.ts:3236 clears schemaGateAfter itself (line 1721). The test pins the outcome that matters (the next update is refused and the binding is read only); the description's "(schemaGatesWrite dropping schemaGateAfter)" reads as more than that.
Confirmed against the code: removing the recovery gate at Binding.ts:2530 makes the commit-limit test fail with 'active', as the description says, and removing the before gate at line 2503 makes "a newer record in a change whose read throws" fail on the warning order. The corrected TableOnOpen.test.ts comment matches YCollabExtension starting the binding in afterRegistration, after every extension's register; the other comments match #9514, #9568, #9571, #9572, the lists in #9400's description, and y.md's Limits, "Other races". As a tests-only change it needs no changeset (CONTRIBUTING.md, Changesets).
| newer.binding.initialize(); | ||
| type(newer, 'd'); | ||
| await new Promise(resolve => setTimeout(resolve, 0)); | ||
| // More pending commits than the read waits for: it gives up before it |
There was a problem hiding this comment.
Optional: the test just above ("a recovery that first writes many pending local updates still gates", lines 408-447) uses 20 listener updates, which its read commits in full before gating in before. It never reaches the recovery its name and its comment at lines 417-418 describe, and it keeps passing without the gate at Binding.ts:2530; with this scenario the read only gives up from 32 updates. Renaming it (for example "a read that first commits many pending local updates still gates") and fixing that comment would match what this comment says.
| await Promise.resolve(); | ||
| expect(caught.length).toBeGreaterThan(0); | ||
| expect(older.binding.status).toBe('readonly'); | ||
| expect(codes(older)).toContain(UNSUPPORTED_SCHEMA_WARNING); |
There was a problem hiding this comment.
Optional: from 48 listener updates this scenario fails closed (UNKNOWN_NODE_TYPES_WARNING, reason 'unreadable') and every assertion here still passes. At 40 the warnings are exactly these two, so pinning them keeps the test on the recovery path:
| expect(codes(older)).toContain(UNSUPPORTED_SCHEMA_WARNING); | |
| expect(codes(older)).toEqual([ | |
| RECOVERED_READ_WARNING, | |
| UNSUPPORTED_SCHEMA_WARNING, | |
| ]); |
| {discrete: true}, | ||
| ), | ||
| ).toThrow('app failure'); | ||
| // The update the gate waited for never committed: the next one is not |
There was a problem hiding this comment.
FYI: this test reaches schemaGatesWrite's reset at Binding.ts:1754, but it (and the rest of the file) still passes with that line removed, because the gateSchema() call that follows at Binding.ts:3236 clears schemaGateAfter too (line 1721). It guards the outcome, the next update refused, rather than the reset itself.
…-commits case
## Description
The schema gate case "local updates still pending past the commit limit:
the read does not run, and the recovery gates" checked only that the
binding ends read only with the unsupported-schema warning. A fail-closed
path ends there too, so the case could pass without the recovery read it
claims to cover (with 48 pending updates instead of 40 it already does,
on the unknown-node-types path). It now asserts the exact warnings of the
recovery: the recovered read, then the unsupported schema. Reported in
review.
## Test plan
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/SchemaVersion.test.ts
Tests 25 passed (25)
```
With 48 pending updates instead of 40 (the fail-closed path) the case now
fails:
```
AssertionError: expected [ …(2) ] to deeply equal [ '@lexical/y/recovered-read', …(1) ]
```
(the warnings there are `@lexical/y/unknown-node-types` and
`@lexical/y/unsupported-schema`). `pnpm run ci-check` passes.
…ote the v1/v2 fixes merged and closed ## Description Two rows of the `@lexical/y` decision register did not match the code or the pull request history: - D72 said the binding's first read is not deferred until every extension registers. The binding starts in `YCollabExtension`'s `afterRegistration`, once every extension has registered (the `TableOnOpen` test comment #9537 corrects says the same). The row now says that, and that the decision is no further deferral or special-casing: an initialization-order bug is fixed in the extension that has it, as #9501 did. - D12 (no more fixes to `@lexical/yjs` v1 and v2) now notes that one v2 table fix, #9584, merged on 10-30 before the row was checked, and that later v1 and v2 fix pull requests, #9595 and #9596, were closed under it. ## Changeset None: documentation only. ## Test plan ### Before D72 contradicted `packages/lexical-y/src/internal/YExtension.ts`, which starts the binding in `afterRegistration`. ### After ``` $ pnpm run ci-check ... Changeset check passed: 31 pending changeset(s) valid. ... Found 0 errors ```
Description
An audit of the
@lexical/ytests after the stack merged, with a v8 coverage run ofpackages/lexical-yandpackages/lexical-mark(lines 94.8%, branches 90.4%).New tests in
SchemaVersion.test.ts, for the schema gate paths no test reached, and the read-only status the gate holds:schemaGatesWritedroppingschemaGateAfter).beforestep can gate, and only the recovery gates (therecoverpath of the remote read). Removing that gate makes this test fail with the binding still'active'.Comments that no longer matched the code:
TableOnOpen.test.tssaid the binding reads the document when it registers; it starts inYCollabExtension'safterRegistration, once every extension has registered.Every
test.failsstill fails onmain(46 expected failures in the full unit run: 42 in@lexical/y, 4 in@lexical/yjs). Besides the environment-gated perf and replay suites, twoCompositionHoldBackcases are skipped as a known limit that no longer happens; #9574 runs them.Changeset
None: tests only.
Test plan
Before
Coverage of
Binding.tsbefore:schemaGatesWrite's reset ofschemaGateAfter(line 1754) and the remote read's recovery gate (line 2530) never ran.After