Give BrowserTestUtils.waitForMutationCondition a default timeout and message
Categories
(Testing :: Mochitest, task)
Tracking
(firefox155 fixed)
| Tracking | Status | |
|---|---|---|
| firefox155 | --- | fixed |
People
(Reporter: dao, Assigned: dao)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
Bug 2062448 - Drop the AI Window tests' hand-rolled bounded mutation wait. r?#ai-frontend-reviewers!
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
Bug 2062448 - Watch for the tab strip leaving moving-tab mode instead of polling for it. r?nsharpley
48 bytes,
text/x-phabricator-request
|
Details | Review |
BrowserTestUtils.waitForMutationCondition (testing/mochitest/BrowserTestUtils/BrowserTestUtils.sys.mjs) attaches a MutationObserver and returns a promise that settles only when the condition passes. If the condition never becomes true, the promise never settles: the test hangs until the harness kills it, with nothing in the log pointing at which wait was stuck.
TestUtils.waitForCondition (testing/modules/TestUtils.sys.mjs) does not have that problem. It takes a msg, gives up after maxTries, and rejects with that message; it also rejects with "still pending at the end of the test" via TestUtils.promiseTestFinished, and uses the message to label its profiler markers.
That gap makes waitForCondition the safer choice even where a mutation-driven wait would be more precise. It came up in bug 2061837 for the final "is the tab strip overflowing" wait in BrowserTestUtils.overflowTabs, where the point of the patch was to replace an untimed wait with one that fails with an assertion.
Proposal - give waitForMutationCondition a bounded mode:
- an optional timeout, defaulting to today's untimed behavior so the existing call sites are unaffected (or a large enough default)
- an optional message used as the rejection text, and to label profiler markers the way
waitForConditiondoes - rejection when the test finishes while the wait is still pending, via
TestUtils.promiseTestFinished
The main design call is the API shape, since the second parameter is already taken by the MutationObserver.observe() options - likely a fourth options argument, e.g. { msg, timeout }.
|
Assignee | |
Updated•27 days ago
|
|
|
Assignee | |
Updated•26 days ago
|
|
|
Assignee | |
Comment 1•26 days ago
|
||
waitForMutationCondition settles only when its condition passes, so a condition
that never becomes true hangs the test until the harness kills it, with nothing
in the log naming the wait. That makes TestUtils.waitForCondition the safer
choice even where a mutation-driven wait would be more precise, and it has
already led tests to hand-roll bounded wrappers of their own.
Take the failure behavior from waitForCondition: a timeout, a message labelling
the wait, and - as waitForEvent already does - rejection if the test finishes
while the wait is still pending. The tab strip overflow wait can then watch the
attribute its check reads instead of polling, and still fail with a message
naming the tab count.
Because the timeout defaults on, most waits end up bounded without anyone having
written a message for them, so an absent message falls back to the call site plus
the source of checkFn. For the usual inline arrow that reads as the condition
itself, which is the thing worth naming, and it means a wait that fails is never
anonymous.
The timeout defaults to 10 seconds, well under the 45s browser-chrome task
timeout, so a stuck wait names itself instead of taking the rest of the task down
with it. Try runs at 60s, 30s, 20s and 10s across opt, debug, asan and tsan turned
up one wait that legitimately needs longer - the sidebar tools resize, opted out
at the top of this stack. Pass Infinity for a wait that genuinely cannot be
bounded; a finite 0 is a bound, not an opt-out, so that a miscomputed timeout
fails fast rather than hanging forever.
The other behavior change is the end-of-test rejection, which can only fire for a
promise that was never awaited, since awaiting an unsettled one would have hung
the test instead.
|
|
Assignee | |
Comment 2•26 days ago
|
||
waitForMutationBounded raced BrowserTestUtils.waitForMutationCondition against a
timer because the helper itself could not fail. It can now, so ask it for the
bound directly. Its sole caller keeps the same 15s budget and the same label in
the failure message.
Worth noting: when the race timed out it abandoned the underlying promise, so the
MutationObserver stayed connected for the rest of the test; the helper disconnects
on every exit path.
spawnBounded stays as it is - it bounds a SpecialPowers.spawn, and the content-side
ContentTaskUtils.waitForMutationCondition still has no timeout of its own.
|
|
Assignee | |
Comment 3•26 days ago
|
||
resetToolsHeight waits for the tools to stop overflowing after resizeTools has
shrunk them, and resizeTools walks the splitter down a frame at a time. Under
tsan that walk runs past a minute, so the bound waitForMutationCondition now
applies by default cut the wait short where it used to just take a while.
A longer bound is no help: measured against tsan the wait wants more than 60s and
the harness gives up on the test at 120s, so anything that fits between the two is
a guess that turns into an intermittent. Opt this wait out and let the harness be
the thing that gives up. The eight callers all go through this helper, so this is
the only place that needs it.
The underlying slowness is worth its own look - a resize this expensive to drive
is a lot of test runtime for what it checks - but that is a bigger change than
unblocking the default.
|
|
Assignee | |
Comment 4•26 days ago
|
||
The stale drag tests polled for the movingtab attribute because
waitForMutationCondition had no way to fail. It does now, so observe the
attribute the check reads: the wait ends on the mutation that clears it, and a
recovery that never happens fails with the message naming what was expected
rather than after fifty quiet retries.
The three waits collapse into one helper alongside assertNotMovingTab. The
reduced-motion wait stays a waitForCondition - gReduceMotion is a script global
with no mutation behind it.
|
||
Updated•26 days ago
|
Backed out for causing bc failures
|
||
Comment 9•25 days ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/34bbca735319
https://hg.mozilla.org/mozilla-central/rev/cd6bea08a948
https://hg.mozilla.org/mozilla-central/rev/fd12604f193a
|
|
Assignee | |
Updated•23 days ago
|
|
||
Comment 10•19 days ago
|
||
|
||
Comment 11•18 days ago
|
||
| bugherder | ||
Description
•