Skip to content

test: harden the suite against brittle, flaky, and shallow tests - #1367

Open
cody-dot-js wants to merge 6 commits into
mainfrom
improve-tests
Open

test: harden the suite against brittle, flaky, and shallow tests#1367
cody-dot-js wants to merge 6 commits into
mainfrom
improve-tests

Conversation

@cody-dot-js

Copy link
Copy Markdown
Collaborator

An adversarial audit of every test file in the repo, the fixes it produced, and the conventions that should
keep the problems from regrowing.

The method was to read each test file next to its implementation and ask two questions per test: what
one-line change to the implementation would make this fail
, and what change that is not a bug would
make this fail
. A second, skeptical pass then tried to refute each finding — it killed 48 of 219, which is
roughly the refutation rate you want from a review that isn't just agreeing with itself.

1,636 → 2,450 tests. All green, plus lint, fmt, typecheck, and the @ngrok/mantle build.

The honest headline

The suite was not especially flaky or brittle — it was thin, and thin in a way the test count hid.

Of 171 verified findings, only 1 was a genuine flake and 8 were brittleness. 131 were missing coverage. The
dominant problem: 40 of 84 component test files dispatched no user event at all, and that set included the
components whose entire value is interaction — Select was never opened, Tabs never clicked, SplitButton's menu
never opened, AlertDialog hardcoded open and never closed, DataTable's sort button never clicked, readOnly
had zero references anywhere in the repo. 1,636 green tests in 8 seconds was fast because most of the tests
were cheap.

The pure-logic layers are genuinely good and were left largely alone — the chart math, the code-block parsers,
and the ~3,000-case cx parity fixture are the house standard. The gap was everything between a pure function
and a rendered component.

Three real bugs, found by asking what no test covered

These are fixed here, each with a regression test that fails before the fix:

  • Open redirect. isSafeLocalPath rejected //evil.com but accepted /\evil.com. The WHATWG URL parser
    treats \ as a synonym for / under special schemes, so a browser resolves that to https://evil.com/.
    The predicate's negative cases covered //, absolute URLs and control characters — never a backslash.
  • Toast.Icon silently discarded a custom svg when intent="info". That one branch rendered InfoIcon
    unconditionally while all three sibling intents honored svg ?? …. The branches are now two module-scope
    maps keyed by ToastIntent, so the fallback can't be forgotten again without a type error.
  • MediaObject dropped almost every prop. All three parts destructured five props with no rest spread, so
    id, onClick, aria-* and data-* were accepted by ComponentProps<"div"> and thrown away.

Plus a duplicate "016" in countryCodes (739 entries, 738 distinct).

An order-dependent test, reproduced and fixed

code-block's "fires onCopyError when clipboard write fails" patched navigator.clipboard.writeText before
userEvent.setup() — which replaces navigator.clipboard wholesale. The patch survived only when an earlier
test had already installed userEvent's stub. It failed on 2 of 6 shuffled seeds.

The general fix is config-level: restoreMocks, unstubEnvs, unstubGlobals on both projects. The suite had
~30 bare spy.mockRestore() calls at the end of test bodies, every one of which leaks when the test throws
before reaching it — turning one failure into a cascade. Now no inline teardown is load-bearing.

Verified with 14 shuffled seeds across both file and test order.

Tests that could not fail

A sample of what the audit found, all now fixed:

Where Problem
use-breakpoint six of eight tests asserted properties of literals declared in the test body
use-debounced-callback neither test asserted a call count, so deleting the timer reset that is the debounce left both green
renderer the equal-area test re-declared the implementation's shape constants and compared its own arithmetic
sidebar the account-swatch hash test compared one id against itself
otp-input, data-table assertions whose selectors could never match
accordion.browser asserted offsetHeight === 0 for a reason unrelated to the line it existed to guard
field-context, mantle-style-sheets byte-for-byte duplicate tests

Also tightened 34 call-count-blind spies to toHaveBeenCalledTimes + toHaveBeenLastCalledWith, and 29
assertions whose only claim was toBeDefined() / toBeTruthy() / not.toThrow().

