1459998 - Stop using sync helpers in addon manager tests

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Andrew Swan [:aswan]]

Helpers used in addons manager tests including {startup,shutdown,restart}Manager, writeInstallRDFToDir, etc were originally synchronous.  They have been rewritten to be properly asynchronous with awaitPromise() wrappers so every user didn't have to be rewritten at the time, but now its time to go back and convert existing users and remove the sync wrappers.

Comment 1

[Kris Maglione [:kmag]]

I've been thinking about just doing a scripted rewrite for this.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1459998: Part 1 - Rewrite tests not to use async helpers.

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

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1459998: Part 2 - Cleanup cruft.

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

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8974168 [details]
Bug 1459998: Part 1 - Rewrite tests not to use async helpers.

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

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8974169 [details]
Bug 1459998: Part 2 - Cleanup cruft.

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

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8974168 [details]
Bug 1459998: Part 1 - Rewrite tests not to use async helpers.

https://reviewboard.mozilla.org/r/242452/#review248398

thanks!

::: toolkit/mozapps/extensions/test/xpcshell/test_backgroundupdate.js:14
(Diff revision 2)
>  
>  var testserver = AddonTestUtils.createHttpServer({hosts: ["example.com"]});
>  const profileDir = gProfD.clone();
>  profileDir.append("extensions");
>  
> -function run_test() {
> +add_task(async function setup() {

if you just move the call to do_test_pending before the call to promiseStartupManager(), you can leave out add_task()

Its functionally equivalent but simpler to read I think.

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update.js:45
(Diff revision 2)
>    let unpacked_addon = profileDir.clone();
>    unpacked_addon.append(IGNORE_ID);
>    do_get_file("data/test_delay_update_ignore/bootstrap.js")
>      .copyTo(unpacked_addon, "bootstrap.js");

ugh

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update.js:195
(Diff revision 2)
>    Assert.ok(addon_upgraded.isCompatible);
>    Assert.ok(!addon_upgraded.appDisabled);
>    Assert.ok(addon_upgraded.isActive);
>    Assert.equal(addon_upgraded.type, "extension");
>  
> -  await shutdownManager();
> +  await await promiseShutdownManager();

i think once is enough.  there are several other instances of this below as well.

::: toolkit/mozapps/extensions/test/xpcshell/test_proxies.js:113
(Diff revision 2)
>  
>    // Check that all add-ons original sources still exist after invalid
>    // add-ons have been removed at startup.
>    checkAddonsExist();
>  
> -  return AddonManager.getAddonsByIDs(ADDONS.map(addon => addon.id)).then(addons => {
> +  return AddonManager.getAddonsByIDs(ADDONS.map(addon => addon.id)).then(async addons => {

why isn't this just
`let addons = await AddonManager.getAddonsByIDs(...);`

::: toolkit/mozapps/extensions/test/xpcshell/test_proxies.js:210
(Diff revision 2)
> -    restartManager();
> -    shutdownManager();
> +    await promiseRestartManager();
> +    await promiseShutdownManager();

whats the point of this?

::: toolkit/mozapps/extensions/test/xpcshell/test_undouninstall.js:258
(Diff revision 2)
> +<<<<<<< dest
>    startupManager();
> +=======
> +  await promiseStartupManager(false);
> +>>>>>>> source

fix

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8974168 [details]
Bug 1459998: Part 1 - Rewrite tests not to use async helpers.

https://reviewboard.mozilla.org/r/242452/#review248398

> if you just move the call to do_test_pending before the call to promiseStartupManager(), you can leave out add_task()
> 
> Its functionally equivalent but simpler to read I think.

Probably. In general I think we should move tests to add_task style, though, and that's one less step when we eventually get around to converting this file.

> why isn't this just
> `let addons = await AddonManager.getAddonsByIDs(...);`

Don't ask me. I didn't write it.

> whats the point of this?

Make sure nothing disappears after restart, then shut down the manager so we can actually remove things without file locking errors and such.

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8974169 [details]
Bug 1459998: Part 2 - Cleanup cruft.

https://reviewboard.mozilla.org/r/242454/#review248416

Comment 9

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/641157ac54c22e3a97c4d6ebf8acef4918b7f167
Bug 1459998: Part 1 - Rewrite tests not to use async helpers. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/4079ea68361ce8e7848af6a8319fba362652e044
Bug 1459998: Part 2 - Cleanup cruft. r=aswan

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/641157ac54c2
https://hg.mozilla.org/mozilla-central/rev/4079ea68361c