You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
test(web): resolve paged-list rows by cell text, not accessible name (#557) (#636)
* test(web): resolve paged-list rows by cell text, not accessible name (#557)
The paged-list tests intermittently failed CI with "Test timed out in
5000ms" — twice on ExpensesPage (#563, #557) and once on CustomersPage,
each time on a branch that touched no web/ source.
It is not the async race the original report guessed. `ByRole("row",
{ name })` makes Testing Library compute the accessible name of EVERY row
in the document — a recursive walk of each row's subtree — and `findBy*`
re-runs the whole query on every DOM mutation until it matches. A test
that mounts a page-size fixture (100 rows, the length a screen's pager
needs before it offers "load more") and then re-renders it through a
paged load spends seconds walking the tree.
Resolve the row from one cell's text instead: a single indexed text query
plus `closest("tr")`. Same row, same assertions, a fraction of the work.
- src/test/rows.ts: findRowByCellText / getRowByCellText, promoted from
the local helper CustomersPage had already hand-rolled one test below
the one that kept failing.
- src/test/pagedRowLookups.test.ts: a lint that fails when a page-size
fixture and a row-role-with-name query appear in the same test, so the
next 100-row site does not reintroduce the flake. Outside a page-size
fixture the role query is the right spelling and is left alone.
- The seven tests that combined both, across CustomersPage, ExpensesPage
and InventoryPage.
Measured locally, per test: ExpensesPage 'load more' 549ms -> 177ms;
CustomersPage create-refresh 616ms -> 197ms, load-more 366ms -> 105ms,
failed-extension 349ms -> 158ms.
Under 14 busy-loop processes on a 16-core box, at the unchanged 5000ms
default timeout, running the four paged-list files repeatedly:
before, 2 of 3 runs failed with a timeout (5267ms, 6129ms), both on the
test #557 reports; after, 0 of 5.
Full web suite: 2058 passed, coverage gate green, typecheck clean.
The global testTimeout is unchanged.
Refs #557, #563, #511
* test(web): name it.each blocks correctly in the paged-row lint (#557)
Review findings on #636.
The lint took a block's first quoted string as its test title. For
`it.each([...])("template", ...)` that string is the first CASE in the
table, not the title — so the one time the lint actually fired on an
`it.each` it would have named the wrong test. Verified against the
`it.each` in CustomersPage: the old logic derived the title "name" (the
first tuple's first element) where the real title is "uses a different
idempotency key when changed %s is retried after the write fails".
Forcing the new branch off reproduces the wrong title; with it on, the
right one comes out.
Also from review:
- Note in rows.ts that the `td` selector does not see a
`<th scope="row">` first column, and that the failure then reads as
"text not present" rather than "wrong selector".
- Return the row as HTMLTableRowElement, which is what closest("tr")
can actually yield.
The reviewer's other point — that the page-size detection is a literal
`100` scan and goes blind behind a named constant or a shared fixture
factory — is real and already disclosed in the file's own comment. It
needs a follow-up issue, not a wider regex that would still be a text
scan.
* test(web): close two detection gaps in the paged-row lint (#557)
CodeRabbit review of #636. Both findings verified against the regexes
before acting; neither pattern occurs in the repo today, so the lint's
current result is unchanged (still exactly the same 7 sites when run
against the pre-fix route tests).
Row-query matcher — was option-order sensitive. It required `name` to be
the FIRST key, so `{ exact: true, name: /…/ }` and `{ hidden: true,
name: /…/ }` walked past a guard whose entire job is to catch them, and
those queries cost exactly as much as the spelling it did catch. `name`
is now matched anywhere in the options object, and both quote styles are
accepted (`'row'` was missed too, which the review did not mention).
`[^}]*` cannot run past the object's own closing brace, so a `name:`
belonging to a later expression is not picked up. This was the finding
that mattered: a false negative in a guard is the silent direction.
Page-size matcher — `\(\s*100\s*\)` also matched `toHaveLength(100)` and
`advanceTimersByTime(100)`, an assertion and a clock nudge, neither of
which can be a fixture. Both are now excluded by name. The review's own
example, `waitFor(…, 100)`, never matched: the pattern needs 100 alone
inside the parens.
Left broad on purpose beyond those two: narrowing to a list of known
fixture-builder names would trade a self-announcing false positive for
a silent false negative, and go quiet the day someone adds a builder
under a new name.
* test(web): match shorthand { name } in the paged-row lint (#557)
Round-3 review of #636. One of the two findings applied; the loop stops
here.
Applied — the row matcher required `name:` with a colon, so the shorthand
`{ name }` slipped through. Not hypothetical: six call sites in this
suite already pass an accessible name that way, for `button`, `dialog`
and `option`, so a row query written in the house style would have gone
undetected by a lint whose whole purpose is to detect it. A quoted
`{ "name": … }` key is accepted for the same cost. `{ nameish: 1 }` is
still not matched.
Not applied — the objection that `(?<!Length)(?<!Time)` also suppresses
`getLength(100)` and `setTime(100)`. True, and there are zero such calls
in the repo. Excluding two complete identifiers instead would be a wider
pattern bought with no defect behind it.
Unchanged behaviour, verified: run against main's pre-fix versions of the
three route test files, the lint still reports exactly the same 7 sites.
Stopping the review loop here deliberately. Three rounds have confirmed
zero defects in what this PR actually fixes — the flake, proven by the
2-of-3 to 0-of-5 load test in the first commit. Every finding since has
been a gap in the lint's own regexes, and regexes always have another
gap. The remaining limits are documented in the file and tracked in #637.
* test(web): stop the paged-row lint matching a quoted "name" value (#557)
Round-4 review of #636, and a regression I introduced in round 3.
Allowing a quoted `{ "name": … }` key meant the closing quote of a VALUE
satisfied the key's optional quote, so `getByRole("row", { description:
"name" })` matched. Dropping the quote support removes the false positive
and costs a key spelling this codebase never uses — zero occurrences.
Subtractive on purpose. The pattern gets narrower, not wider.
Not adding the regression test the review asked for: a test asserting
which strings a lint's regex matches is a control for a control, and the
behaviour it would pin is the lint's precision, not the product's. The
cases are recorded in the comment above the pattern instead.
Unchanged behaviour, verified: against main's pre-fix versions of the
three route test files, the lint still reports exactly the same 7 sites.
Final change on this branch.
0 commit comments