Closed Bug 1404946 Opened 3 years ago Closed 3 years ago

Rename wait.js module to sync.js

Categories

(Testing :: Marionette, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Attachment #8914365 - Attachment is obsolete: true
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.
Attachment #8914362 - Flags: review?(hskupin) → review-
Comment on attachment 8914363 [details]
Bug 1404946 - Rename wait.until to PollPromise.

https://reviewboard.mozilla.org/r/185630/#review191368
Attachment #8914363 - Flags: review?(hskupin) → 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.
Attachment #8914364 - Flags: review?(hskupin) → review+
Comment on attachment 8914366 [details]
Bug 1404946 - Have PollPromise accept an options dictionary.

https://reviewboard.mozilla.org/r/185636/#review191376
Attachment #8914366 - Flags: review?(hskupin) → review+
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.
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.
Comment on attachment 8914362 [details]
Bug 1404946 - Rename wait module to sync.

https://reviewboard.mozilla.org/r/185628/#review191836
Attachment #8914362 - Flags: review?(hskupin) → review+
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
Attachment #8914364 - Attachment is obsolete: true
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.
Flags: needinfo?(ato)
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
You need to log in before you can comment on or make changes to this bug.