Sync tests should use the FxA provider by default

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: markh, Assigned: markh)

Tracking

unspecified
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
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.
Attachment #8813021 - Flags: review?(tchiovoloni) → review+

Comment 4

a year ago
mozreview-review
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)
Attachment #8813022 - Flags: review?(tchiovoloni) → review+
Assignee: nobody → markh
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
mozreview-review
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.
(Assignee)

Comment 8

a year ago
mozreview-review
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
(Assignee)

Comment 9

a year ago
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

a year ago
Pushed by mhammond@skippinet.com.au:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f9d328d6fe20
https://hg.mozilla.org/mozilla-central/rev/ed292fc1713c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.