987185 - Add test for the functionality of the Sync button when user is signed into an account

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

This is a follow-up bug for the Sync button functionality in Australis - bug #967000 comment #21:
"For this, too, can you file a followup bug to test that it starts a sync process if you're signed in?"

Comment 1

[Mihaela Velimiroviciu (:mihaelav)]

I think this is TPS dependent. Henrik, can you please help with some tracking/blocking bugs for this? Thank you!

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

This is definitely a TPS related test. Also it might have to rely on Mozmill. As of now we do not have Mozmill tests active, and also have to update mozmill to 2.0.6. But all that should not stop us to get a test implemented.

Mihaela, do you want to work on that test?

Comment 3

[:Gijs (he/him)]

I don't know what TPS is, but I presume that this can just be a browser mochitest with some mocking of the signed in state...

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

If a mockup for sync is fine for you, you wont need tps (https://developer.mozilla.org/en-US/docs/TPS), which is for testing against the real sync server.

Comment 5

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Mihaela, do you want to work on that test?
I was thinking that maybe we can use this in this week's automation training days. If nobody else jumps for it, I can work on it later.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

I don't think we will find someone with such an experience to work on that during the training day. So it would be good if you can get started. I'm happy to help with sync and fxaccount related questions.

Comment 7

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v1

I couldn't fake the account to be signed in so that clicking the Sync button to start a sync, but I managed to set up the account and pressing the Sync button starts the login with the configured account, which IMO also validates the functionality of the button.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8432502 [details] [diff] [review]
v1

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

This looks reasonable to me, but for the nits below. I'm flagging Mark for review because this is sync stuff and I am not an expert. Can you also do a try push of this patch? Thanks!

::: browser/components/customizableui/test/browser_987185_sync.js
@@ +7,5 @@
> +let tmp = {};
> +
> +Components.utils.import("resource://gre/modules/Promise.jsm", tmp);
> +Components.utils.import("resource://gre/modules/Services.jsm", tmp);
> +Components.utils.import("resource://gre/modules/FxAccounts.jsm", tmp);

I expect none of these imports are necessary. :-)

@@ +17,5 @@
> +
> +  let account = new FxAccounts({onlySetInternal: true})
> +  let credentials = {
> +    email: "[e-mail]",
> +    uid: "[e-mail]",

lcip.org ? :-)

Might as well use example.org/.com if it doesn't matter.

@@ +55,5 @@
> +  // reset the panel UI to the default state
> +  yield resetCustomization();
> +  ok(CustomizableUI.inDefaultState, "The panel UI is in default state again.");
> +
> +  if(isPanelUIOpen()) {

Nit: space after 'if'

Comment 9

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v1.1

Updated patch.

remote:   https://tbpl.mozilla.org/?tree=Try&rev=9ad448e09ec5

Comment 10

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

Comment on attachment 8432539 [details] [diff] [review]
v1.1

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

As you probably noticed, the try run was orange on some platforms.  Without arranging for the sync logs to appear in the test output, it's hard to guess why, but the most likely option is that without mocking more of the FxA stuff, we might be failing to initialize correctly, so no attempt to login will happen.  This initialization is async.  I suspect this test will also cause us to hit the real network to try and authenticate the stub user you setup (which might not be a real problem)

Also, the platforms that pass still log:

09:05:51     INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
09:05:51     INFO -  [Exception... "Unexpected error"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/browser.js :: SUI_onActivityStart/< :: line 9294"  data: 

Sadly getting logs will require some cargo-culting - probably making the logs 'FirefoxAccounts', 'Sync.BrowserIDManager' and 'Sync.SyncScheduler' have a level of TRACE and creating a new log DumpAppender and attaching it to them - the resulting dump() calls should end up in the log.

On a more general note, I don't actually see a whole lot of value in this test trying to go quite this far.  The button calls gSyncUI.handleToolbarButton() and we want to ensure that calls Weave.Service.errorHandler.syncAndReportErrors().  The latter function already has test coverage, so it's probably not necessary to check a sync actually starts, simply that the function is called (or maybe that service.sync() is called, which is what syncAndReportErrors() calls).  So I wonder if the test can mock-out gSyncUI.needsSetup() to always return false, then mock-out either errorHandler.syncAndReportErrors or service.sync just to verify the call is made, and if so, assume the sync xpcshell tests cover the fact that these functions actually work as intended.  This might help keep sync and FxA totally out of the test path which sounds like a win.

::: browser/components/customizableui/test/browser_987185_sync.js
@@ +27,5 @@
> +  yield account.setSignedInUser(credentials);
> +
> +  Weave.Svc.Obs.add("weave:service:login:start", function onSyncStart() {
> +    Weave.Svc.Obs.remove("weave:service:login:start", onSyncStart);
> +    loginStarted = true;

instead of a flag and a waitForCondition, an alternative would be to create a deferred and resolve it in the observer.  The waitForCondition would then be replaced with a yield for the promise.

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v2

I mocked the syncAndReportErrors function and faked the conditions for needsSetup to return false.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e987a461b109

Comment 12

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

Comment on attachment 8498886 [details] [diff] [review]
v2

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

This looks fine to me - thanks!  r=me with the requested changes.

::: browser/components/customizableui/test/browser_987185_syncButton.js
@@ +13,5 @@
> +let service = syncService.Service;
> +let signedInUser;
> +let syncWasCalled = false;
> +
> +add_task(function() {

generator functions should be declared as "function* ()" these days (both here and for asyncCleanup) - and this function should have a name as IIRC, the name is used in some test logs.

@@ +49,5 @@
> +});
> +
> +function mockFunctions() {
> +  // alter properties to make gSyncUI._needsSetup() always return false
> +  Weave.Status._authManager._signedInUser = false;

Can we do the same mock for _needsSetup - I'd think something like gSyncUI._needsSetup = function() false; (along with saving and restoring like for syncAndREportErrors) would do it and be a little cleaner and more obvious.

Comment 13

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v2.1

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Try runs: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=45547642876d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/23a0a5f3c0ad

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23a0a5f3c0ad