Tailwind class assertions

32 files asserted utility strings as a proxy for behavior. Neither project loads a stylesheet, so those compare
a string against the source literal — they verify nothing and false-fail on token renames. Worse, they pass
while the behavior is broken: code-block asserted overflow-x-auto/shrink-0 to guard "TabList scrolls
instead of wrapping", yet adding flex-wrap — the exact regression a previous PR fixed — satisfies every
assertion.

All 256 were adjudicated individually rather than swept:

  • 52 deleted, or replaced with a data attribute or a real behavioral assertion
  • 121 kept — they pin a consumer's className beating a default, which is the tailwind-merge contract and
    is worth testing
  • 83 kept with a comment naming what consumes the class (mostly Button's size/appearance variants, where
    the class genuinely is the only observable)

field.test.tsx alone went 52 → 14. The raw repo-wide count barely moved because the suite grew 50%; what
changed is that every remaining one is now either a real contract or annotated with why it exists.

Test placement

code-block-fold and mantle-style-sheets move out of the Playwright project — neither used a real-browser
API, so they were paying Chromium startup and browser-launch flake for assertions that are deterministic in
happy-dom.

New browser tests were added only where one is genuinely required, each injecting its load-bearing CSS inline
(browser tests load no Tailwind): canvas paint geometry, Element.moveBefore, real overflow observers.

New coverage for shared primitives

Untested component directories: 20 → 13. Deliberately not all 20 — the ones written are the primitives
other, already-tested components depend on:

dialog (including the primitive.tsx guards Dialog, Sheet and AlertDialog share), table's
ResizeObserver/MutationObserver overflow engine, dropdown-menu, separator, sandboxed-on-click,
media-object and flag. The remaining 13 are leaf presentation (badge, card, kbd, skeleton, …) and
warrant a few assertions each, tracked for later.

Conventions

CONVENTIONS.md § Testing, COMPONENT_SPEC.md §8, and the AGENTS.md diff-audit checklist now state the
rules this audit showed were missing:

  • A test must be able to fail — name the one-line change it catches. Bans toBeDefined() /
    not.toThrow() as a test's only assertion, tautologies, and unmatchable queries.
  • Drive the interaction — a component with an event handler, controlled prop, or keyboard contract needs a
    real event, not an assertion about its initial markup.
  • Assert behavior, not styling internals — with the two legitimate carve-outs spelled out.
  • Determinism — no arbitrary sleeps, spies after userEvent.setup(), no order dependence.
  • Pin cross-file contracts — when a selector in one file depends on an attribute emitted in another,
    assert both sides in one test.
  • Cover the server renderrenderToString for SSR branches; evaluate stringified inline scripts.

oxlint's vitest(expect-expect) already catches the assertion-free case; the rest is reviewer-enforced.

Not done, on purpose

  • 61 findings need an implementation change to become testable — nearly all "emit a documented data
    attribute so this variant is observable" (e.g. data-appearance on TextArea, data-size on Flag,
    data-slot on the Button spinner). Left for a follow-up rather than changing public API inside a test PR.
  • 17 newly found source bugs are recorded but unfixed, including: DataTable.EmptyRow computes colSpan
    from all columns instead of visible leaf columns; goToLastPage doesn't clamp to 1, so an empty list reports
    a negative offset; the date comparators return NaN for an Invalid Date, which Array#sort reads as
    "equal"; MultiSelect needs two ArrowDown presses from a focused tag; DataTable.Root clobbers
    data-slot="table" instead of joining it. Happy to file these as issues.
  • No mutation testing or coverage tool ran. Agents hand-verified ~40 mutants; the true count of surviving
    mutants is higher. Line coverage was deliberately skipped — it's the wrong instrument here, since a
    render() with zero assertions scores 100%.
  • One unreproduced observation: a shuffled full-suite run once reported an unhandled
    ReferenceError: window is not defined from an input-otp internal timer firing after teardown. Exit code
    0, no test failed, and it did not reproduce in 11 further attempts. Flagged rather than speculatively
    patched.

Verification

lint          0 errors
fmt:check     clean (727 files)
typecheck     7/7 tasks, 0 errors
build         @ngrok/mantle builds clean
mantle        144 files, 2450 tests passed
apps/www       18 files,  170 tests passed
shuffle       14 seeds green across file + test order
browser        4 consecutive full runs green

Copilot AI review requested due to automatic review settings July 25, 2026 20:02
@changeset-bot

changeset-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: dd78af5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ngrok/mantle Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@ngrok-ship

ngrok-ship Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

🚀 Deploy Previews

Updated 2026-08-05 17:26 UTC

App URL Commit Status
ngrok-mantle https://mantle-1367.ngrok-previews.ngrok.app dd78af5 🟢 Running

`isSafeLocalPath` rejected `//evil.com` but accepted `/\evil.com`. The WHATWG URL
parser treats `\` as a synonym for `/` under special schemes, so a browser resolves
`/\evil.com` against https://ngrok.com to https://evil.com/ — the protocol-relative
open redirect the predicate exists to block, spelled with a backslash.

A backslash is never legal in a URL path, so reject it outright. Found by an
adversarial audit of the test suite: the predicate's negative cases covered `//`,
absolute URLs and control characters, but never a backslash.
… props, dedupe country codes

Three defects an adversarial audit of the test suite surfaced by asking what no
test covered:

- `Toast.Icon` ignored a custom `svg` when `intent="info"`. That one branch
  rendered `InfoIcon` unconditionally while every sibling intent honored
  `svg ?? …`. The four per-intent branches are now two module-scope maps keyed by
  `ToastIntent`, so the fallback cannot be forgotten again without a type error.
- `MediaObject.Root`/`.Media`/`.Content` destructured five props with no rest
  spread, so `id`, `onClick`, `aria-*` and `data-*` were accepted by
  `ComponentProps<"div">` and then silently discarded. All three now spread.
- `countryCodes` listed "016" twice — 739 entries for 738 distinct codes.

Also hoist the default fallback elements in `Toast.Icon`, `OtpInput.Separator`,
`Breadcrumb.Separator`, `SplitButton.MenuTrigger`, `DataTable.ExpandHeader` and
`ThemeSwitcher.Content` to module scope, so each is one stable element reference
shared across renders instead of a fresh allocation every render.
…ng rules

Set `restoreMocks`, `unstubEnvs` and `unstubGlobals` on both Vitest projects. The
suite had ~30 bare `spy.mockRestore()` calls at the end of test bodies, every one
of which leaks when the test throws before reaching it — a failing test would then
cascade into unrelated files. Restoring centrally means no inline teardown is
load-bearing. `integer-ticks.browser.test.tsx` moves its `fillText` spy from
`beforeAll` to `beforeEach` to suit.

This fixed a real order-dependent failure: code-block's "fires onCopyError when
clipboard write fails" patched `navigator.clipboard.writeText` *before*
`userEvent.setup()`, which replaces `navigator.clipboard` wholesale — so the patch
survived only when an earlier test had already installed userEvent's stub. It
failed on 2 of 6 shuffled seeds. Now 14/14 seeds pass.

CONVENTIONS.md § Testing, COMPONENT_SPEC.md §8 and the AGENTS.md diff-audit
checklist now state the rules an adversarial audit of the suite showed were
missing: a test must be able to fail (name the one-line change it catches);
interactive contracts must be driven with a real event, not asserted from initial
markup; Tailwind utility-string assertions are not coverage, because neither
project loads a stylesheet; determinism rules (no arbitrary sleeps, spies after
`userEvent.setup()`, no order dependence); pin the data-attribute contracts that
cross files; and cover the server render with `renderToString`.
An adversarial audit read every test file against its implementation and asked two
questions per test: what one-line change would make this fail, and what change that
is *not* a bug would make this fail. 171 findings survived a skeptical verification
pass that refuted 48 more. This is the fix sweep. 1,636 tests -> 2,450.

Determinism
- Replaced 13 of 14 fixed `setTimeout` sleeps with waits on the observable state
  (`waitFor` / `findBy*` / `expect.poll`). The remaining one is documented: it waits
  out a body-style restore that has no implementation seam to observe.
