1196096 - SyncManager in System app.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Sean Lee [:seanlee][:weilonge]]

In system app, the synchronization scheduler do the following task:
* Handle Data Sync configurations changes, e.g. collection selection (history, bookmarks, tabs, and password), sync enabling or disabling and "Sync now" request. (1168185)
* Sync State Machine - handling these states: disabled, enabling, enabled, synchronizing, error. (bug 1168160)
* Configure requestSync and then trigger the synchronization task to Synchronizer app (bug 1195647).

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] ferjm:bug1196096.syncmanager > mozilla-b2g:master

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Test applications

These are the applications I am using to test the manager locally.

- test-sync app: this is a mock of the settings UI.
- test-synchronizer app: this is a mock of the synchronizer app.

With these two applications installed you should be able to enable Sync, which should schedule a sync request to be triggered in 2 minutes. You can also play with the SyncStateMachine and  check that some states are allowed and others aren't depending on the current state. You can logout from FxA while Sync is enabled to check that it is properly disabled. You can change the collections to be synchronized. You can request a sync on demand, which should open the synchronizer app that emulates a sync with a five seconds timeout and sends the result of the sync back to the manager ....

I recommend enabling debug on SyncManager while testing this.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8657355 [details] [review]
[gaia] ferjm:bug1196096.syncmanager > mozilla-b2g:master

This is still not finished. I need to clean the code, add some more comments, fix the TODOs and write some unit tests, but I could use some feedback in the meantime.

I am specially interested on feedback about the architecture (IAC, settings, events usage, etc.), edge cases and potential bugs that I missed.

Thank you!

Comment 4

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8657355 [details] [review]
[gaia] ferjm:bug1196096.syncmanager > mozilla-b2g:master

Great stuff, exciting to see us progress! :) Left some comments on github.

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8657355 [details] [review]
[gaia] ferjm:bug1196096.syncmanager > mozilla-b2g:master

Hey Fernando,

Thanks a lot for defining IAC API for Settings app. \O/
The two main opinions have been left at github:
* IAC API definition for getting status of FxSync.
* L10N support for error messages.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Test applications

I am updating the test applications to address the latest changes in the SyncManager. Basically, the app mocking what the Settings app should do is now using the IAC based API to obtain the information about Sync (current state, error, last sync time) instead of the Settings API, as suggested by Sean.

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8657355 [details] [review]
[gaia] ferjm:bug1196096.syncmanager > mozilla-b2g:master

Etienne, do you mind taking a look at this patch, please?

You can find an explanation of what the SyncManager is supposed to do in the code comments, but feel free to ping me on IRC if you need any further explanation.

Thanks!

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 8657355 [details] [review]
[gaia] ferjm:bug1196096.syncmanager > mozilla-b2g:master

Looking good!
Made some comments on github, mainly waiting for the Settings stuff to be sorted out before r+'ing.

Question: any ideas about how we're going to add integration "sanity" tests for this once all the moving parts will be in place?

Remark: once everything is landed we should formalize the review ownership for this, probably something similar to the dialer (ie. an app directory + very specific system app files).

Cheers!

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Etienne Segonzac (:etienne) from comment #8)
> Comment on attachment 8657355 [details] [review]
> [gaia] ferjm:bug1196096.syncmanager > mozilla-b2g:master
> 
> Looking good!
> Made some comments on github, mainly waiting for the Settings stuff to be
> sorted out before r+'ing.

Thanks! I just updated the PR.

> 
> Question: any ideas about how we're going to add integration "sanity" tests
> for this once all the moving parts will be in place?
> 

For functionally testing the SyncManager we'll need to finish Bug 1168185 first and write some Marionette tests to test the Settings UI.

For functionally testing the whole Sync feature, we need to check if Marionette allow us to clear the profile and reboot the phone in the middle of a test. Or if we can trigger two different instances of B2G within the same Marionette test (seems unlikely). Once we have that, we should be able to test how the synchronization happens between two different sync clients.

> Remark: once everything is landed we should formalize the review ownership
> for this, probably something similar to the dialer (ie. an app directory +
> very specific system app files).
> 

Yes. We should be able to move this to a subfolder once bug 1201804 is fixed.

Comment 10

[Etienne Segonzac (:etienne)]

Comment on attachment 8657355 [details] [review]
[gaia] ferjm:bug1196096.syncmanager > mozilla-b2g:master

Awesome, thanks for addressing all the comments!

> > 
> > Question: any ideas about how we're going to add integration "sanity" tests
> > for this once all the moving parts will be in place?
> > 
> 
> For functionally testing the SyncManager we'll need to finish Bug 1168185
> first and write some Marionette tests to test the Settings UI.
> 
> For functionally testing the whole Sync feature, we need to check if
> Marionette allow us to clear the profile and reboot the phone in the middle
> of a test. Or if we can trigger two different instances of B2G within the
> same Marionette test (seems unlikely). Once we have that, we should be able
> to test how the synchronization happens between two different sync clients.

Can we spawn a fake sync server with static content with the current marionette tooling?
Without going all the way to testing 2 way sync it would be cool to have a sanity check marionette test involving settings + system + sync app.

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Etienne Segonzac (:etienne) from comment #10)
> Comment on attachment 8657355 [details] [review]
> [gaia] ferjm:bug1196096.syncmanager > mozilla-b2g:master
> 
> Awesome, thanks for addressing all the comments!
> 

Thanks for the review :)

> > > 
> > > Question: any ideas about how we're going to add integration "sanity" tests
> > > for this once all the moving parts will be in place?
> > > 
> > 
> > For functionally testing the SyncManager we'll need to finish Bug 1168185
> > first and write some Marionette tests to test the Settings UI.
> > 
> > For functionally testing the whole Sync feature, we need to check if
> > Marionette allow us to clear the profile and reboot the phone in the middle
> > of a test. Or if we can trigger two different instances of B2G within the
> > same Marionette test (seems unlikely). Once we have that, we should be able
> > to test how the synchronization happens between two different sync clients.
> 
> Can we spawn a fake sync server with static content with the current
> marionette tooling?

I'm not sure. But it'd be quite useful indeed.

> Without going all the way to testing 2 way sync it would be cool to have a
> sanity check marionette test involving settings + system + sync app.

For testing this flow we don't really need the Sync server, just the Firefox Accounts one. This kind of stuff is what we need to investigate on bug 1202347. We'll work on that on the next sprint that starts on Monday :)

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/47c773889c03657919fd326ab7740ccab052ad3e