Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

testing/marionette/wait.js originally contained a utility for poll-waiting on a condition.  It has since been expanded to also include TimedPromise which is a specialisation of Promise that is rejected after a duration.

The latter is not a wait utility but a synchronisation primitive.  This terminology also covers the first.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8914362 [details]
Bug 1404946 - Rename wait module to sync.

https://reviewboard.mozilla.org/r/185628/#review191366

It's great to see tests added for that module. But you only did that for the `wait.until` method. What about `timedPromise`?

::: testing/marionette/test_sync.js:23
(Diff revision 1)
> +  await wait.until((resolve, reject) => {
> +    ++nevals;
> +    reject();
> +  });
> +  let end = new Date().getTime();
> +  greaterOrEqual((end - start), 2000);

Mind having this default timeout defined globally? Also why do we have different timeouts for `until`, and `timedPromise`?

::: testing/marionette/test_sync.js:24
(Diff revision 1)
> +    ++nevals;
> +    reject();
> +  });
> +  let end = new Date().getTime();
> +  greaterOrEqual((end - start), 2000);
> +  greaterOrEqual(nevals, 15);

The 15 here is just a random number? With an interval of 10ms we should have around 200 evaluations. So maybe 150 is a better value here, which also gives some room in regards of inreliablity of the execution.

::: testing/marionette/test_sync.js:33
(Diff revision 1)
> +  let nevals = 0;
> +  let err;
> +  try {
> +    await wait.until(() => {
> +      ++nevals;
> +      throw new Error();

Error is the basic type. Maybe it's better to throw a more generic one, so we can ensure it keeps the same type.

::: testing/marionette/test_sync.js:73
(Diff revision 1)
> +  let nevals = 0;
> +  await wait.until((resolve, reject) => {
> +    ++nevals;
> +    reject();
> +  }, 100, 100);
> +  equal(2, nevals);

Shouldn't that be at maximum 2? In case there is a delay after the first iteration the timeout has already been hit, and no further iteration should be executed. I feel that could become a race condition here.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8914363 [details]
Bug 1404946 - Rename wait.until to PollPromise.

https://reviewboard.mozilla.org/r/185630/#review191368

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8914364 [details]
Bug 1404946 - Rename wait.until to PollPromise.

https://reviewboard.mozilla.org/r/185632/#review191370

That change makes sense given that this method returns a promise.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8914366 [details]
Bug 1404946 - Have PollPromise accept an options dictionary.

https://reviewboard.mozilla.org/r/185636/#review191376

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 13 • 7 years ago
mozreview-review-reply

Comment on attachment 8914362 [details]
Bug 1404946 - Rename wait module to sync.

https://reviewboard.mozilla.org/r/185628/#review191366

I didn’t write new tests for wait.until. test_wait.js was just
renamed to test_sync.js.  TimedPromise doesn’t have any tests,
which can probably be considered a bug.

> Mind having this default timeout defined globally? Also why do we have different timeouts for `until`, and `timedPromise`?

Added a DEFAULT_TIMEOUT constant.

> Also why do we have different timeouts for until, and
> timedPromise?

We use a default that makes sense in the cases where TimedPromise is
being used.  That timeout might be different to that used for the
poll-waiting promises.

> The 15 here is just a random number? With an interval of 10ms we should have around 200 evaluations. So maybe 150 is a better value here, which also gives some room in regards of inreliablity of the execution.

I remember lowering this number a long time ago due to intermittents
on debug builds, so I’d rather keep it artificially low.  The
reason for 15 is that is’t longer than 10.

> Error is the basic type. Maybe it's better to throw a more generic one, so we can ensure it keeps the same type.

I don’t think it matters so much.  We just want anything that throws.

> Shouldn't that be at maximum 2? In case there is a delay after the first iteration the timeout has already been hit, and no further iteration should be executed. I feel that could become a race condition here.

In practice it has never caused a problem.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 18 • 7 years ago
mozreview-review-reply

Comment on attachment 8914362 [details]
Bug 1404946 - Rename wait module to sync.

https://reviewboard.mozilla.org/r/185628/#review191366

So I blame the UI of reviewboard here. It shouldn't have shown any content in case no modifications have been made. Mind filing a bug for the TimedPromise? I think that would be great for a mentored bug.

> I remember lowering this number a long time ago due to intermittents
> on debug builds, so I’d rather keep it artificially low.  The
> reason for 15 is that is’t longer than 10.

I see.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8914362 [details]
Bug 1404946 - Rename wait module to sync.

https://reviewboard.mozilla.org/r/185628/#review191836

	Pulsebot
	Comment 24 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/231a24060d29
Rename wait module to sync. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/135c1e44a92b
Add markup to wait.until's docs. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/bf79d5b1b4b0
Rename wait.until to PollPromise. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/dfc766f28aec
Have PollPromise accept an options dictionary. r=whimboo

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 25 • 7 years ago

Backed out for linting failure at testing/marionette/driver.js:3054:35 | 'f' is defined but never used:

https://hg.mozilla.org/integration/autoland/rev/7aec94f0c517cd3758c2cc3bee25136a37133c02
https://hg.mozilla.org/integration/autoland/rev/ff635ffa3ab790e0369ba447a3419d731a5ef53b
https://hg.mozilla.org/integration/autoland/rev/35beb8d02dcc9069d97cbc6530cfe7344763fd60
https://hg.mozilla.org/integration/autoland/rev/567eafaa5efe59677570a1b30c157d0398105b3c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dfc766f28aecf342e383c749af8b408f18e77a0c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135706527&repo=autoland

 TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/marionette/driver.js:3054:35 | 'f' is defined but never used. (no-unused-vars)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/marionette/sync.js:85:10 | 'PollPromise' is defined but never used. (no-unused-vars)

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 31 • 7 years ago

This was backed out because another patch with stricter linting
rules landed in the mean time since this changeset was written.
I have now rebased the patch and somewhat improved the commit
messages.

Doing a try run to be sure before sending off for autolanding.

	Pulsebot
	Comment 32 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88012e76b4af
Rename wait module to sync. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/3665cb253cb8
Rename wait.until to PollPromise. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/66694cc53d51
Have PollPromise accept an options dictionary. r=whimboo

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 33 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/88012e76b4af
https://hg.mozilla.org/mozilla-central/rev/3665cb253cb8
https://hg.mozilla.org/mozilla-central/rev/66694cc53d51