If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED INCOMPLETE

Status

Cloud Services
Firefox Sync: Backend
RESOLVED INCOMPLETE
3 years ago
3 months ago

People

(Reporter: mihaelav, Assigned: Daniel Gherasim, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][sprint])

Attachments

(1 attachment, 1 obsolete attachment)

5.39 KB, patch
Andrei Eftimie
: review-
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Blocks: 1007559
(Reporter)

Comment 1

3 years ago
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!
Flags: needinfo?(edwong)

Comment 2

3 years ago
steps look good, nothing to add.
Flags: needinfo?(edwong)
Which sync preferences are you referring here? What makes it different from bug 1018931?
(Reporter)

Comment 4

3 years ago
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.
We can do everything as long as it is no os wide dialog.
Mentor: hskupin@gmail.com
Whiteboard: [mentor=whimboo][lang=js] → [lang=js]
(Reporter)

Updated

3 years ago
Blocks: 1035848
No longer blocks: 1007559
(Assignee)

Updated

3 years ago
Assignee: nobody → daniel.gherasim
(Assignee)

Comment 6

3 years ago
Created attachment 8463940 [details] [diff] [review]
v1.patch
Attachment #8463940 - Flags: review?(andrei.eftimie)
Attachment #8463940 - Flags: review?(andreea.matei)
(Assignee)

Updated

3 years ago
Depends on: 1009441
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
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.
Attachment #8463940 - Flags: review?(andrei.eftimie)
Attachment #8463940 - Flags: review?(andreea.matei)
Attachment #8463940 - Flags: review-

Comment 8

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
Created attachment 8468439 [details] [diff] [review]
v1.1.patch

Thanks, fixed what you asked.
Attachment #8463940 - Attachment is obsolete: true
Attachment #8468439 - Flags: review?(andrei.eftimie)
Attachment #8468439 - Flags: review?(andreea.matei)

Comment 10

3 years ago
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
Attachment #8468439 - Flags: review?(andrei.eftimie)
Attachment #8468439 - Flags: review?(andreea.matei)
Attachment #8468439 - Flags: review-
Whiteboard: [lang=js] → [lang=js][sprint]
(Assignee)

Updated

3 years ago
Depends on: 1079736
(Assignee)

Updated

3 years ago
No longer depends on: 1009441
Flags: needinfo?(markh)
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Flags: needinfo?(markh)
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.