Closed Bug 1379630 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Places, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

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.
(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 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+
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
https://hg.mozilla.org/mozilla-central/rev/e85a615528a2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.