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

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P2)

Description

[Mark Banner (:standard8)]

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 1

[Mark Banner (:standard8)]

Attachment: Bug 1379630 - Update more places xpcshell-tests in tests/unit to use the new async bookmark APIs.

Review commit: https://reviewboard.mozilla.org/r/155716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155716/

Comment 2

[Marco Bonardo [:mak]]

(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

[Marco Bonardo [:mak]]

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?

Comment 4

[Mark Banner (:standard8)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155716/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e85a615528a2