1018931 - Add automated test to verify that sync options specified at sign in are persisted after restart

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Steps:

1. Sign into FxA with only some of the data types to sync enabled
2. Restart the browser
3. Go to Preferences > Sync and check the UI
> Only the data types selected at step 1 are enabled

Comment 1

[Mihaela Velimiroviciu (:mihaelav)]

Edwin, can you please take a look over the steps and let us know if they are OK or there is anything else needed for this test?
Thanks!

Comment 2

[Edwin Wong [:edwong]]

this is more clear:

1. Sign into FxA
2. open Preferences > Sync and change the data types selected
3. Restart browser
4. Go to Preferences > Sync and check the UI

Otherwise - looks good.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It may also be good to not only test some of them but all. Let it be in chunks or each one individually.

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Edwin Wong [:edwong] from comment #2)
> this is more clear:
> 
> 1. Sign into FxA
> 2. open Preferences > Sync and change the data types selected
> 3. Restart browser
> 4. Go to Preferences > Sync and check the UI
> 
> Otherwise - looks good.

The test for these steps is actually tracked in bug 1018927. What I wanted for this was  to select the sync preferences when verifying the account (in the dialog you get if you have previously selected the "Choose what to sync" option at account setup). Clearer steps for this would be:

1. Set up a Sync account, checking the "Choose what to sync" option at setup
2. Verify the account
> You are shown the sync options dialog
3. Select some of the options and submit
4. Restart the browser
5. Go to Preferences > Sync and check the UI

What do you think about this?

Comment 5

[Edwin Wong [:edwong]]

:mihaelav - you're right - my steps from comment #2 is a dupe of bug 1018927

Your steps in comment #4 is good, please proceed with those steps.

Comment 6

[Daniel Gherasim]

I'll take it as jakub hasn't give us any input here in almost 2 months.

Comment 7

[Daniel Gherasim]

Attachment: WIP.patch

I have an WIP patch ready for this.

There are still issues with waiting for the content to load (waitForPageLoad for some pages doesn't seem to be enough), also for iframe elements, the "load" event is not enough, so as a workaround we wait for an element on page to be avaible.

Creating a new account with restmail & verifying it works great, what I see as a problems here (on which I currently don't have an idea how to solve) would be:

1. Auto-sync (login) called at start-up/closed of firefox may broke our test and let the firefox hang-out. 
It happened to me in one specific case: I closed the firefox with an unverified account, at the next test, when closing the firefox it actually freezed.
( The line where it was hanging is: Weaves.Services.Login())

2. Removing the account in teardown. I currently couldn't find an API method to force this.

3. We may also have to include the windows.js library implemented in 1006996 to ensure all windows are closed. Otherwise we again hit the problem from step 1.
And again, if one window stays open (let's say it's modal) removing the account through the interface will fail.

There might be other problems too. 
But I'm currently updating an WIP patch for a feedback, before I continue with improving this.

So Henrik, could you tell me if I'm on the right path here ? Just ignore the modifications that will be made once your review on the dependent bug will be handled by us.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8487838 [details] [diff] [review]
WIP.patch

Review of attachment 8487838 [details] [diff] [review]:
-----------------------------------------------------------------

Something I wonder when checking this patch is if this really needs to be a test in the TPS suite, or if we can make it a simple restart test in mozmill-tests. There is nothing in here, where we need the TPS framework around, or?

::: services/sync/tests/lib/syncUI.js
@@ +192,5 @@
>      var spec = aSpec || { };
>      var root = spec.parent ? spec.parent.getNode() : this._controller.window.document;
>      switch (spec.type) {
> +      case "inputSync":
> +        var iframe = this.getElement({type: "remoteFrame"}).getNode();

Please put an extra type into the list to retrieve the iFrame, which then can be used as root. We shouldn't try to receive it each single time.

::: services/sync/tests/tps/mozmill_setSignInOptions.js
@@ +15,5 @@
> +var utils = require("../lib/utils");
> +
> +const TEST_DATA = {
> +  getMailUrl: "http://restmail.net/mail/",
> +  signUpUrl: "https://accounts.firefox.com/signup?service=sync&context=fx_desktop_v1",

the base url we should retrieve from the preference here.

@@ +16,5 @@
> +
> +const TEST_DATA = {
> +  getMailUrl: "http://restmail.net/mail/",
> +  signUpUrl: "https://accounts.firefox.com/signup?service=sync&context=fx_desktop_v1",
> +  user: 'test1018931' + new Date().getTime(),

For the user prefix please use the same value as we have on the python side. It should be 'coversheet_' afair.

@@ +70,5 @@
> +                   "Frame has been loaded");
> +  }
> +  finally {
> +    iframe.removeEventListener("load", onFrameLoaded);
> +  }

Does waitForPageLoad() not work for the frame document?

@@ +103,5 @@
> +  controller.waitForPageLoad();
> +  md.waitForDialog();
> +}
> +
> +function handleSelectOptionsToSyncDialog(aController) {

new UI class?

@@ +125,5 @@
> +
> +function getEmail(aUrl, aCallback) {
> +  var data = null;
> +
> +  xhr.open('get', aUrl, true);

I would suggest that this gets added to the sync backend module.

@@ +166,5 @@
> +    assert.waitFor(() => finishedSignedOut, "Sign out process finished");
> +  }
> +  finally {
> +    Services.obs.removeObserver(observer, "fxaccounts:onlogout");
> +  }

Same for that one.

::: services/sync/tests/tps/mozmill_testSignInOptionsPersisted.js
@@ +27,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +}
> +
> +function teardownModule(aModule) {
> +  // Couldn't find an API method to use for removing the account

there should definitely be one. We will have to check that.