1018927 - Add automated test to verify that sync prefs are saved after restart

Status: RESOLVED INCOMPLETE

(Firefox :: Sync, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Steps:

1. Sign into fxa
2. Change some sync preferences
3. Restart the browser
4. Check that the preferences changed at step 2 are persisted after browser restart

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]]

steps look good, nothing to add.

Comment 3

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

Which sync preferences are you referring here? What makes it different from bug 1018931?

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

With Edwin's input from bug 1018931 comment #2, the tests are the same. In bug 1018931 I was initially thinking to select the sync preferences when verifying the account (you get a dialog to select what to sync if you have previously selected the "Choose what to sync" option at account setup), but I'm not sure we can do that in our tests.

Comment 5

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

We can do everything as long as it is no os wide dialog.

Comment 6

[Daniel Gherasim]

Attachment: v1.patch

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8463940 [details] [diff] [review]
v1.patch

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

::: services/sync/tests/tps/mozmill_1018927_syncPrefs.js
@@ +14,5 @@
> +  {label: "Bookmarks", checked: false},
> +  {label: "Passwords", checked: false},
> +  {label: "History", checked: false},
> +  {label: "Add-ons", checked: false},
> +  {label: "Preferences", checked: true}

Can we please sort these alphabetically?

@@ +22,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  TPS.Login();
> +}
> +
> +function testSyncPreferences() {

I'd like to have jsdoc here

@@ +23,5 @@
> +  TPS.Login();
> +}
> +
> +function testSyncPreferences() {
> +  // Instatiate the sync UI related objects

nit: instantiate

@@ +43,5 @@
> +    if (pref.checked !== aPref.getNode().checked) {
> +      aPref.click();
> +    }
> +  });
> +

Please remove this empty line.

::: services/sync/tests/tps/mozmill_1018927_syncPrefsPersisted.js
@@ +37,5 @@
> +    var pref =  PREFS.filter(aElement => {
> +      return aElement.label === aPref.getNode().getAttribute("label");
> +    })[0];
> +
> +    expect.equal(aPref.getNode().checked, pref.checked,

This should be assert, I believe you used expect so we catch all but in the end one pref not persisted and the test should fail.

Comment 8

[Andrei Eftimie]

(In reply to Andreea Matei [:AndreeaMatei] from comment #7)
> Comment on attachment 8463940 [details] [diff] [review]
> v1.patch
> 
> Review of attachment 8463940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/tests/tps/mozmill_1018927_syncPrefs.js
> @@ +14,5 @@
> > +  {label: "Bookmarks", checked: false},
> > +  {label: "Passwords", checked: false},
> > +  {label: "History", checked: false},
> > +  {label: "Add-ons", checked: false},
> > +  {label: "Preferences", checked: true}
> 
> Can we please sort these alphabetically?

I would argue to leave them in the order they are presented in the UI, which is this order.

Comment 9

[Daniel Gherasim]

Attachment: v1.1.patch

Thanks, fixed what you asked.

Comment 10

[Andrei Eftimie]

Comment on attachment 8468439 [details] [diff] [review]
v1.1.patch

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

I'm having some issues properly applying this patch.
Anyway we'll have to wait for bug 1009441 to land first.

::: services/sync/tests/tps/mozmill_1018927_syncPrefs.js
@@ +14,5 @@
> +  {label: "Bookmarks", checked: false},
> +  {label: "Passwords", checked: false},
> +  {label: "History", checked: false},
> +  {label: "Add-ons", checked: false},
> +  {label: "Preferences", checked: true}

Well, this approach won't work for localised builds.
We'll have to take the labels from DTD's...

Or I have a better idea, instead of using the label, lets use their `preference` attribute: engine.tabs, engine.bookmarks, engine.passwords, engine.history, engine.addons, engine.prefs
These should not change for l10n builds, and should be less brittle than labels.

We could probably also use the name as a key and have an object:

> {
>   "engine.tabs": true,
>   [...]
> }

@@ +36,5 @@
> +            "FXA signed in account deck is displayed");
> +
> +  var syncPrefsList = syncPrefs.getElement({type: "syncPreferences"});
> +
> +  // Check/Uncheck some preferences

So the "some" part is dictated by the `checked` value in the PREFS constant?
Seems a bit counterintuitive...

@@ +39,5 @@
> +
> +  // Check/Uncheck some preferences
> +  syncPrefsList.elements.forEach(aPref => {
> +    var label = aPref.getNode().getAttribute("label");
> +    var pref = PREFS.filter(aElement => {

With the mentioned changes to the PREFS constant, we can also get rid of this filter and select the element directly.
Should simplify this whole block a lot.

@@ +41,5 @@
> +  syncPrefsList.elements.forEach(aPref => {
> +    var label = aPref.getNode().getAttribute("label");
> +    var pref = PREFS.filter(aElement => {
> +        return aElement.label === label;
> +      })[0];

nit: indent

Comment 11

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

Thanks for the contribution, but this patch has bit-rotted, and we no longer make any attempt to have TPS login via FxA's UI. I'm happy to look at an updated patch, but I'll close this for now.