It's critical we update TPS, sync desktop client, to use FxA. about: https://developer.mozilla.org/en-US/docs/TPS src: http://hg.mozilla.org/services/services-central/file/279aaa11c402/testing/tps TPS would allow us to quickly test sync 1.5 servers and FxA in desktop to minimize regressions and use test driven development.

You were right with the component the first time :) TPS just drives a real Firefox, talking to production servers, so there's no server-side part to this.

Here's my error when running Nightly, test passes with Fx26 ♠ runtps --testfile=/Users/<user>/dev/services-central/services/sync/tests/tps/test_sync.js --binary=/Applications/FirefoxNightly.app/Contents/MacOS/firefox using result file tps_result.json MOZPROCESS WARNING: ProcessHandler.waitForFinish() is deprecated, use ProcessHandler.wait() instead CROSSWEAVE INFO: Sync version: 1.32.0 CROSSWEAVE INFO: Firefox builddate: 20140205030203 CROSSWEAVE INFO: Firefox version: 30.0a1 CROSSWEAVE INFO: Waiting for weave:service:ready... CROSSWEAVE INFO: weave:service:ready observed! CROSSWEAVE INFO: Starting phase 1/4 CROSSWEAVE INFO: setting client.name to profile1 CROSSWEAVE INFO: ----------event observed: sessionstore-windows-restored CROSSWEAVE INFO: starting action: [null] CROSSWEAVE INFO: Setting client credentials. CROSSWEAVE ERROR: [phase1] Exception caught: account setter should be not used in BrowserIDManager No traceback available TEST-UNEXPECTED-FAIL | test_sync.js | [phase1] Exception caught: account setter should be not used in BrowserIDManager No traceback available phase1: FAIL Test Summary TEST-UNEXPECTED-FAIL | test_sync.js | [phase1] Exception caught: account setter should be not used in BrowserIDManager No traceback available

The Sync command in a TPS test case automatically invokes the origin weave.login() method: http://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/tps/modules/tps.jsm#809 I'm guessing we need to update TPS not to do this.

This week I'm totally covered in getting our automation working for Metro. Therefore Mozmill 2.1 is needed. Once this is done I will getting started for fxaccount. Until then this bug has to wait or find some other assignee in the meantime.

:jedp any chance you can look into this. TPS offers significant test coverage for desktop and sync servers.

Gavin, still looking for someone to fix this, it broke with our fxa changes. It offers a lot of test coverage, tests to run in continuous integration, and uncover some profile integrity bugs.

Mark, Chris, any easy fixes here? I don't know anything about TPS. rnewman, do you have any advice?

The part of TPS we care about here is basically an add-on that sets up Sync. Someone needs to spend the time to touch services/sync/tps/extensions/tps/modules/sync.jsm and related code to set up a Firefox Account programmatically. There will be a small amount of busy-work to pre-verify the email address, or to somehow simulate verification during setup.

