Repository navigation
[lexical-mdast][lexical-y] Chore: Give slow tests a timeout that holds under parallel load, and measure the follow-up check's cost against a smaller document - #9576
Open
zurfyx wants to merge 3 commits into
Conversation
…s under parallel load, and measure the follow-up check's cost against a smaller document ## Description Four tests take 2 to 4 seconds alone against vitest's default 5 second limit, so they time out when the whole suite runs in parallel: `DisposeMemory`, `ServerScale`'s linear-time test, both `MdastTextRuns` tests and the first `extensionFlowSubpaths` test (which starts the TypeScript checker). Their times and measured values are the same on `main`, on `main` of a day earlier (0cd7448) and on the first `@lexical/y` commit, so the timeouts hide no slowdown and no leak. They now have a 30 second limit, as `Headless`'s synchronous loop test already does. `FollowUpCheckCost` asserted that the follow-up check costs under 5 ms per follow-up in a 10,000-block document. Alone it measures 2.1 to 2.7 ms, and it measured 5.06 ms with type checks running beside it. The property is that the check reads the part of the document a follow-up changed, so the test now compares, in the same process, the check in 10,000 blocks with the check in 1,000 blocks: reading the whole document would cost about ten times as much. A check that reads whole containers fails it (1,160 ms against a bound of 547 ms). ## Test plan ### Before ``` $ vitest --run --project unit --maxWorkers=16 packages/lexical-y # on main, next to other work FAIL |unit| packages/lexical-y/src/__tests__/unit/DisposeMemory.test.ts > 40 editors bound, edited and disposed in a synchronous loop hold no document Error: Test timed out in 5000ms. Tests 1 failed | 7482 passed | 46 expected fail | 4 skipped (7533) ``` Test durations alone on `main`, 10 runs each (median / max, against the 5,000 ms default): `DisposeMemory` 2,881 / 3,738 ms; `ServerScale` full-state updates 2,746 / 3,247 ms; `MdastTextRuns` per-run nesting 2,181 / 3,274 ms; `extensionFlowSubpaths` first test 2,297 / 2,686 ms. `FollowUpCheckCost`'s check alone: 2.07 to 2.65 ms per follow-up at the end, 1.47 to 1.74 ms in the middle, against its 5 ms bound. ### After ``` $ vitest --run --project unit --maxWorkers=12 packages/lexical-y packages/lexical-mdast Test Files 173 passed | 2 skipped (175) Tests 7768 passed | 46 expected fail | 4 skipped (7818) $ vitest --run --project scripts-unit scripts/__tests__/unit/extensionFlowSubpaths.test.ts Tests 32 passed (32) $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/FollowUpCheckCost.test.ts FOLLOWUP_COST end: 60 follow-ups checked; median follow-up 23.45 ms without the check, 24.91 ms with it; the check 2.187 ms per follow-up, 0.746 ms in 1000 blocks FOLLOWUP_COST middle: 60 follow-ups checked; median follow-up 23.68 ms without the check, 25.82 ms with it; the check 1.457 ms per follow-up, 0.726 ms in 1000 blocks Tests 2 passed (2) $ # with YBinding#$checkFollowUp changed to read every container whole FOLLOWUP_COST end: 60 follow-ups checked; median follow-up 28.82 ms without the check, 1168.80 ms with it; the check 1159.723 ms per follow-up, 109.382 ms in 1000 blocks FOLLOWUP_COST middle: 60 follow-ups checked; median follow-up 22.02 ms without the check, 1152.41 ms with it; the check 1157.358 ms per follow-up, 108.534 ms in 1000 blocks AssertionError: expected 1159.7230917000006 to be less than 547.4100489999983 AssertionError: expected 1157.3581318499996 to be less than 543.1716224166692 Tests 2 failed (2) $ pnpm run ci-check Changeset check passed: 31 pending changeset(s) valid. Found 0 errors (exit 0) ```
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
October 30, 2026 11:46
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
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: |
![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 tracking these down. The 30 s limits look right: describe(..., {timeout: 30_000}, ...) reaches both MdastTextRuns tests, the numeric third argument covers DisposeMemory and ServerScale, the assertions behind them are unchanged, and this follows the existing precedent in Headless.test.ts.
FollowUpCheckCost: the new bound is tighter than the one it replaces (packages/lexical-y/src/__tests__/unit/FollowUpCheckCost.test.ts:155)
5 * small.checkMs + 0.5is below the old 5 ms whenever the 1,000-block check costs under 0.9 ms, which is the usual case: 0.48 and 0.77 ms in this PR's two CI unit jobs, 0.75 ms in the run in the description. It only loosens when load also slows the 1,000-block run.- In this PR's Node 24 unit job the end test printed
the check 2.855 ms per follow-up, 0.481 ms in 1000 blocks, so it passed against 2.905 ms by 0.05 ms; the old bound had 2.1 ms to spare there. - The comment at lines 34–36 says the check "costs about the same in a document ten times longer", but it still grows with the follow-up's position:
followUpRangesfinds the unit withunits.indexOf(unit)(packages/lexical-y/src/internal/Binding.ts:6371), and$followUpDifferencewalks the container's items fromcs.y._startinformatsAt(packages/lexical-y/src/internal/SelfCheck.ts:857-873). The 10,000/1,000 ratio was 1.9× in the Node 22 job and 5.9× in the Node 24 job, while a check reading whole containers, the regression this guards, comes out at about 10.6× in the description's own run, so a factor of 5 sits close to the passing side. off,onandsmallrun one after another andcheckMsis a mean (lines 133 and 143–145), so a slowdown that hits only the 10,000-block run lands on one side: in the Node 24 job that run's median follow-up was 22.08 ms with the check against 10.79 ms without it in the run before.
Suggested fix: build both editors first, run a few warm-up keystrokes, then alternate keystrokes between the two documents and compare per-check medians, like the warm-up in HeadlessScale.test.ts and the median comparison in RepeatedIdReads.test.ts. Both sides then see the same load and JIT state, so a burst or a cold first check no longer lands on one of them. Please also update the comment to say the check walks the container up to the follow-up.
Minor: AGENTS.md asks the Test plan to paste real output rather than describe it; the 5.06 ms FollowUpCheckCost failure and the other three timeouts appear only as prose under Before.
| expect(small.stored).toBe(`renamed-${KEYS - 1}`); | ||
| // The check reads a few blocks, not the 10,000: ten times the blocks | ||
| // would cost about ten times as much. | ||
| expect(on.checkMs).toBeLessThan(5 * small.checkMs + 0.5); |
There was a problem hiding this comment.
This sits below the old < 5 whenever small.checkMs is under 0.9 ms, which is the usual case, and in this PR's Node 24 CI job it passed by 0.05 ms (2.855 against 5 * 0.481 + 0.5 = 2.905). Measuring the two documents interleaved after a warm-up and comparing per-check medians would give the factor headroom on both sides; see the review comment.
Member
Author
There was a problem hiding this comment.
Done in 0a8b7e9: both documents are built first, then after 10 warm-up keystrokes each keystroke goes to the same block of both, in turns, and the per-check medians are compared (3 * small + 0.1 ms). Measuring at the same block keeps the position-dependent walk equal on both sides, so a correct check measures 1.07 to 1.35 times the smaller document (20 runs alone and under load), while a check that reads every container whole (followUpRanges returning null) measures 10.7 times and fails. Numbers are in the description.
Comment on lines
+34
to
+36
| // document. So the check costs about the same in a document ten times | ||
| // longer; measured in the same process, the comparison holds on a loaded | ||
| // machine, where an absolute bound in milliseconds does not. |
There was a problem hiding this comment.
The check isn't size-independent: followUpRanges uses units.indexOf(unit) (internal/Binding.ts:6371) and formatsAt walks the container's items from cs.y._start (internal/SelfCheck.ts:857-873), so its cost grows with the follow-up's position (1.9× on Node 22 and 5.9× on Node 24 in this PR's CI). Worth saying so here, since the bound's factor depends on it.
Member
Author
There was a problem hiding this comment.
Done in 0a8b7e9: the comment now says the check walks the container up to the follow-up (followUpRanges uses units.indexOf, formatsAt walks from the container start), so its cost grows with the follow-up position, which is why both documents are measured at the same block.
…oth documents, in turns, by medians
## Description
`FollowUpCheckCost` compared the follow-up check's mean cost at the end (or middle) of a 10,000-block document with its cost at the end of a 1,000-block one, each measured in its own run, against `5 * small + 0.5`. That bound was tighter than the old 5 ms whenever the 1,000-block check took under 0.9 ms (the usual case), and it could not have headroom on both sides: the check walks the container up to the follow-up (`followUpRanges` finds its unit with `units.indexOf`, and `formatsAt` walks the container's items from its start), so its cost grows with the follow-up's position, by 1.9 to 5.9 times between those two positions in CI, close to the about 10 times a check reading whole containers costs.
Now both documents are built first, then, after 10 warm-up keystrokes, each keystroke (and its naming follow-up) goes to the same block of both, in turns: the last block of the 1,000-block document, and its middle. The per-check medians are compared: the 10,000-block one must stay under `3 * small + 0.1` ms. A correct check measures 1.07 to 1.35 times the smaller document's (only the rest of the document differs); one that reads whole containers measures about 10.7 times. The comment says what the check walks and why its cost grows with position.
## Test plan
```
pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/FollowUpCheckCost.test.ts
```
### Before
This PR's first commit, in its Node 24 CI unit job (bound `5 * 0.481 + 0.5 = 2.905` ms):
```
the check 2.855 ms per follow-up, 0.481 ms in 1000 blocks
```
The four slow tests alone (on this branch, with the same bodies as on main):
```
✓ unit packages/lexical-y/src/__tests__/unit/DisposeMemory.test.ts (1 test) 1648ms
✓ 40 editors bound, edited and disposed in a synchronous loop hold no document 1647ms
✓ full-state updates resolve in linear time 1743ms
✓ never round-trips worse than per-run nesting, for any format mix 1363ms
✓ declares all exports of @lexical/extension 1487ms
```
### After
```
FOLLOWUP_COST end: median check 0.812 ms in 10000 blocks, 0.623 ms in 1000 blocks (1.30x)
FOLLOWUP_COST middle: median check 0.692 ms in 10000 blocks, 0.591 ms in 1000 blocks (1.17x)
Tests 2 passed (2)
```
With `followUpRanges` changed to return null (every container read whole):
```
FOLLOWUP_COST end: median check 743.910 ms in 10000 blocks, 69.480 ms in 1000 blocks (10.71x)
FOLLOWUP_COST middle: median check 738.341 ms in 10000 blocks, 68.840 ms in 1000 blocks (10.73x)
AssertionError: expected 743.9102489999932 to be less than 208.54115800000872
AssertionError: expected 738.3414029999985 to be less than 206.62141899996786
Tests 2 failed (2)
```
10 runs alone and 10 next to a 9-worker `packages/lexical-y` run, on one 2-worker vitest each:
```
this commit, alone: 10/10 pass; ratio 1.07-1.33x (bound 3x + 0.1 ms)
this commit, under load: 10/10 pass; ratio 1.07-1.35x
first commit, alone: 10/10 pass; smallest margin to 5 * small + 0.5: 2.02 ms
first commit, under load: 10/10 pass; smallest margin: 2.28 ms
main, alone: 10/10 pass; check 0.825-1.475 ms against 5 ms
main, under load: 10/10 pass; check 0.832-1.579 ms
```
(The machine has many cores, so that load was mild: it did not reproduce the CI job's slow 10,000-block run.)
There was a problem hiding this comment.
Thanks, this looks good to me. The 30 s limits reach the tests they name (Vitest applies the describe-level {timeout: 30_000} to both MdastTextRuns tests), the assertions behind them are unchanged, and the warmed-up, interleaved median comparison in FollowUpCheckCost still fails a check that reads whole containers, at about 10.6x as the description reports.
One optional, non-blocking suggestion:
FollowUpCheckCostno longer guards cost that grows with the follow-up's position (packages/lexical-y/src/__tests__/unit/FollowUpCheckCost.test.ts:110-159). Both documents are typed at the same block, so a cost proportional to the position cancels out ofb < 3 * m + 0.1. For example, starting every range at unit 0 infollowUpRanges(found.push([0, j])inpackages/lexical-y/src/internal/Binding.ts) makes the check read every block before the follow-up: about 72 ms per follow-up at block 999 instead of under 1 ms, and both tests still pass at 1.00x, while the previous version of this test failed it (about 763 ms against 5 ms at block 9,999). A generous absolute ceiling for a third interleaved series at the last block of the 10,000-block document (say 50 ms, against the 2 to 3 ms the description measures there) would keep that coverage without bringing back the flakiness.
| expect(on.checkMs).toBeLessThan(5); | ||
| // About the same in both; a check reading the whole container would | ||
| // cost about ten times as much in the larger document. | ||
| expect(b).toBeLessThan(3 * m + 0.1); |
There was a problem hiding this comment.
Optional, non-blocking: because both documents are typed at the same block, any cost that grows with the follow-up's position is equal on both sides and cancels out here. Starting every range at unit 0 in followUpRanges (found.push([0, j])) makes the check cost about 72 ms per follow-up at block 999 instead of under 1 ms, and this assertion still passes at 1.00x; the previous absolute bound at block 9,999 failed it at about 763 ms. A generous ceiling for a series typed at the larger document's last block (say 50 ms, against the 2 to 3 ms the description measures there) would keep that coverage without the flakiness.
…ument's last block, against an absolute ceiling
## Description
Comparing the follow-up check at the same block of a 10,000- and a 1,000-block document cancels any cost that grows with the follow-up's position: a check that starts every range it reads at the container's first unit costs about 70 ms per follow-up at block 999 of both documents, and the ratio stays about 1. The test now also types at the last block of the 10,000-block document, in the same turns after the warm-up, and requires the median check there to stay under 20 ms (it measures about 1 ms). Each series renames its own block (`renamed-k`, `end-k`), so the block-id repair never sees two blocks with one id.
## Test plan
```
pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/FollowUpCheckCost.test.ts
```
### Before
With `followUpRanges` changed to start every range at unit 0, the first commit's test passes:
```
FOLLOWUP_COST end: median check 68.201 ms in 10000 blocks, 67.919 ms in 1000 blocks (1.00x)
FOLLOWUP_COST middle: median check 34.854 ms in 10000 blocks, 34.061 ms in 1000 blocks (1.02x)
```
This commit's test, with the same change:
```
FOLLOWUP_COST end: median check 68.201 ms in 10000 blocks, 67.919 ms in 1000 blocks (1.00x); 742.212 ms at block 9999
FOLLOWUP_COST middle: median check 34.854 ms in 10000 blocks, 34.061 ms in 1000 blocks (1.02x); 759.364 ms at block 9999
AssertionError: expected 742.2124700000022 to be less than 20
AssertionError: expected 759.3642030000046 to be less than 20
Tests 2 failed (2)
```
### After
Three runs:
```
FOLLOWUP_COST end: median check 0.799 ms in 10000 blocks, 0.604 ms in 1000 blocks (1.32x); 1.004 ms at block 9999
FOLLOWUP_COST middle: median check 0.694 ms in 10000 blocks, 0.587 ms in 1000 blocks (1.18x); 0.896 ms at block 9999
Tests 2 passed (2)
FOLLOWUP_COST end: median check 0.772 ms in 10000 blocks, 0.558 ms in 1000 blocks (1.38x); 0.952 ms at block 9999
FOLLOWUP_COST middle: median check 0.615 ms in 10000 blocks, 0.544 ms in 1000 blocks (1.13x); 0.921 ms at block 9999
Tests 2 passed (2)
FOLLOWUP_COST end: median check 0.820 ms in 10000 blocks, 0.631 ms in 1000 blocks (1.30x); 1.001 ms at block 9999
FOLLOWUP_COST middle: median check 0.733 ms in 10000 blocks, 0.622 ms in 1000 blocks (1.18x); 1.224 ms at block 9999
Tests 2 passed (2)
```
`pnpm run tsc`, `tsc-test`, `flow` (`Found 0 errors`), `prettier` (no files listed), `check-changesets` and `lint` pass, run one at a time.
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
Four tests take 2 to 4 seconds alone against vitest's default 5 second limit, so they time out when the whole suite runs in parallel:
DisposeMemory,ServerScale's linear-time test, bothMdastTextRunstests and the firstextensionFlowSubpathstest (which starts the TypeScript checker). Their times and measured values are the same onmain, onmainof a day earlier (0cd7448) and on the first@lexical/ycommit, so the timeouts hide no slowdown and no leak. They now have a 30 second limit, asHeadless's synchronous loop test already does.FollowUpCheckCostasserted that the follow-up check costs under 5 ms per follow-up in a 10,000-block document. Alone it measures 2.1 to 2.7 ms, and it measured 5.06 ms with type checks running beside it. The property is that the check reads the part of the document a follow-up changed, after walking the container up to it (followUpRangesfinds the follow-up's unit withunits.indexOf, andformatsAtwalks the container's items from its start), so its cost grows with the follow-up's position, not with the rest of the document. The test now builds a 10,000- and a 1,000-block document, and after 10 warm-up keystrokes types into the same block of both in turns (the last block of the smaller one, and its middle), comparing the per-check medians: the larger document's must stay under3 * small + 0.1ms. A correct check measures 1.07 to 1.35 times the smaller document's; a check that reads whole containers measures about 10.7 times, and fails.Test plan
Before
Test durations alone on
main, 10 runs each (median / max, against the 5,000 ms default):DisposeMemory2,881 / 3,738 ms;ServerScalefull-state updates 2,746 / 3,247 ms;MdastTextRunsper-run nesting 2,181 / 3,274 ms;extensionFlowSubpathsfirst test 2,297 / 2,686 ms.FollowUpCheckCost's check alone: 2.07 to 2.65 ms per follow-up at the end, 1.47 to 1.74 ms in the middle, against its 5 ms bound.The first commit's
FollowUpCheckCost, in this PR's Node 24 CI unit job (its bound was5 * 0.481 + 0.5 = 2.905ms):After