1596165 - Consider removing the initial wait from TestUtils.waitForCondition.

Status: RESOLVED FIXED

(Toolkit :: General, task, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

In https://phabricator.services.mozilla.com/D52833 (bug 1595285) I naively changed waitForCondition from doing:

setInterval(async function() { ... }, interval);

to:

async function tryOnce() {
  // ...
  setTimeout(tryOnce, interval);
}
tryOnce();

That happened to break a bunch of tests which weren't expecting the initial check to be synchronous, so I had to change the last line to:

setTimeout(tryOnce, interval);

We should consider removing that.

Comment 1

[Florian Quèze [:florian]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #0)

That happened to break a bunch of tests which weren't expecting the initial check to be synchronous,

I think it's reasonable for tests to expect this function to not be synchronous, but the initial 100ms wait is nasty as it makes tests implicitly wait for "stuff" to happen during these 100ms... and sometimes that stuff can take longer, and cause intermittent failures.

I'm proposing to change the last line to: TestUtils.executeSoon(tryOnce);. This should speed up the tests significantly, but avoid the synchronous call, and avoid the timer.

This still causes LOTS of test failures (most of them are existing intermittent failures that become perma fail), that I'll try to resolve in dependent bugs.

Comment 2

[Florian Quèze [:florian]]

Attachment: Bug 1596165 - Remove the initial timer in TestUtils.waitForCondition, r=Gijs.

Depends on D108010

Comment 3

[Florian Quèze [:florian]]

Attachment: Bug 1596165 - Avoid browser_aaa_eventTelemetry_run_first.js failures by putting back a 100ms timer before the waitForCondition call, r=Gijs.

Depends on D108019

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ae4c40c2d0f
Avoid browser_aaa_eventTelemetry_run_first.js failures by putting back a 100ms timer before the waitForCondition call, r=Gijs.

Comment 5

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/3ae4c40c2d0f

Comment 6

[Florian Quèze [:florian]]

Reopening, I forgot to put the leave-open keyword here before landing a first batch of test fixes.

Comment 7

[Florian Quèze [:florian]]

Attachment: Bug 1596165 - add back timers for tests that permafail without the waitForCondition initial timer, r=Gijs.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d8fb53c8fcb
add back timers for tests that permafail without the waitForCondition initial timer, r=Gijs.
https://hg.mozilla.org/integration/autoland/rev/4eccbb3d6549
Remove the initial timer in TestUtils.waitForCondition, r=Gijs.

Comment 9

[Cristian Brindusan [:cbrindusan]]

Backed out 2 changesets (bug 1596165) for bc failures in browser_bookmarkProperties_folderSelection.js and browser_doorhanger_submit_telemetry.js.
https://hg.mozilla.org/integration/autoland/rev/f616889f61563e180009ee1bcd8edc4a435032d4

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=4eccbb3d6549091c7763aef9b2d896072a50f5bd&selectedTaskRun=fBgRgidgQ1miQOdWMMCIyw.0

Failure log:
browser_doorhanger_submit_telemetry.js:
https://treeherder.mozilla.org/logviewer?job_id=334344122&repo=autoland&lineNumber=6881
browser_bookmarkProperties_folderSelection.js
https://treeherder.mozilla.org/logviewer?job_id=334348413&repo=autoland&lineNumber=2312

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21067aab87ee
add back timers for tests that permafail without the waitForCondition initial timer, r=Gijs.
https://hg.mozilla.org/integration/autoland/rev/8581dbde5b5b
Remove the initial timer in TestUtils.waitForCondition, r=Gijs.

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/21067aab87ee
https://hg.mozilla.org/mozilla-central/rev/8581dbde5b5b