Closed Bug 966434 Opened 6 years ago Closed 6 years ago

Update TPS to use FxA

Categories

(Testing Graveyard :: TPS, defect, P1)

defect

Tracking

(firefox29+ fixed, firefox30+ fixed)

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 + fixed
firefox30 + fixed

People

(Reporter: edwong, Assigned: whimboo)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [qa+][backport for firefox-29 blocked by bug 943521])

Attachments

(3 files, 3 obsolete files)

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.
Component: Firefox Sync: UI → Server: Sync
OS: Mac OS X → All
Hardware: x86 → All
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.
Component: Server: Sync → Firefox Sync: Backend
Whiteboard: [qa+]
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.
Assignee: nobody → hskupin
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.
Blocks: 905997
Flags: needinfo?(jparsons)
Assignee: hskupin → nobody
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.
Flags: needinfo?(jparsons) → needinfo?(gavin.sharp)
Blocks: 969593
No longer blocks: 905997
Mark, Chris, any easy fixes here?

I don't know anything about TPS. rnewman, do you have any advice?
Flags: needinfo?(gavin.sharp)
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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
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.
Depends on: 979815
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...
Priority: -- → P1
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?
Flags: needinfo?(warner-bugzilla)
: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?
Flags: needinfo?(warner-bugzilla) → needinfo?(rnewman)
Just use it as inspiration. TPS's account setup should be fairly straightforward inlined code.
Flags: needinfo?(rnewman)
(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?
Flags: needinfo?(jgriffin)
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).
Flags: needinfo?(jgriffin)
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.
Attached patch WIP v0.1 (obsolete) — Splinter Review
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.
Attachment #8387011 - Attachment is obsolete: true
Attachment #8387572 - Flags: review?(jgriffin)
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.
Attachment #8387572 - Flags: feedback?(rnewman)
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
Attachment #8387572 - Flags: review?(jgriffin) → review+
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.
Flags: needinfo?(jgriffin)
https://hg.mozilla.org/mozilla-central/rev/947e70a10659
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Sorry, missed to add the keep open keyword. As mentioned above we still have to do a refactor to get both authentication methods working.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
(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.
Flags: needinfo?(jgriffin)
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?
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 982591
Blocks: 982591
No longer depends on: 982591
Blocks: 982610
Blocks: 983071
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
Blocks: 981836
Blocks: 983565
Attachment #8387572 - Attachment description: Patch v1 → Patch FxAccount inclusion v1 [checked-in]
Attachment #8387572 - Flags: feedback?(rnewman)
Attached patch Refactor TPS framework (obsolete) — Splinter Review
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.
Attachment #8391089 - Flags: review?(jgriffin)
Attached patch Refactor TPS framework v1.1 (obsolete) — Splinter Review
Last patch was the git patch and not the hg version of it. Re-attaching now.
Attachment #8391089 - Attachment is obsolete: true
Attachment #8391089 - Flags: review?(jgriffin)
Attachment #8391090 - Flags: review?(jgriffin)
Status: REOPENED → ASSIGNED
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.
Depends on: 943521
Whiteboard: [qa+] → [qa+][backport for firefox-29 blocked by bug 943521]
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.
Attachment #8391090 - Flags: review?(jgriffin) → review+
(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.
Attachment #8391090 - Attachment is obsolete: true
Attachment #8391451 - Flags: review+
Comment on attachment 8391451 [details] [diff] [review]
Refactor TPS framework v2 [checked-in]

https://hg.mozilla.org/integration/mozilla-inbound/rev/500bcf9f9d3a
Attachment #8391451 - Attachment description: Refactor TPS framework v2 → Refactor TPS framework v2 [checked-in]
https://hg.mozilla.org/mozilla-central/rev/500bcf9f9d3a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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
Attachment #8387572 - Attachment description: Patch FxAccount inclusion v1 [checked-in] → FxAccount inclusion v1 [checked-in]
Attachment #8387572 - Flags: approval-mozilla-aurora?
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
Attachment #8391451 - Flags: approval-mozilla-aurora?
Attachment #8387572 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8391451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8387572 - Flags: approval-mozilla-beta?
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
Attachment #8391451 - Flags: approval-mozilla-beta?
Attachment #8387572 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8391451 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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+.
Attachment #8393420 - Flags: review+
Component: Firefox Sync: Backend → TPS
Product: Mozilla Services → Testing
Depends on: 996027
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.