Repository navigation
[lexical-y] Chore: Add the tests of the parked keep-ends pull request, with main's failures as expected failures - #9631
Merged
Merged
Conversation
…, with main's failures as expected failures ## Description #9617 (keep each key's run end in `Cursor#collapseGap`, for #9609) is parked: every version of it changed what other code does with the kept item, and a few randomized sessions came out wrong where main is right (see its Parked section). Its tests move to main here, as #9605 did for #9562: - `regressions/CollapseKeepsEnds.test.ts`: #9609 with a real collapse (12 cases, `test.fails` on main: the collapse deletes the comment's end), and controls that pass on main (a kept end must leave the value after the run; bold and style). - `CollapseKeepsEndsProperty.test.ts`: random sessions compared with the same session without any collapse; taking turns passes on main, the concurrent case is `test.fails` (#9609). - `regressions/KeepEndsSessions.test.ts`: four reduced randomized sessions. 2725 and 1861 (restored text takes no comment it never had) pass on main and failed with #9617's first rule; 4694 and 3645 (a comment main's collapse drops from text no delete touched) are `test.fails`. - `regressions/KeepEndsUndoPlainWord.test.ts` and `regressions/TypedTextAfterDeletedSpace.test.ts`: sequential and concurrent sessions where a kept end gave text a comment it never had; both pass on main and guard any later design. ## Test plan ### Before The files do not exist on main. ### After `pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/TypedTextAfterDeletedSpace.test.ts packages/lexical-y/src/__tests__/unit/regressions/CollapseKeepsEnds.test.ts packages/lexical-y/src/__tests__/unit/CollapseKeepsEndsProperty.test.ts packages/lexical-y/src/__tests__/unit/regressions/KeepEndsSessions.test.ts packages/lexical-y/src/__tests__/unit/regressions/KeepEndsUndoPlainWord.test.ts` on main (1853707): ``` ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/TypedTextAfterDeletedSpace.test.ts (1 test) ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/KeepEndsUndoPlainWord.test.ts (2 tests) ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/KeepEndsSessions.test.ts (4 tests) ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/CollapseKeepsEnds.test.ts (18 tests) ✓ |unit| packages/lexical-y/src/__tests__/unit/CollapseKeepsEndsProperty.test.ts (4 tests) Test Files 5 passed (5) Tests 13 passed | 16 expected fail (29) ``` `npx tsc -p tsconfig.test.json --noEmit` and eslint pass.
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
October 30, 2026 23:50
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
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: |
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks good to me: moving #9617's cases onto main, with main's failures as test.fails, keeps #9609 and the cases its variants broke pinned whatever design lands next. The expected failures fail on main at their final assertions (the comment spreading onto cc and dd, c2 missing from wk75, c9 missing in 3645), and each plain guard fails against at least one of #9617's variants, so they discriminate.
A few optional, non-blocking suggestions:
- The concurrent property rests on one seed (
packages/lexical-y/src/__tests__/unit/CollapseKeepsEndsProperty.test.ts:301). Of seeds 1-60 only seed 53 fails on main, in both client orders (the next is 69). As onetest.fails, the other 59 concurrent sessions per order do not guard main: a regression in any of them stays green, and a change that alters seed 53 alone turns CI red with just "Expect test to fail". Consider a plaintestover the seeds minus a known-failing list, plus atest.failsfor seed 53 that names #9609. - Name the fix on 4694 and 3645 (
packages/lexical-y/src/__tests__/unit/regressions/KeepEndsSessions.test.ts:330and:342).packages/lexical-y/AGENTS.md(Testing) asks that a known failure be atest.failsnaming the issue or pull request that fixes it. 3645 matters most: it differs from the no-collapse document only after its last step,['undo', 2](line 134), and applying #9630'sBinding.tschange makes it report "Expect test to fail" with the collapse unchanged. Naming #9617 and #9630 there, and rewording "now" in the comment at lines 340-341, tells whoever lands #9630 why it flips. - Check self-check warnings in the property test (
CollapseKeepsEndsProperty.test.ts:274).runcollectserrorsbut notwarnings, so a binding self-check mismatch would not fail it, although the harness turns the self-check on so that a mismatch fails tests and the other property suites assertwarningsis empty. No warnings occur on main in these sessions, so it costs nothing. - Nit: comments that read as main's behavior (
regressions/CollapseKeepsEnds.test.ts:45-47, thedescribetitle at line 116, andCollapseKeepsEndsProperty.test.ts:51). "It keeps the end now" and "Before the collapse kept ends" describe #9617; on main the collapse still deletes the end, which is why those cases aretest.fails.
No changeset is needed for a tests-only change (AGENTS.md, Changesets).
| } | ||
| }, 600_000); | ||
|
|
||
| test.fails( |
There was a problem hiding this comment.
Optional: of seeds 1-60 only seed 53 fails on main (both client orders; the next failing seed is 69), so this test.fails rests on that one session and the other 59 per order guard nothing. A plain test over the seeds minus a known-failing list, plus a test.fails for seed 53 that names #9609, would keep them as a live guard.
| const fresh = await freshReader(t.a); | ||
| return { | ||
| clocks: [t.a, t.b].map(p => p.doc.store.getClock(p.doc.clientID)), | ||
| errors: [...t.a.errors, ...t.b.errors, ...fresh.errors], |
There was a problem hiding this comment.
Optional: the peers' warnings (where the binding's self-check reports a mismatch) are not collected, so a self-check failure would not fail this property, unlike expectConverged. Adding the three peers' warnings costs nothing: none occur on main in these sessions.
| // Main's collapse dropped c9 from "wxq4xq56abcd" (3645), where a document | ||
| // read without any collapse keeps it. (With every undo press changing what | ||
| // the document shows, the session's last undo now also removes c9.) | ||
| test.fails( |
There was a problem hiding this comment.
Optional: packages/lexical-y/AGENTS.md (Testing) asks that a known failure name the issue or pull request that fixes it, and neither this case nor 4694 above does. This one differs from the no-collapse document only after the final ['undo', 2] (line 134), and #9630's undo-press change alone makes it report "Expect test to fail"; naming #9617 and #9630 here would explain the flip when either lands.
| // comments' value, nor the run's last items, and for the collaborator they | ||
| // bracket deleted content only. The collapse used to delete both, and once | ||
| // merged the restore had no end: the comment covered every later word and | ||
| // line. It keeps the end now. |
… exact ## Description Review of the test-only change found two expected failures that a real behaviour change could slip past: - The concurrent property test was one `test.fails` over 60 seeds. Since seed 53 fails on main, a change that broke a seed passing on main kept the whole test failing, so nothing noticed. Now one regular test runs every seed and checks everything (no errors, no more writes, the same characters, peers and a fresh reader agreeing); it skips the own-text comparison only on the seeds where main lets a comment spread (53). A `test.fails` per such seed checks only that comparison (#9609). - KeepEndsSessions 3645 shows main's drop of c9 only after the session's last undo. It also flips with #9630 (every undo press changes what the document shows), with no collapse change. Its name and comment now say so. ## Test plan ### Before `CollapseKeepsEndsProperty.test.ts` on main: 2 passed, 2 expected fail; one aggregate `test.fails` per id order. ### After `pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/CollapseKeepsEndsProperty.test.ts packages/lexical-y/src/__tests__/unit/regressions/KeepEndsSessions.test.ts` on main (1853707): ``` ✓ taking turns: every character shows as with no collapse (ids 1,2) ✓ typing ‖ deletes: the document's own text shows as with no collapse (ids 1,2) ✓ taking turns: every character shows as with no collapse (ids 2,1) ✓ typing ‖ deletes: the document's own text shows as with no collapse (ids 2,1) Tests 6 passed | 4 expected fail (10) ``` Seed 53 is the only one of 60 that fails on main, in both id orders, and only on the own text (its other checks pass). `npx tsc -p tsconfig.test.json --noEmit` and eslint pass.
There was a problem hiding this comment.
This looks good. The new @lexical/y tests record main's known failures as test.fails, and each one fails at its final assertion: for #9609, the comment runs past its end; for sessions 4694 and 3645, a comment drops from text no delete touched. The 15 regular tests pass on main and guard whatever collapse design comes next.
Optional, non-blocking suggestions:
- Name the tracking issue in the 4694 expected failure (
packages/lexical-y/src/__tests__/unit/regressions/KeepEndsSessions.test.ts:331).packages/lexical-y/AGENTS.md(Testing) asks that "a known failure is atest.failsthat names the issue or pull request that fixes it". The bare(4694)is a session seed, but on GitHub it reads like issue #4694. The comment drop in 4694 and 3645 is a different symptom from the #9609 spread, and no issue orDECISIONS.mdrow tracks it yet. Opening one (or naming #9617), writingseed 4694as the property test writesseed 53, and adding #9617 and #9631 to D65's links would keep it findable when a fix lands. - Reword two comments that describe the parked design as current behaviour:
regressions/CollapseKeepsEnds.test.ts:45-47("It keeps the end now.") andCollapseKeepsEndsProperty.test.ts:51-53("Before the collapse kept ends, …"). On main the collapse still deletes the end, which is why those cases aretest.fails. Something like "#9617 (parked) kept the end; main still deletes it" would say that.
No changeset is needed: AGENTS.md exempts test-only changes.
| // stays where a document read without any collapse has it: main's | ||
| // collapse dropped c2 from "wk75" (4694). | ||
| test.fails( | ||
| 'a comment stays on text that deletes around it never touched (4694)', |
There was a problem hiding this comment.
Optional: packages/lexical-y/AGENTS.md (Testing) asks that a known failure be a test.fails that names the issue or pull request that fixes it. This title names neither, and the bare (4694) (a session seed) reads like issue #4694 on GitHub. Consider seed 4694 plus the issue that tracks the comment drop, or #9617 if that stays the fix vehicle.
| // comments' value, nor the run's last items, and for the collaborator they | ||
| // bracket deleted content only. The collapse used to delete both, and once | ||
| // merged the restore had no end: the comment covered every later word and | ||
| // line. It keeps the end now. |
There was a problem hiding this comment.
Optional: "It keeps the end now." describes #9617, which is parked. On main the collapse still deletes the end, which is why the cases below are test.fails. Something like "#9617 (parked) kept the end; main still deletes it" would avoid confusion. CollapseKeepsEndsProperty.test.ts:51-53 has the same wording.
zurfyx
added a commit
that referenced
this pull request
Oct 31, 2026
## Description Merges main at f59f74d (#9631, #9642 and #9641). There were no conflicts. ## Test plan ### Before The branch was on main 2e2acb7. ### After ``` $ pnpm exec vitest run --project unit --project scripts-unit Tests 30437 passed | 282 expected fail | 135 skipped (30854) $ MATRIX_FULL=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 9062 passed | 206 expected fail (9268) $ pnpm run ci-check Found 0 errors ```
This branch was successfully deployed
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
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.
Description
#9617 (keep each key's run end in
Cursor#collapseGap, for #9609) is parked: every version of it changed what other code does with the kept item, and a few randomized sessions came out wrong where main is right (see its Parked section). Its tests move to main here, as #9605 did for #9562:regressions/CollapseKeepsEnds.test.ts: Bug: [lexical-y] A comment's restore inside a word its own client commented runs on over the rest of the document when a collaborator's delete drops the comment's end #9609 with a real collapse (12 cases,test.failson main: the collapse deletes the comment's end), and controls that pass on main (a kept end must leave the value after the run; bold and style).CollapseKeepsEndsProperty.test.ts: random sessions compared with the same session without any collapse. Taking turns passes on main. Typing and deleting at the same time: one regular test checks every seed (no errors, no more writes, the same characters, peers and a fresh reader agreeing) and the own text on every seed except 53, the only one of 60 where main's collapse lets a comment spread; atest.failsper order checks seed 53's own text (Bug: [lexical-y] A comment's restore inside a word its own client commented runs on over the rest of the document when a collaborator's delete drops the comment's end #9609).regressions/KeepEndsSessions.test.ts: four reduced randomized sessions. 2725 and 1861 (restored text takes no comment it never had) pass on main and failed with [lexical-y][lexical-website] Bug Fix: Keep where each format and comment run ends when a delete collapses deleted text's format items #9617's first rule; 4694 and 3645 (a comment main's collapse drops from text no delete touched) aretest.fails. 3645 shows the drop only after the session's last undo, so it also flips with [lexical-y][lexical-website] Bug Fix: Undo behaviour change: every undo or redo press changes what the document shows, or empties its stack #9630 (every undo press changes what the document shows), with no collapse change; its name says so.regressions/KeepEndsUndoPlainWord.test.tsandregressions/TypedTextAfterDeletedSpace.test.ts: sequential and concurrent sessions where a kept end gave text a comment it never had; both pass on main and guard any later design.Test plan
Before
The files do not exist on main.
After
pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/TypedTextAfterDeletedSpace.test.ts packages/lexical-y/src/__tests__/unit/regressions/CollapseKeepsEnds.test.ts packages/lexical-y/src/__tests__/unit/CollapseKeepsEndsProperty.test.ts packages/lexical-y/src/__tests__/unit/regressions/KeepEndsSessions.test.ts packages/lexical-y/src/__tests__/unit/regressions/KeepEndsUndoPlainWord.test.tson main (1853707):npx tsc -p tsconfig.test.json --noEmitand eslint pass.