Bug 1350135 - Fix failing TPS test_history_collision.js test r=markh
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Wed, 19 Apr 2017 14:38:00 -0400
changeset 354060 51bf9d43200f6860029ff8bb0166fefd631ce09b
parent 354059 e6cd1cb7eaa9dbafeb921209d1f710ff387e67a9
child 354061 816a2cc1254aff32e4f38b9aa49bf0e18c85a045
push id31685
push userkwierso@gmail.com
push dateThu, 20 Apr 2017 21:45:29 +0000
treeherdermozilla-central@5e3dc7e1288a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1350135
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1350135 - Fix failing TPS test_history_collision.js test r=markh There were three issues here: The first is that the TPS's history module didn't wait for PlacesUtils.history.remove's promise to resolve. The second is that the SYNC_WIPE_REMOTE in the previous client would cause a write to the clients collection, which would cause the next client to get a "sync:collection_changed" push. This caused a sync of *only* the clients collection upon reciept, which would prevent TPS from explicitly syncing all engines. The third is that TPS wasn't correctly handling the cases where logIn would trigger a sync, which would cause a failure during the first sync of a session. This would cause failures on other TPS tests as well. MozReview-Commit-ID: LpqZ7Kt9fyy
services/sync/modules/service.js
services/sync/tps/extensions/tps/resource/modules/history.jsm
services/sync/tps/extensions/tps/resource/tps.jsm
testing/tps/tps/testrunner.py
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -398,17 +398,20 @@ Sync11Service.prototype = {
 
   // nsIObserver
 
   observe: function observe(subject, topic, data) {
     switch (topic) {
       // Ideally this observer should be in the SyncScheduler, but it would require
       // some work to know about the sync specific engines. We should move this there once it does.
       case "sync:collection_changed":
-        if (data.includes("clients")) {
+        // We check if we're running TPS here to avoid TPS failing because it
+        // couldn't get to get the sync lock, due to us currently syncing the
+        // clients engine.
+        if (data.includes("clients") && !Svc.Prefs.get("testing.tps", false)) {
           this.sync([]); // [] = clients collection only
         }
         break;
       case "weave:service:setup-complete":
         let status = this._checkSetup();
         if (status != STATUS_DISABLED && status != CLIENT_NOT_CONFIGURED)
             Svc.Obs.notify("weave:engine:start-tracking");
         break;
--- a/services/sync/tps/extensions/tps/resource/modules/history.jsm
+++ b/services/sync/tps/extensions/tps/resource/modules/history.jsm
@@ -170,17 +170,17 @@ var HistoryEntry = {
    *
    * @param item An object representing items to delete
    * @param usSinceEpoch The number of microseconds from Epoch to
    *        the time the current Crossweave run was started
    * @return nothing
    */
   Delete(item, usSinceEpoch) {
     if ("uri" in item) {
-      PlacesUtils.history.remove(item.uri);
+      Async.promiseSpinningly(PlacesUtils.history.remove(item.uri));
     } else if ("host" in item) {
       PlacesUtils.history.removePagesFromHost(item.host, false);
     } else if ("begin" in item && "end" in item) {
       let cb = Async.makeSpinningCallback();
       let msSinceEpoch = parseInt(usSinceEpoch / 1000);
       let filter = {
         beginDate: new Date(msSinceEpoch + (item.begin * 60 * 60 * 1000)),
         endDate: new Date(msSinceEpoch + (item.end * 60 * 60 * 1000))
--- a/services/sync/tps/extensions/tps/resource/tps.jsm
+++ b/services/sync/tps/extensions/tps/resource/tps.jsm
@@ -264,17 +264,25 @@ var TPS = {
     Weave.Svc.Prefs.set("syncThreshold", 10000000);
   },
 
   StartAsyncOperation: function TPS__StartAsyncOperation() {
     this._operations_pending++;
   },
 
   FinishAsyncOperation: function TPS__FinishAsyncOperation() {
-    this._operations_pending--;
+    // We fire a FinishAsyncOperation at the end of a sync finish (somewhat
+    // dubious, but we're consistent about it), but a sync may or may not be
+    // triggered during login, and it's hard for us to know (without e.g.
+    // auth/fxaccounts.jsm calling back into this module). So we just assume
+    // the FinishAsyncOperation without a StartAsyncOperation is fine, and works
+    // as if a StartAsyncOperation had been called.
+    if (this._operations_pending) {
+      this._operations_pending--;
+    }
     if (!this.operations_pending) {
       this._currentAction++;
       Utils.nextTick(function() {
         this.RunNextTestAction();
       }, this);
     }
   },
 
@@ -1126,34 +1134,39 @@ var TPS = {
   /**
    * Login on the server
    */
   Login: function Login(force) {
     if (Authentication.isLoggedIn && !force) {
       return;
     }
 
+    // This might come during Authentication.signIn
+    this._triggeredSync = true;
     Logger.logInfo("Setting client credentials and login.");
     Authentication.signIn(this.config.fx_account);
     this.waitForSetupComplete();
     Logger.AssertEqual(Weave.Status.service, Weave.STATUS_OK, "Weave status OK");
     this.waitForTracking();
-    // We get an initial sync at login time - let that complete.
-    this._triggeredSync = true;
+    // We might get an initial sync at login time - let that complete.
     this.waitForSyncFinished();
   },
 
   /**
    * Triggers a sync operation
    *
    * @param {String} [wipeAction]
    *        Type of wipe to perform (resetClient, wipeClient, wipeRemote)
    *
    */
   Sync: function TPS__Sync(wipeAction) {
+    if (this._syncActive) {
+      Logger.logInfo("WARNING: Sync currently active! Waiting, before triggering another");
+      this.waitForSyncFinished();
+    }
     Logger.logInfo("Executing Sync" + (wipeAction ? ": " + wipeAction : ""));
 
     // Force a wipe action if requested. In case of an initial sync the pref
     // will be overwritten by Sync itself (see bug 992198), so ensure that we
     // also handle it via the "weave:service:setup-complete" notification.
     if (wipeAction) {
       this._syncWipeAction = wipeAction;
       Weave.Svc.Prefs.set("firstSync", wipeAction);
--- a/testing/tps/tps/testrunner.py
+++ b/testing/tps/tps/testrunner.py
@@ -71,16 +71,17 @@ class TPSTestRunner(object):
         'extensions.update.enabled': False,
         # Don't open a dialog to show available add-on updates
         'extensions.update.notifyUser': False,
         '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,
+        'services.sync.testing.tps': True,
     }
 
     debug_preferences = {
         'services.sync.log.appender.console': 'Trace',
         'services.sync.log.appender.dump': 'Trace',
         'services.sync.log.appender.file.level': 'Trace',
         'services.sync.log.appender.file.logOnSuccess': True,
         'services.sync.log.rootLogger': 'Trace',