Skip to content

test(lint-framework): prove lint request coalescing and settle the ignore-suggestion e2e test - #3913

Draft
rodbegbie wants to merge 5 commits into
Automattic:masterfrom
rodbegbie:test/3911-ignore-suggestion-coverage
Draft

test(lint-framework): prove lint request coalescing and settle the ignore-suggestion e2e test#3913
rodbegbie wants to merge 5 commits into
Automattic:masterfrom
rodbegbie:test/3911-ignore-suggestion-coverage

Conversation

@rodbegbie

Copy link
Copy Markdown

Disclaimer: this patch and description were produced by a Claude Code
agent working interactively under @rodbegbie's direction, per the "Be Honest"
section of AGENT_POLICY.md. Every design decision was reviewed and approved
by a human.

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 vitest
  • test(chrome-ext): let linting settle before ignoring a suggestion

The 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 suggestion
Firefox test but ships without automated proof, because
packages/lint-framework has no test harness at all
("test": "echo 'no tests'"). This adds one, and closes the remaining gap in
the e2e helper.

Vitest for lint-framework, 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

Wired in as just test-lintframework plus a CI matrix entry. Without that the
package's tests would never run — lint-framework was not in just test.

A settle wait in testCanIgnoreSuggestion. It acted on the first highlight
to 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/master and pass with
#3912 applied:

RED   expected [ 'T' ]      to deeply equal [ 'T', 'This is a mistaek.' ]
      expected [ 'First.' ] to deeply equal [ 'First.', 'Second.' ]
GREEN 2 passed

Worth recording, because it nearly went the other way: the first version of
these tests passed against unfixed code
. LintFramework polls itself every
second 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
attachWindowListeners subscribes to scroll, resize and selectionchange
and ambient events legitimately trigger re-lints. No production change makes it
fail cleanly, so it was noise rather than coverage.

Also run: just format and pnpm -w run check clean across 365 files;
just test-lintframework green end to end.

Noted but not fixed

update() calls requestLintUpdate() without awaiting or catching it, so a
rejected 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

  • I am a human and didn't use any AI.
  • I used LLM features of my editor, but not an agent.
  • I used an AI agent interactively.
  • I am an agent or I got an agent to do the work autonomously.

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

  • I have performed a self-review of my own code
  • I have added tests to cover my changes
  • I have considered splitting this into smaller pull requests

rodbegbie and others added 4 commits July 28, 2026 22:08
…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
rodbegbie force-pushed the test/3911-ignore-suggestion-coverage branch from 73f23f4 to d820fb4 Compare July 29, 2026 05:11
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant