Merge pull request #2352 from modelcontextprotocol/v2/docs/2347-asyncutiltimeout-pin-comments
docs: stop justifying the asyncUtilTimeout pin by pointing at #2335
This commit is contained in:
commit
7ec4eec018
3 changed files with 39 additions and 20 deletions
16
AGENTS.md
16
AGENTS.md
|
|
@ -435,12 +435,16 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race.
|
|||
tool can be asked to resolve; assert at runtime when it is about behavior.
|
||||
- **A budget is stated, which is not the same as raised.** Testing Library's
|
||||
`asyncUtilTimeout` is pinned at its own default in both web setup files
|
||||
because raising it was *measured worse*: a wait that is meant to expire —
|
||||
a test asserting something never appears, a poll allowed to run out — spends
|
||||
its whole budget on the happy path, so a raise is a proportional cost on
|
||||
exactly those tests and starves the worker pool. Measure before raising a
|
||||
budget that a passing test can spend in full; the call-site comment records
|
||||
the three-arm run.
|
||||
because raising it was *measured* and found to buy nothing: #2323's
|
||||
three-arm run went red at 5000, and #2335's interleaved re-run on a leased
|
||||
machine found no web test that lets a Testing Library wait expire on its
|
||||
passing path and no difference between 1000 and 5000 beyond the guard
|
||||
refusing the raised value. The hazard the rule guards against is still
|
||||
real: a wait that is meant to expire — a test asserting something never
|
||||
appears, a poll allowed to run out — spends its whole budget on the happy
|
||||
path, so a raise is a proportional cost on exactly those tests. Measure
|
||||
before raising a budget that a passing test can spend in full; the
|
||||
call-site comment records both runs.
|
||||
- **An inner budget must be strictly smaller than the budget enclosing it.**
|
||||
`waitFor({ timeout: N })` inside a test whose own budget is `N` can never win:
|
||||
the test expires first and reports a timeout naming neither the wait nor its
|
||||
|
|
|
|||
|
|
@ -22,11 +22,15 @@ import { getConfig } from "@testing-library/react";
|
|||
*
|
||||
* A different unrelated set failed each time and every one passed in about a
|
||||
* second in isolation, which is the shape of CPU starvation rather than of a
|
||||
* slow assertion. The mechanism: `asyncUtilTimeout` is not only a ceiling — any
|
||||
* wait that is *meant* to expire spends the whole budget on the happy path, so
|
||||
* a 5x raise is a 5x cost on exactly those tests, which saturates the worker
|
||||
* pool and starves tests that were never slow. The tests that assert absence
|
||||
* that way are the real defect and are #2335; revisit this when they are gone.
|
||||
* slow assertion. The mechanism proposed at the time — a wait that is *meant*
|
||||
* to expire spends the whole budget on the happy path, so a 5x raise is a 5x
|
||||
* cost on those tests — was checked on #2335 and does not hold here: no web
|
||||
* test lets a Testing Library wait expire on its passing path, and the same
|
||||
* three-arm protocol re-run interleaved on a leased machine showed no failure
|
||||
* in any arm beyond this very assertion refusing the raised value, and 0 tests
|
||||
* at or above 5000ms with the raise in place. Raising
|
||||
* the budget buys nothing, so the pin stays at the default until a measurement
|
||||
* says otherwise. `setup.ts` carries the full record.
|
||||
*
|
||||
* ⚠️ This asserts the EFFECTIVE value at runtime rather than checking that
|
||||
* `setup.ts` contains a `configure()` call. An earlier revision of #2334 did
|
||||
|
|
|
|||
|
|
@ -21,17 +21,28 @@ import { cleanup, configure } from "@testing-library/react";
|
|||
//
|
||||
// The failures were a different, unrelated set each time and every one passed
|
||||
// in about a second in isolation, which is the shape of CPU starvation rather
|
||||
// than of a slow assertion. The mechanism is that `asyncUtilTimeout` is not
|
||||
// only a ceiling: any wait that is *meant* to expire — a test asserting
|
||||
// something never appears, and anything that lets a poll run out — spends the
|
||||
// whole budget on the happy path, so a 5x raise is a 5x cost on those tests,
|
||||
// which saturates the worker pool and starves tests that were never slow.
|
||||
// than of a slow assertion. The mechanism proposed at the time was that
|
||||
// `asyncUtilTimeout` is not only a ceiling: a wait that is *meant* to expire —
|
||||
// a test asserting something never appears, a poll allowed to run out — spends
|
||||
// the whole budget on the happy path, so a 5x raise would be a 5x cost on
|
||||
// those tests, saturating the worker pool.
|
||||
//
|
||||
// #2335 went looking for those tests and found none. Zero hits for every
|
||||
// expiring-wait shape against Testing Library across the unit, `core/`,
|
||||
// integration and story files; no test at or above 2000ms in a full unit run;
|
||||
// and the same three-arm protocol re-run interleaved on a leased machine
|
||||
// (1000 / 5000 / reverted, three rounds) gave 0 failures in every arm — the
|
||||
// one red test in each 5000 arm being `asyncUtilTimeout.test.ts` refusing the
|
||||
// raised value, which is the guard doing its job — and 0 tests at or above
|
||||
// 5000ms with the raise in place. Nothing on the passing
|
||||
// path spends this budget, so raising it buys nothing — and the #2323 reds
|
||||
// read as ambient load the arms did not share (an inference from that data,
|
||||
// not a re-measurement of the original run).
|
||||
//
|
||||
// So the general rule this repo now follows — a budget must be a value someone
|
||||
// chose — is satisfied by choosing it, not by raising it. Raising it is what
|
||||
// #2323's own "do not raise" list would have said had the measurement been
|
||||
// taken first. The tests that assert absence by letting a wait expire are the
|
||||
// real defect and are tracked on #2335; when they are gone, revisit this.
|
||||
// chose — is satisfied by choosing it, not by raising it. The pin stays at the
|
||||
// default until a measurement says otherwise; `asyncUtilTimeout.test.ts`
|
||||
// asserts the effective value, so moving it is deliberate by construction.
|
||||
configure({ asyncUtilTimeout: 1000 });
|
||||
|
||||
// Node 22+ exposes an experimental `localStorage` placeholder that overrides
|
||||
|
|
|
|||
Loading…
Reference in a new issue