Closed
Bug 1018927
Opened 10 years ago
Closed 7 years ago
Add automated test to verify that sync prefs are saved after restart
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: mihaelav, Assigned: danisielm, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=js][sprint])
Attachments
(1 file, 1 obsolete file)
5.39 KB,
patch
|
andrei
:
review-
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 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 3•10 years ago
|
||
Which sync preferences are you referring here? What makes it different from bug 1018931?
Reporter | ||
Comment 4•10 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.
Comment 5•10 years ago
|
||
We can do everything as long as it is no os wide dialog.
Updated•10 years ago
|
Mentor: hskupin
Whiteboard: [mentor=whimboo][lang=js] → [lang=js]
Reporter | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → daniel.gherasim
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8463940 -
Flags: review?(andrei.eftimie)
Attachment #8463940 -
Flags: review?(andreea.matei)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 7•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 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-
Updated•10 years ago
|
Whiteboard: [lang=js] → [lang=js][sprint]
Updated•7 years ago
|
Flags: needinfo?(markh)
Comment 11•7 years ago
|
||
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
Closed: 7 years ago
Flags: needinfo?(markh)
Resolution: --- → INCOMPLETE
Updated•6 years ago
|
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•