test(lint-framework): prove lint request coalescing and settle the ignore-suggestion e2e test - #3913
Draft
rodbegbie wants to merge 5 commits into
Draft
Conversation
…light `requestLintUpdate` used `lintRequested` as a single-flight mutex, but a request arriving while one was in flight was discarded rather than queued. The default delay is 0, so there is no debounce to coalesce them either. A burst of input could therefore leave the rendered lints belonging to a stale prefix of the text, recovered only by the 1000ms safety-net timer. That staleness is invisible on screen, because `remapLintToCurrentSource` keeps the highlight correctly positioned. It is not invisible to the ignore path: `LintContext` hashes the tokens following a lint, so a lint computed against a prefix carries a `context_hash` that never matches the one derived from the final text. Dismissing such a lint records a hash that matches nothing, and the highlight comes straight back and stays. Track a `lintDirty` flag and re-run once after the in-flight pass releases the mutex. Two placement details matter. The re-run must happen after that release, or it hits the same guard and is dropped in turn. And it belongs inside the `finally`, so that a pass which threw still hands off the input that arrived while it was running -- otherwise a rejected lint strands exactly the work this change exists to preserve, and recovery falls back to the 1000ms timer. That `finally` also fixes a pre-existing bug: a rejected `lintProvider` previously left `lintRequested` stuck at true, permanently stopping all linting. Refs Automattic#3911 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Entire-Checkpoint: ffb36eab0913
`packages/lint-framework` had no test harness at all, so the scheduling fix in the parent commit had no automated proof and the only coverage was the Playwright suite -- a poor instrument for this, since the flaky test it addresses already passes roughly two runs in three. Add vitest following the `harper.js` and `obsidian-plugin` precedent: browser mode via `@vitest/browser-playwright`, headless chromium. Two tests drive a lint provider whose responses resolve on demand, so a lint can be held in flight deliberately: - requests dropped mid-flight produce exactly one follow-up lint, and it sees the final text - a rejected provider does not leave the framework permanently wedged Both fail against the pre-fix scheduler and pass with it. Every wait is bounded to ten animation frames. `LintFramework` polls itself every second to cover editors that fail to emit events, and an earlier draft of these tests waited long enough for that poll to rescue them -- passing against unfixed code. Staying well under a second is what makes a pass mean the framework re-linted deliberately. Wire the suite into `just test-lintframework` and the CI matrix; without that the package's tests would never run. Refs Automattic#3911 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Entire-Checkpoint: b35af98d196a
`testCanIgnoreSuggestion` acted on the first highlight to appear. That highlight can belong to a lint computed against a prefix of the typed text, because input events arriving mid-lint are coalesced into a follow-up pass. It looks identical on screen -- `remapLintToCurrentSource` keeps it correctly positioned -- but its context hash covers different trailing tokens. Ignoring it records a hash matching nothing, so the next pass returns the lint unfiltered and the highlight comes back and stays. That is the assertion which has been failing intermittently on Firefox. Wait for the follow-up pass once highlights first appear. The scheduling fix earlier in this branch is what makes waiting sufficient: before it, a dropped request was never re-run, so no amount of waiting converged. The interval clears `LintFramework`'s one-second self-poll, the slowest path by which a pass over the final text can arrive. Only this helper needs it -- it is the one whose assertion depends on context hashes agreeing across the action. Refs Automattic#3911 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Entire-Checkpoint: 797267b3e02e
Review of Automattic#3913 pointed out that the coalesced follow-up was skipped when the in-flight pass rejected -- the dirty check sat after the `try`/`finally`, so the exception propagated past it. That is fixed in the parent commit by moving the check inside the `finally`; this adds the test that pins it. The new case fails against the previous placement and passes with the fix, while the other two pass either way, so it isolates the error path precisely. Relax the existing rejection test to assert on order rather than an exact call count. Coalescing plus ambient page events -- the window listeners cover scroll, resize and selectionchange -- can legitimately add passes, so an exact count asserts something the framework never promised. It still fails against unfixed code. Also correct the `LINT_SETTLE_MS` comment. It justified the interval by the framework's one-second self-poll, which this wait cannot reliably cover: it starts at an arbitrary phase relative to that timer. The real justification is that the follow-up pass is event-driven and needs only a lint plus a render. Refs Automattic#3911 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Entire-Checkpoint: 94085980577d
rodbegbie
force-pushed
the
test/3911-ignore-suggestion-coverage
branch
from
July 29, 2026 05:11
73f23f4 to
d820fb4
Compare
… count Review of Automattic#3913 noted that the ten-animation-frame waits only stayed under `LintFramework`'s one-second self-poll because rAF happens to run fast. requestAnimationFrame is throttled when a page is backgrounded and stretches under load, so a fixed frame count can silently exceed the poll -- at which point the poll supplies the follow-up the test is looking for and a broken scheduler passes. A silent false pass is worse than a flake. Bound every wait by wall-clock time instead, and assert on *when* the follow-up arrived rather than only that it did. A lint produced by the poll is a second late by construction, so it cannot satisfy a 400ms budget however slowly the machine is running. The budget must stay below the poll interval for any of this to hold, so say so where the constant is defined -- raising it past a second is precisely what would restore the failure mode. Refs Automattic#3911 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Entire-Checkpoint: 2707dcf7ac8b
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.
Issues
Refs #3911.
Important
Stacked on #3912. This branch is based on that one, so the diff currently
shows its commit too. Only the top two commits belong to this PR:
test(lint-framework): cover lint request coalescing with vitesttest(chrome-ext): let linting settle before ignoring a suggestionThe vitest tests fail without #3912 — that is the point of them. Happy to
rebase once #3912 lands, or to squash the two together if you would rather.
Description
#3912 fixes the lint-scheduling race behind the flaky
Can ignore suggestionFirefox test but ships without automated proof, because
packages/lint-frameworkhas no test harness at all(
"test": "echo 'no tests'"). This adds one, and closes the remaining gap inthe e2e helper.
Vitest for
lint-framework, following theharper.jsandobsidian-pluginprecedent: browser mode via@vitest/browser-playwright,headless chromium. Two tests drive a lint provider whose responses resolve on
demand, so a lint can be held in flight deliberately:
the final text
Wired in as
just test-lintframeworkplus a CI matrix entry. Without that thepackage's tests would never run —
lint-frameworkwas not injust test.A settle wait in
testCanIgnoreSuggestion. It acted on the first highlightto appear, which can belong to a lint computed against a prefix of the typed
text. That looks identical on screen but carries a different context hash, so
ignoring it records a hash matching nothing. #3912 narrows that window but does
not close it for this test; waiting for the follow-up pass does. Note the
ordering dependency: before #3912 a dropped request was never re-run, so no
amount of waiting converged.
Demo
N/A — test-only change.
How Has This Been Tested?
Both new tests were confirmed to fail against
upstream/masterand pass with#3912 applied:
Worth recording, because it nearly went the other way: the first version of
these tests passed against unfixed code.
LintFrameworkpolls itself everysecond to cover editors that fail to emit events, and the original waits span
roughly 1.6 seconds of animation frames — long enough for that poll to supply
the "follow-up" lint the test was checking for. Every wait is now bounded to
ten animation frames, comfortably below the poll, so a pass means the framework
re-linted deliberately rather than being rescued.
A third test ("does not re-lint when nothing arrived during the pass") was
written and then deleted. It failed even against unfixed code, because
attachWindowListenerssubscribes toscroll,resizeandselectionchangeand ambient events legitimately trigger re-lints. No production change makes it
fail cleanly, so it was noise rather than coverage.
Also run:
just formatandpnpm -w run checkclean across 365 files;just test-lintframeworkgreen end to end.Noted but not fixed
update()callsrequestLintUpdate()without awaiting or catching it, so arejected lint surfaces as an unhandled promise rejection rather than reaching a
caller — in real browsers, not only under test. #3912 releases the mutex but
does not stop the rejection escaping. It is suppressed narrowly in the one test
that provokes it rather than widening #3912's scope. Happy to file it
separately if useful.
AI Disclosure
An agent wrote the patch and this description, interactively, with a human
approving the design, the scope and the final diff. Not autonomous.
If Your PR Implements or Enhances a Linter
N/A — this changes test infrastructure, not a linter rule.
Checklist