(In reply to Richard Newman [:rnewman] from comment #8) > The part of TPS we care about here is basically an add-on that sets up Sync. > > Someone needs to spend the time to touch > > services/sync/tps/extensions/tps/modules/sync.jsm > > and related code to set up a Firefox Account programmatically. There will be > a small amount of busy-work to pre-verify the email address, or to somehow > simulate verification during setup. Are there API's for this, or does this have to be done via the UI (via a mozmill script?)

/services/fxaccounts/FxAccounts.jsm, setSignedInUser() should suffice for the pure-FxA part. Then there's presumably some Sync configuration, too. Lacking the time right now to dig up the UI setup code to find out what wraps it.

So I'm back from vacation and I might have the time to dig into tps. Not sure how fast I will be but hopefully I can find the problem in the next days so a solution will be ready by end of the week. Edwin, would that be sufficient?

:whimboo earlier the better- but friday works.

So I tried to run the tps tests but it already fails in setting up the virtual environment. Exactly in the step for installing setuptools. " --always-copy -U setuptools: Downloading http://pypi.python.org/packages/2.7/s/setuptools/setuptools-0.6c11-py2.7.egg Traceback (most recent call last): [..] " --always-copy -U setuptools failed with error code 1 ./INSTALL.sh: line 53: bin/activate: No such file or directory virtualenv wasn't installed correctly, aborting I will have to get this fixed first before I can tackle the real underlying issue here.

So it turned out the used virtualenv.py from jgriffins github account is kinda outdated and refers to virtualenv 1.7.1.2. We should update the URL to the officially virtualenv github repository and version 1.9 (https://raw.github.com/pypa/virtualenv/1.9.X/virtualenv.py), which is the last version which allows to run the script without bundled setuptools/pip packages. I will file a new bug to get this fixed.

As of now tps contains mozmill 2.0b1 which is a very outdated and broken version of the extension. I will not upgrade it right now but that should be definitely something to consider for future development of tps!

It's a bit disappointing that no developer documentation exists for sync. In this case it's for https://developer.mozilla.org/en-US/search?q=BrowserIDManager. Is there any work planned for that? It would kinda help developers and add-on authors to work on and with sync.

:whimboo what is the reference? What are you looking at that shows BrowserIDManager? I can't even find that in GitHub...

Well, it's not only that class. It's nearly all under http://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/. I'm digging through all that code at the moment, and hope to have a solution soon wit the above hint (comment 10). A question through. When we update tps it should work with both fxaccounts and the old sync, right? Just in case we have to disable fxaccounts for whatever reason. The tests should not fail then.

That's a good question. I would think initially it should run with both for compatibility/functional checks of the TPS code itself. But the idea of "disabling" fxaccounts has no meaning with Fx29 and beyond.

There are a couple of setters in browserid_identity.js which are implemented like that now: http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/browserid_identity.js#241 set account(value) { throw "account setter should be not used in BrowserIDManager"; }, That work was done via bug 949259 and no further information is given what you should use instead. It would be good to call out what code has to be used instead. Brian, can you help us here?

:whimboo, for now just forking and disabling the sync 1.1 path is fine. But eventually we want to regression test sync 1.1 till we officially disable it's usage - which is Fx31/32 time frame. Also, Tim Talbert can help if you have an low level questions with FxA+Sync1.5

(In reply to Edwin Wong [:edwong] from comment #21) > :whimboo, for now just forking and disabling the sync 1.1 path is fine. But > eventually we want to regression test sync 1.1 till we officially disable > it's usage - which is Fx31/32 time frame. Ok, so the correct code path has to be run given if fxaccounts are enabled or not via the preference. I will take this into account. > Also, Tim Talbert can help if you have an low level questions with > FxA+Sync1.5 I know and already tried to reach him. But it may be too late today. I will try tomorrow.

I added some pointers to Bug 949259.

Ok, so Tim gave me a reference to a test for Australis which uses fxaccounts: https://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_946320_tabs_from_other_computers.js So i will try to get it running in a similar way for tps. I will have to see how much data we can fake so we can still use the account which is already setup.

(In reply to Richard Newman [:rnewman] from comment #23) > I added some pointers to Bug 949259. Thanks. services/sync/modules-testing/utils.js looks to be a good starting point. I will work with that.

(In reply to Henrik Skupin (:whimboo) from comment #25) > (In reply to Richard Newman [:rnewman] from comment #23) > > I added some pointers to Bug 949259. > > Thanks. services/sync/modules-testing/utils.js looks to be a good starting > point. I will work with that. Richard, those testing modules are not available for nightly builds of Firefox. So we cannot use if for testing. I don't really want to copy those to the tps extension and keep them in-sync. So what do you think?

Just use it as inspiration. TPS's account setup should be fairly straightforward inlined code.

(In reply to Richard Newman [:rnewman] from comment #27) > Just use it as inspiration. TPS's account setup should be fairly > straightforward inlined code. That's what I tried in the meantime and it looks like that I found a problem. In tps.jsm we call Weave.Service.wipeServer() after the call to Weave.Service.login(). It fails with: this.storageURL is null JS Stack trace: wipeServer@service.js:1333:1 < ResetData@tps.jsm:822:5 < TPS.RunNextTestAction@tps.jsm:567:7 Richard is that a known problem? As it looks like the value is null and 'this.storageURL.slice(0, -1)' fails.

(In reply to Henrik Skupin (:whimboo) from comment #28) > That's what I tried in the meantime and it looks like that I found a > problem. In tps.jsm we call Weave.Service.wipeServer() after the call to > Weave.Service.login(). It fails with: FxA's auth model is much more complicated than Sync 1.1. Sync 1.1 uses Basic Auth: once you have the username and password, you can do anything you want. FxA requires you to exchange your username and password for a bundle of FxA-related values, then exchange those for a Sync token, which also includes the storage URL. You can't perform any storage operations until you've gone through those steps. Waiting for weave:service:setup-complete is probably enough.

Ok, so good news! I was able to login via SetSignedInUser() and the tests are getting run with a single failure: phase1: PASS phase2: PASS phase3: PASS phase4: FAIL CROSSWEAVE ERROR: [phase4] Exception caught: ASSERTION FAILED! Uri visits found in history database, but they shouldn't be No traceback available Most likely wiping the data doesn't work as expected. I will check that. Otherwise what I will do now is most likely to create a patch which enables us to use fxaccounts, keep all the old sync 1.x code around but comment it out. I can do a refactoring of the code in a follow-up patch. Jonathan, when I have a patch, how do I have to test it? Is there anything on try? Or is all testing done in a separate CI? How is testing done then?

:whimboo - Nice! I think... phase 4 could be a real bug as the error doesn't have anything to do with your authentication change. Thank you for working so quickly!

(In reply to Henrik Skupin (:whimboo) from comment #30) > Jonathan, when I have a patch, how do I have to test it? Is there anything > on try? Or is all testing done in a separate CI? How is testing done then?

If it works locally, then I can deploy the patches to CI, which is running on an Ubuntu VM inside the MPT VPN. The CI reports to autolog (although no recent reports are available, since I turned the CI off since all it did was to repeat the same error over and over again).

Actually, if the only patches needed are to in-tree code, the CI will pick them up automatically; I'll just need to start it up again, which I'll do now.

I just want to upload the current state given that my day is over. If you want have a look, but please be warned that nothing has been cleaned-up yet nor is in the final state.

Btw, I've turned TPS back on in CI, and it's reporting to http://brasstacks.mozilla.com/autolog/?tree=mozilla-inbound&source=autolog. Any changes that are landed on mozilla-inbound will get automatically picked up by this, and you can view the results at that url.

Once we have this bug fixed we have to update the documentation on MDN: https://developer.mozilla.org/en-US/docs/TPS

Cleaned-up version of the patch and finally got the real login working by using FxAccountsClient.signIn() together with setSignedInUser. That way we only need the email and password, and can be sure to get a fresh sessionToken each time. That's more close to the purpose of our testing. The version as presented here will not remove all the old sync code, but comments it out. As a follow-up patch I will have to refactor the code of the extension to allow both fxaccounts and the old sync authentication schema. But for now it is more important to get the whole tests executed.

Comment on attachment 8387572 [details] [diff] [review] FxAccount inclusion v1 [checked-in] Review of attachment 8387572 [details] [diff] [review]: ----------------------------------------------------------------- Richard, would you mind giving us feedback in regards how the fxaccount authentication has been done? I would appreciate your feedback.

Comment on attachment 8387572 [details] [diff] [review] FxAccount inclusion v1 [checked-in] Review of attachment 8387572 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, although I didn't try to understand whether the sync calls are correct. I'll go ahead and land this so we can see how it affects the CI; we can land follow-up patches if rnewman's feedback indicates we should. ::: services/sync/tps/extensions/tps/modules/fxaccounts.jsm @@ +21,5 @@ > + */ > +var FxAccountsHelper = { > + > + /** > + * Wrapper to synchronize the login of an user nit: s/an/a @@ +28,5 @@ > + * The email address for the account (utf8) > + * @param password > + * The user's password > + */ > + signIn : function signIn(email, password) { nit: extra space before the colon

Thanks Jonathan! I will fix those issues in a follow-up for refactoring. As landed I can see issues at http://brasstacks.mozilla.com/autolog/?tree=mozilla-inbound&source=autolog: TEST-UNEXPECTED-FAIL | [phase1] Exception caught: this.config.fx_account is undefined JS Stack trace: _executeTestPhase@tps.jsm:632:7 Have you re-created the virtual environment? It looks like it still uses the old config file.

Sorry, missed to add the keep open keyword. As mentioned above we still have to do a refactor to get both authentication methods working.

(In reply to Henrik Skupin (:whimboo) from comment #42) > Thanks Jonathan! I will fix those issues in a follow-up for refactoring. As > landed I can see issues at > http://brasstacks.mozilla.com/autolog/?tree=mozilla-inbound&source=autolog: > > TEST-UNEXPECTED-FAIL | [phase1] Exception caught: this.config.fx_account is > undefined JS Stack trace: _executeTestPhase@tps.jsm:632:7 > > Have you re-created the virtual environment? It looks like it still uses the > old config file. I just updated the config file used by the CI. It doesn't pick up config file changes automatically, so changes to that file have to be manually added.

FYI: CI may fail as there's a problem with the acct it's using: bug 979009 I don't think we need to work around the bug till later in the week. For now, I can run manually while we're sorting out the bugs.

(In reply to Edwin Wong [:edwong] from comment #46) > FYI: CI may fail as there's a problem with the acct it's using: bug 979009 I haven't seen this failure yet. But thanks for letting me know.

To get the CI properly working a couple of changes have to be made. For now i have filed issues, but will start working on them tomorrow. See: https://github.com/jonallengriffin/coversheet/issues

It appears to me that the original scope of work suggested by this bug is done (Great work Henrik!). Can we close this bug and use additional issues to track specific TPS failures?

The code as landed is not that clean and needs a follow-up as stated earlier. I still want to do this here which will also make it easier for us to get all backported. Lets keep the bug open for that work, which I'm working on right now.

The remaining patch will be ready tomorrow. Just an update here: * It will contain a config file setting to select the authentication method (fxaccounts vs. old sync) * We keep Mozmill in even we don't have tests for now, but any mozmill related test will only support fxaccounts. Doing it for both is kinda hard. * Lots and lots of refactoring

Sorry for that massive patch, but as mentioned earlier lot of code had to be changed to make the extension better manageable in the future. Jonathan, it would be great if you could do a review ASAP so we can get it landed before the merge on Monday. There is nothing I can test here via try given that all testing is done on the CI box or on developer machines.

Last patch was the git patch and not the hg version of it. Re-attaching now.

The code changes I have made in my initial patch here will apply on mozilla-aurora but will not work because the patch on bug 943521 has not been backported yet. So it is blocking me from any further work on mozilla-aurora.

Comment on attachment 8391090 [details] [diff] [review] Refactor TPS framework v1.1 Review of attachment 8391090 [details] [diff] [review]: ----------------------------------------------------------------- I'll land soon-ish. ::: testing/tps/README @@ +22,5 @@ > +something like: > + > + (linux): /path/to/virtualenv/lib/python2.6/site-packages/tps-0.2.40-py2.6.egg/tps/config.json > + (win): /path/to/virtualenv/Lib/site-packages/tps-0.2.40-py2.6.egg/tps/config.json > + The intention is that the user would create a copy of config.json.in and make whatever modifications to it that are needed, then pass the file to TPS with the --configfile CLI. If that's not working correctly, we should fix it; it's nice not to have to figure out the path to config.json in the virtualenv. ::: testing/tps/tps/testrunner.py @@ +313,5 @@ > if self.mobile: > self.preferences.update({'services.sync.client.type' : 'mobile'}) > > + # If sync accounts have been chosen, disable Firefox Accounts > + if self.config.get('auth_type', 'fx_account') != 'fx_account': config.json.in actually specified 'auth', rather than 'auth_type'. Can be fixed in a follow-up patch.

(In reply to Jonathan Griffin (:jgriffin) from comment #55) > ::: testing/tps/README > @@ +22,5 @@ > > +something like: > > + > > + (linux): /path/to/virtualenv/lib/python2.6/site-packages/tps-0.2.40-py2.6.egg/tps/config.json > > + (win): /path/to/virtualenv/Lib/site-packages/tps-0.2.40-py2.6.egg/tps/config.json > > + > > The intention is that the user would create a copy of config.json.in and > make whatever modifications to it that are needed, then pass the file to TPS > with the --configfile CLI. If that's not working correctly, we should fix > it; it's nice not to have to figure out the path to config.json in the > virtualenv. Yeah, I agree. For now the above is working, so I didn't wanted to change that behavior. > > + # If sync accounts have been chosen, disable Firefox Accounts > > + if self.config.get('auth_type', 'fx_account') != 'fx_account': > > config.json.in actually specified 'auth', rather than 'auth_type'. Can be > fixed in a follow-up patch. Good catch! I tested the patch with my own config.json, and missed to fix the template. I will fix that and land the patch on inbound. Thanks for your review!

Nits fixed and ready for landing.

Comment on attachment 8391451 [details] [diff] [review] Refactor TPS framework v2 [checked-in] https://hg.mozilla.org/integration/mozilla-inbound/rev/500bcf9f9d3a

The documentation has been updated: https://developer.mozilla.org/en-US/docs/TPS

Comment on attachment 8387572 [details] [diff] [review] FxAccount inclusion v1 [checked-in] [Approval Request Comment] Bug caused by (feature/regressing bug #): Switching to FxAccounts User impact if declined: Test automation via TPS does not work due to missing authentication Testing completed (on m-c, etc.): yes, for more than a week Risk to taking this patch (and alternatives if risky): None, all testing code String or IDL/UUID changes made by this patch: None

Comment on attachment 8391451 [details] [diff] [review] Refactor TPS framework v2 [checked-in] [Approval Request Comment] Bug caused by (feature/regressing bug #): Switching to FxAccounts User impact if declined: Test automation via TPS does not work for old sync authentication Testing completed (on m-c, etc.): yes, for about 6 days Risk to taking this patch (and alternatives if risky): None, all testing code String or IDL/UUID changes made by this patch: None

Comment on attachment 8387572 [details] [diff] [review] FxAccount inclusion v1 [checked-in] [Approval Request Comment] Bug caused by (feature/regressing bug #): Switching to FxAccounts User impact if declined: Test automation via TPS does not work due to missing authentication Testing completed (on m-c, etc.): yes, for more than a week Risk to taking this patch (and alternatives if risky): None, all testing code String or IDL/UUID changes made by this patch: None

Comment on attachment 8391451 [details] [diff] [review] Refactor TPS framework v2 [checked-in] [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or IDL/UUID changes made by this patch: Comment on attachment 8391451 [details] [diff] [review] [diff] [review] Refactor TPS framework v2 [checked-in] [Approval Request Comment] Bug caused by (feature/regressing bug #): Switching to FxAccounts User impact if declined: Test automation via TPS does not work for old sync authentication Testing completed (on m-c, etc.): yes, for about 6 days Risk to taking this patch (and alternatives if risky): None, all testing code String or IDL/UUID changes made by this patch: None

Applying the patch on beta caused a hunk failed to apply message for the readme file. This patch fixes it. No code changes included. Taking over r+ and a+.

https://hg.mozilla.org/releases/mozilla-beta/rev/036df66a5b06 FxAccount inclusion v1 https://hg.mozilla.org/releases/mozilla-beta/rev/33ed70de78c4 [beta] Refactor TPS framework (readme fixes)