708965 - TPS enhancements around Sync data handling and test setup

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Attachment: TPS enhancements, v1

As part of developing the TPS tests for add-on sync, I refactored TPS's handling of the special syncs that occur at the beginning and end of a single TPS test.

Changes made:

* WIPE_SERVER renamed to WIPE_REMOTE because it was actually calling Service.wipeRemote()
* Refactor login() logic into a standalone function. It only does work when it needs to instead of every time.
* Implement ResetData() action which performs a wipeServer + local client data wipe and logs in again. This simulates a fresh Sync experience.
* Invoke ResetData() automagically at the beginning of the first test phase
* Modify existing tests and remove now uncessary call to WIPE_SERVER during first Sync.
* Fix bug around handling of cleanup wipeServer that could result in inappropriate scheduling.
* Log the current phase progress better.
* Misc style and nits improvements.
* A misc add-ons change snuck in. I'm too lazy to split it out of the patch. The add-ons code isn't active, so the change should be harmless. This patch is a prereq for add-on sync, so it needs to land sometime.

Comment 1

[Jonathan Griffin (:jgriffin)]

Comment on attachment 580323 [details] [diff] [review]
TPS enhancements, v1

Thanks for the cleanup!

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 580323 [details] [diff] [review]
TPS enhancements, v1

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

If jgriffin's happy, I'm happy. But please give a little thought to forensics here.

::: services/sync/tps/extensions/tps/modules/tps.jsm
@@ +528,5 @@
>        Logger.logInfo("setting client.name to " + this.phases["phase" + this._currentPhase]);
>        Weave.Svc.Prefs.set("client.name", this.phases["phase" + this._currentPhase]);
>  
> +      // TODO Phases should be defined in a data strongly that has strong
> +      // ordering, not by lexical sorting.

English, please :D

@@ +535,5 @@
> +      if (currentPhase <= 1) {
> +        this_phase.unshift([this.ResetData]);
> +      }
> +
> +      // Wipe the server at the end of the final test phase.

I'm a little leery of the impact of this on our ability to investigate failures.

If you wipe, do it at the start of a test run, not the end?

Comment 3

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #2)

> @@ +535,5 @@
> > +      if (currentPhase <= 1) {
> > +        this_phase.unshift([this.ResetData]);
> > +      }
> > +
> > +      // Wipe the server at the end of the final test phase. 
> I'm a little leery of the impact of this on our ability to investigate
> failures.
> 
> If you wipe, do it at the start of a test run, not the end?

The current behavior is to perform a a wipeRemote() before *and* after test runs. I agree we can probably eliminate the after case. But, I think that is in the realm of a follow-up bug where other TPS nits (like the lexical ordering of phases) is addressed.

Comment 4

[Gregory Szorc [:gps]]

Pushed to s-c: https://hg.mozilla.org/services/services-central/rev/cfd838426cf2

Comment 5

[Gregory Szorc [:gps]]

Pushed to m-c: https://hg.mozilla.org/mozilla-central/rev/cfd838426cf2