1319323 - Sync tests should use the FxA provider by default

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

Most Sync xpcshell tests use the "legacy" provider which is deprecated. It adds complexity to tests and makes adding new features more difficult. We want to remove the old provider at some stage, and the tests are the biggest blockers.

I'm attaching a couple of patches that:
* removes many ensureLegacyIdentityManager() calls from the tests.
* changes SyncTestingInfrastructure to test with the FxA identity (it previously arranged for the legacy provider).

Some ensureLegacyIdentityManager calls remain, particularly in tests that don't really make sense with FxA. add_identity_test() also still remains, allowing for tests to be run with both providers.

But this seems a good start.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1319323 (part 1) - improve sync unit tests; remove many ensureLegacyIdentityManager calls.

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

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1319323 (part 2) - improve sync unit tests; SyncTestingInfrastructure now uses the FxA identity provider.

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

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8813021 [details]
Bug 1319323 (part 1) - improve sync unit tests; remove many ensureLegacyIdentityManager calls.

https://reviewboard.mozilla.org/r/94540/#review94906

r=me, it would be nice to address the nit, but IMO it's not worth rewriting the test to address (and certainly not worth the larger "clean up our tests" effort on a "this test could be slightly cleaner" complaint)

::: services/sync/tests/unit/test_hmac_error.js:205
(Diff revision 1)
>  
>      _("Be prepared for the second (automatic) sync...");
>    }
>  
>    _("Make sure that syncing again causes recovery.");
> +  await new Promise(resolve => {

How much work would this take to make not use such nested callbacks inside a promise? (either .then's or more granular await seems better)

Seems worth fixing unless it would take substantial work -- feel free to drop if it would be.

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8813022 [details]
Bug 1319323 (part 2) - improve sync unit tests; SyncTestingInfrastructure now uses the FxA identity provider.

https://reviewboard.mozilla.org/r/94542/#review94912

Looks good to me as long as everything still works!

::: services/sync/tests/unit/test_syncengine.js:195
(Diff revision 1)
>    }
>  }
>  
> -function run_test() {
> +add_task(async function run_test() {
>    server = httpd_setup({});
> -  test_url_attributes();
> +  await test_url_attributes();

What's the reason these aren't done via add_task? Shared server setup? 

(Not opening an issue since I don't think it matters and nearly any answer here is probably fine by me)

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8813021 [details]
Bug 1319323 (part 1) - improve sync unit tests; remove many ensureLegacyIdentityManager calls.

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

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8813022 [details]
Bug 1319323 (part 2) - improve sync unit tests; SyncTestingInfrastructure now uses the FxA identity provider.

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

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8813022 [details]
Bug 1319323 (part 2) - improve sync unit tests; SyncTestingInfrastructure now uses the FxA identity provider.

https://reviewboard.mozilla.org/r/94542/#review95586

::: services/sync/tests/unit/test_syncengine.js:195
(Diff revision 1)
>    }
>  }
>  
> -function run_test() {
> +add_task(async function run_test() {
>    server = httpd_setup({});
> -  test_url_attributes();
> +  await test_url_attributes();

Mainly because I was lazy :) I changed this to use multiple add_task calls.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8813022 [details]
Bug 1319323 (part 2) - improve sync unit tests; SyncTestingInfrastructure now uses the FxA identity provider.

https://reviewboard.mozilla.org/r/94542/#review95588

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Sheesh, reviewboard can be dumb - I already wrote this in reviewboard when clearing the issue, but it seems to have lost the comment :/

(In reply to Thom Chiovoloni [:tcsc] from comment #3)
> How much work would this take to make not use such nested callbacks inside a
> promise? (either .then's or more granular await seems better)

The problem is that 3 observer notifications are fired here, which don't work well with promises, and as you saw, rewriting the test for that doesn't seem worthwhile. It probably would have read better using PromiseUtils.defer but I want to avoid that module spreading too far (indeed, I want to see that module deleted ;)

Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9d328d6fe20
(part 1) - improve sync unit tests; remove many ensureLegacyIdentityManager calls. r=tcsc
https://hg.mozilla.org/integration/autoland/rev/ed292fc1713c
(part 2) - improve sync unit tests; SyncTestingInfrastructure now uses the FxA identity provider. r=tcsc

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9d328d6fe20
https://hg.mozilla.org/mozilla-central/rev/ed292fc1713c