Closed Bug 1229986 Opened 4 years ago Closed 4 years ago

Get TPS running correctly (but not necessarily passing!)

Categories

(Testing Graveyard :: TPS, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(2 files)

Running TPS locally found some interesting issues:
* It wouldn't start - it seems some of the dependencies are out of date.
* The FxA login process failed as it didn't get encryption keys due to a recent FxA change.
* Most tests failed early with an "unexpected sync", which appears to have been caused by the FxA login process automatically starting the first sync.
* There was a |let password| statement in a scope where password was already declared.
* The tps addon is not signed - setting a pref to avoid that works on Nightly.

Other tweaks not directly related to failing:
* Lots of log noise caused by Fx terminating during a sync - I think this was only in error cases, but I cleaned that up.
* DumpError() often didn't report a stack.

With these changes I get TPS running to completion with many tests passing. Many tests still fail for reasons that aren't clear, but I figured we should get these improvements in.
Attachment #8695053 - Flags: review?(hskupin)
Assignee: nobody → markh
Status: NEW → ASSIGNED
Comment on attachment 8695053 [details] [diff] [review]
0001-Bug-XXXXXXX-get-Sync-tps-tests-starting-again.-r-whi.patch

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

It was hard to get into this code again which I haven't touched for a very long time! But all the changes look reasonable to me at least from the TPS view. Maybe you want another review from a FxA developer also because of the one question I left for tps.jsm.

::: services/sync/tps/extensions/tps/resource/tps.jsm
@@ +133,5 @@
>        Cu.import("resource://tps/auth/sync.jsm", module);
>      }
>    },
>  
> +  DumpError(msg, exc = null) {

nit: I think there should be no spaces around the `=` operator also in JS.

@@ +142,5 @@
> +    } else {
> +      // always write a stack even if no error passed.
> +      errInfo = Utils.stackTrace(new Error());
> +    }
> +    Logger.logError(`[phase ${this._currentPhase}] ${msg} - ${errInfo}`);

Lovely! Haven't known that this is actually possible now.

@@ +886,5 @@
> +    // If fxaccounts is enabled we get an initial sync at login time - let
> +    // that complete.
> +    if (this.fxaccounts_enabled) {
> +      this._triggeredSync = true;
> +      this.waitForEvent("weave:service:sync:start");

Could this cause a race? How long is the delay between the successful login and the sync start event? If it is too short we might set _triggeredSync too late.

::: testing/tps/.gitignore
@@ +1,1 @@
> +# These files are added by running the TPS test suite.

Do we really need this file? We have a hg repository here.

::: testing/tps/tps/testrunner.py
@@ +72,5 @@
>          'services.sync.firstSync': 'notReady',
>          'services.sync.lastversion': '1.0',
>          'toolkit.startup.max_resumed_crashes': -1,
> +        # hrm - not sure what the release/beta channels will do?
> +        'xpinstall.signatures.required': False,

When it gets enabled again Firefox will refuse to install the TPS addon. See bug 1219446 for alternatives.
Attachment #8695053 - Flags: review?(hskupin) → review+
Thanks for the quick review! A couple of clarifications:

(In reply to Henrik Skupin (:whimboo) from comment #2)
> nit: I think there should be no spaces around the `=` operator also in JS.

Although the style guide doesn't explicitly say so, the rule we use in JS is that operators *always* get spaces. I also just re-checked in #fx-team, and that was indeed the consensus. It took me quite some time to get out of the Python habit ;)

> @@ +886,5 @@
> > +    // If fxaccounts is enabled we get an initial sync at login time - let
> > +    // that complete.
> > +    if (this.fxaccounts_enabled) {
> > +      this._triggeredSync = true;
> > +      this.waitForEvent("weave:service:sync:start");
> 
> Could this cause a race? How long is the delay between the successful login
> and the sync start event? If it is too short we might set _triggeredSync too
> late.

This will work OK, as I also removed the nextTick call from waitForEvent - so because observers are synchronous, there's no risk of other stuff happening. (I removed that nextTick call for exactly that reason - stuff got way ahead of us)

> Do we really need this file? We have a hg repository here.

I use git :) There are already 22 .gitignore files in the tree.

Thanks!
(In reply to Mark Hammond [:markh] from comment #3)
> > Could this cause a race? How long is the delay between the successful login
> > and the sync start event? If it is too short we might set _triggeredSync too
> > late.
> 
> This will work OK, as I also removed the nextTick call from waitForEvent -
> so because observers are synchronous, there's no risk of other stuff
> happening. (I removed that nextTick call for exactly that reason - stuff got
> way ahead of us)

Ah, great. So all fine then. Thanks!

> > Do we really need this file? We have a hg repository here.
> 
> I use git :) There are already 22 .gitignore files in the tree.

Maybe that should then be listed as exclusions in a .gitignore higher up in the tree. Those entries will apply to lot of folders in /testing at least.
(In reply to Henrik Skupin (:whimboo) from comment #5)
> Maybe that should then be listed as exclusions in a .gitignore higher up in
> the tree. Those entries will apply to lot of folders in /testing at least.

sgtm! I can't see harm in having those patterns in the root.
https://hg.mozilla.org/mozilla-central/rev/6794f4bf3f8e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.