- Removed the inline `mockRestore()` calls that the new config-level teardown makes
  dead code.
- Verified with 14 shuffled seeds over both file and test order.

Tests that could not fail
- `use-breakpoint`: six of eight tests asserted properties of literals declared in
  the test body. `use-debounced-callback`: neither test asserted a call count, so
  deleting the timer reset that *is* the debounce left both green. `renderer`'s
  equal-area test re-declared the implementation's shape constants and compared its
  own arithmetic. `sidebar`'s account-swatch hash compared one id against itself.
  `otp-input` and `data-table` had assertions whose selectors could never match.
  `accordion.browser` asserted `offsetHeight === 0` for a reason unrelated to the
  line it existed to guard. Byte-for-byte duplicate tests in `field-context` and
  `mantle-style-sheets` are gone.
- Tightened 34 call-count-blind spies to `toHaveBeenCalledTimes` +
  `toHaveBeenLastCalledWith`, and 29 assertions whose only claim was
  `toBeDefined()`, `toBeTruthy()` or `not.toThrow()`.

Interaction
- 40 of 84 component test files dispatched no user event at all, including the
  components whose entire value is interaction. Select is now opened, Tabs triggers
  clicked, the DataTable sort cycle driven, SplitButton's menu opened, AlertDialog
  actually closed, `readOnly` guards on Switch and Checkbox exercised, and
  Toast.Action's dismiss-and-preventDefault contract covered.

Styling internals
- 32 files asserted Tailwind utility strings as a proxy for behavior; neither project
  loads a stylesheet, so those compare a string to the source literal. Each of 256
  was adjudicated: 52 deleted or replaced with a data attribute or a real behavioral
  assertion, 121 kept because they pin a consumer's `className` beating a default
  (the tailwind-merge contract), 83 kept with a comment naming what consumes the
  class. `field.test.tsx` alone went from 52 to 14.

Test placement
- `code-block-fold` and `mantle-style-sheets` move out of the Playwright project;
  neither used a real-browser API. New browser tests were added only where one is
  genuinely required (canvas paint geometry, `Element.moveBefore`, real overflow
  observers), each injecting its load-bearing CSS inline.

New coverage for shared primitives
- First tests for `dialog` (including the `primitive.tsx` guards Dialog, Sheet and
  AlertDialog share), `table`'s overflow observer, `dropdown-menu`, `separator`,
  `sandboxed-on-click`, `media-object` and `flag`. Untested component directories:
  20 -> 13. The remaining 13 are leaf presentation, tracked for later.
- Regression tests for the three defects fixed in the preceding commits.

Deferred: 61 findings whose fix needs an implementation change (mostly a documented
data attribute so a variant becomes observable), and 17 newly found source bugs,
are recorded in the PR description rather than fixed here.
…bad intent

Two follow-ups to 5755fcf, both found by mutation-testing that commit's own
tests.

MediaObject's parts spread `{...props}` after their literal `data-slot`, so the
rest spread that fixed the dropped-props bug handed an incoming `data-slot` the
last word. Under `<Slot>` the attribute vanished outright — mantle's `Slot`
always writes the key, and `undefined` is omitted by React — so consumer CSS on
`[data-slot="media-object"]` silently stopped matching with all 23 tests green.
All three parts now accept `WithDataSlot` and merge with `joinDataSlot`, like
centered-layout, main, and theme-switcher already do.

`Toast.Icon`'s replacement guard looked up `defaultIcons[ctx.intent]` and
checked the result for nullish. `defaultIcons` is an object literal, so an
out-of-union `intent` of "toString" or "constructor" from untyped JS resolves an
inherited function, skips the guard, and dies inside `SvgOnly` with a misleading
message. `Object.hasOwn` restores the fail-fast the exhaustive `switch` gave.

Both are covered by regression tests, and the toast intent→tone table is back:
permuting `iconColors` typechecks (`satisfies` only checks keys) and left the
whole file green. So did permuting `defaultIcons`, so the default glyph per
intent is now pinned by its rendered path geometry.
…config

