Convert some more xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Places
P2
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
I've been finishing off some of the ones I initially started for bug 1376234 but didn't have time to debug. In addition, I've found a few more tests to transition to the new API as well.

This gets the vast majority of the tests/unit files transitioned across to the new async APIs. There's one or two remaining for various issues, and there's a few calls for the old batch mode which doesn't appear to have a replacement just yet.

In any case, time to get these test changes landed so we can start testing the new API more.
Comment hidden (mozreview-request)

Comment 2

7 months ago
(In reply to Mark Banner (:standard8) from comment #0)
> There's one or two remaining for various issues, and
> there's a few calls for the old batch mode which doesn't appear to have a
> replacement just yet.

yeah, batching is something I'm trying to avoid in the new API, a better replacement is insertTree, and bug 1340498 may provide an alternative by passing an array of notifications to the consumers when related operations happen at once, then the consumer can take its own decisions based on the size and contents of that array.

Comment 3

7 months ago
mozreview-review
Comment on attachment 8884828 [details]
Bug 1379630 - Update more places xpcshell-tests in tests/unit to use the new async bookmark APIs.

https://reviewboard.mozilla.org/r/155716/#review161150

::: toolkit/components/places/tests/unit/test_412132.js:116
(Diff revision 1)
> -      do_throw("Nonexistent bookmark should throw.");
> -    } catch (ex) {}
> -  }
> -
> -  // First try a straight-up bogus item ID, one greater than the current max
>    // ID.

the comment doesn't apply, since there's no concept of a "greater" guid.

::: toolkit/components/places/tests/unit/test_asyncExecuteLegacyQueries.js:76
(Diff revision 1)
> -  if (tests.length == 0) {
> -    do_test_finished();
> -    return;
> -  }
>  
> -  Promise.all([
> +async function cleanupTest() {

doesn't need to be async.

::: toolkit/components/places/tests/unit/test_history_sidebar.js:79
(Diff revision 1)
>    function(aContainer) { return aContainer.visible });
>  
>  /**
>   * Asynchronous task that fills history and checks containers' labels.
>   */
> -async function task_fill_history() {
> +add_task(async function task_fill_history() {

I'm not a great fan of mixing up add_task and add_test in the same test file, I suspect the order may not be future-proof.
Can we just use one?
Attachment #8884828 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 5

7 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e85a615528a2
Update more places xpcshell-tests in tests/unit to use the new async bookmark APIs. r=mak

Comment 6

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e85a615528a2
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.