Closed Bug 1458501 Opened 2 years ago Closed 2 years ago

Rewrite some places tests to use add_task & async functionality rather than do_test_pending/finished

Categories

(Toolkit :: Places, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

I was just modifying a test for another bug (test_424958-json-quoted-folders.js) and decided to rewrite it slightly to remove unnecessary boilerplate.

At the same time, I looked for "do_test_pending" and realised we only have 5 tests remaining for that, so I rewrote those as well to use add_task etc.

Landing these separately to avoid unnecessarily inflating the other bug.
Comment on attachment 8972533 [details]
Bug 1458501 - Rewrite tests to use newer structures and async APIs.

https://reviewboard.mozilla.org/r/241124/#review247184

::: toolkit/components/places/tests/bookmarks/test_424958-json-quoted-folders.js:15
(Diff revision 1)
> -    var query = PlacesUtils.history.getNewQuery();
> +  var query = PlacesUtils.history.getNewQuery();
> -    query.setFolders([PlacesUtils.bookmarks.toolbarFolder], 1);
> +  query.setFolders([PlacesUtils.bookmarks.toolbarFolder], 1);
> -    var result = PlacesUtils.history.executeQuery(query, PlacesUtils.history.getNewQueryOptions());
> +  var result = PlacesUtils.history.executeQuery(query, PlacesUtils.history.getNewQueryOptions());
>  
> -    var toolbar = result.root;
> +  var toolbar = result.root;
> -    toolbar.containerOpen = true;
> +  toolbar.containerOpen = true;

while here, all of this could use getFolderContents

::: toolkit/components/places/tests/expiration/test_notifications.js:31
(Diff revision 1)
> -  do_timeout(2000, check_result);
> -  do_test_pending();
> +  await new Promise(resolve => {
> +    do_timeout(2000, () => {
> -}
> -
> -function check_result() {
> -  Services.obs.removeObserver(gObserver, PlacesUtils.TOPIC_EXPIRATION_FINISHED);
> +      Services.obs.removeObserver(gObserver, PlacesUtils.TOPIC_EXPIRATION_FINISHED);

I think we can remove the observer after the promise, and that should simplify the promise code.
Invoking the observer more than once would indicate a bug, iirc.
Attachment #8972533 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd7d5c81670c
Rewrite tests to use newer structures and async APIs. r=mak
https://hg.mozilla.org/mozilla-central/rev/fd7d5c81670c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.