Mutation-testing the previous two commits found tests that could not fail:

- `preferredWidth` could be deleted from `Dialog.Content` with all 69 dialog
  tests green — every assertion for it was negative, and the one positive class
  was removed by the fixture's own conflicting `className`.
- text-area's font-merge test declared `.font-sans` after `.font-mono`, so the
  caller's font won by cascade whether or not tailwind-merge deduped. Both
  classes shipping still passed.
- bar-chart waited on `canvas.width > 0`, which a `<canvas>` satisfies before
  the engine sizes anything (300x150 intrinsic backing store).
- line-chart's `toHaveBeenCalledExactlyOnceWith` sat inside `vi.waitFor`, which
  resolves the moment the callback stops throwing — an async duplicate call
  could never be seen.
- skip-to-main-link asserted only `location.hash`, which reads identically for
  `replaceState`, `pushState`, and `location.hash = …`.
- table's "the tolerance stays sub-pixel" probed a 20px gap, green for any
  tolerance under 20.
- split-button lost the `MenuTriggerProps omits size` `@ts-expect-error`;
  `DataTable.ExpandHeader`'s default label was never asserted `sr-only`;
  `useInView`'s root test read `intersectionOptions?.root`, which is `undefined`
  both when the root is right and when no observer was constructed at all.
- changelog's fenced-block fixture indented its fences, so `inFence` — which
  gates on `line.startsWith("```")` — was never entered. Both the real
  (indented) and column-0 shapes now have a fixture.
- multi-select's last `setTimeout(resolve, 50)` is gone; ariakit defers the
  stale body-style restore with `queueMicrotask`, so awaiting frames cannot
  race it.

Determinism moves into config, where a single-file or editor-driven run still
gets it: `TZ`/`LC_ALL` for the happy-dom project (8 tests failed under
`TZ=Asia/Tokyo LC_ALL=de_DE.UTF-8` before — vitest merges `test.env` into the
worker at spawn, so ICU initializes pinned), Playwright `locale`/`timezoneId`
for Chromium, which `process.env` cannot reach, and the three mock-hygiene flags
in every test-bearing package — `apps/www`, `mantle-vite-plugins`, and
`mantle-server-syntax-highlighter` had none, while CONVENTIONS.md claimed
otherwise and two live spies depended on an inline `mockRestore()`.

CONVENTIONS.md also no longer claims `restoreMocks` clears a `vi.fn()`'s call
history; it restores `vi.spyOn` descriptors only, so a shared `vi.fn()` still
needs `mockReset()`.
cody-dot-js added a commit that referenced this pull request Jul 29, 2026
#1381)

Lands the conventions from #1367 ahead of the test changes themselves, so
the rules are in force for new work while that PR is still in review.

CONVENTIONS.md § Testing gains the rules the audit behind #1367 showed were
missing: a test must be able to fail (name the one-line change it catches),
drive the interaction with a real event, assert behavior rather than Tailwind
utility strings, pin contracts that cross files, cover the server render, and
determinism. COMPONENT_SPEC.md §8 and the AGENTS.md diff-audit checklist point
at the same bar.

The Vitest configs come along because CONVENTIONS.md § Determinism now states
that every test-bearing package sets `restoreMocks`, `unstubEnvs`, and
`unstubGlobals`, and that an inline `mockRestore()` is therefore dead code.
Without the configs that instruction is false, and it would tell a reader to
delete teardown that is still load-bearing. All five packages the bullet names
now set the flags. `packages/mantle` also pins TZ/locale in the config rather
than only in its `test` script, so the pin survives a single-file run.

`restoreMocks` restores spies *before each test*, which breaks a spy installed
once in `beforeAll` — `integer-ticks.browser.test.tsx` recorded canvas
`fillText` calls that way, so its per-test spy moves to `beforeEach`. That is
the only test change here; it is required to keep the suite green, not new
coverage.

COMPONENT_SPEC.md's scope-and-status counts are restated against this branch
(20 of 67 component directories ship no test file), not against #1367's
post-audit numbers.
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.

2 participants