Bug 1488899 - Get TPS working. r=tcsc

This patch disables a couple of tests, removes old async support from TPS, and puts more effort into preventing Syncs while TPS is running, and removed an unused import. Differential Revision: https://phabricator.services.mozilla.com/D8692 --HG-- extra : moz-landing-system : lando
2018-10-15 22:42:22 +00:00 · 2018-10-15 22:42:22 +00:00 · c8597f4ec4
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@ -256,7 +256,9 @@ this.BrowserIDManager.prototype = {
      }
      // There's no need to wait for sync to complete and it would deadlock
      // our AsyncObserver.
-      Weave.Service.sync({why: "login"});
+      if (!Svc.Prefs.get("testing.tps", false)) {
+        Weave.Service.sync({why: "login"});
+      }
      break;
    }

--- a/services/sync/tests/tps/.eslintrc.js
+++ b/services/sync/tests/tps/.eslintrc.js
@ -27,5 +27,15 @@ module.exports = {
    "Tabs": false,
    "Windows": false,
    "WipeServer": false,
-  }
+  },
+  // TPS test files are also hackily parsed by python's JSON parser,
+  // so trailing commas aren't valid.
+  "overrides": [{
+    "files": [
+      "test_*.js",
+    ],
+    "rules": {
+      "comma-dangle": "off",
+    }
+  }]
 };
--- a/services/sync/tests/tps/all_tests.json
+++ b/services/sync/tests/tps/all_tests.json
@ -1,34 +1,34 @@
-{ "tests": [
-    "test_bookmark_conflict.js",
-    "test_sync.js",
-    "test_prefs.js",
-    "test_tabs.js",
-    "test_passwords.js",
-    "test_history.js",
-    "test_formdata.js",
-    "test_bug530717.js",
-    "test_bug531489.js",
-    "test_bug538298.js",
-    "test_bug556509.js",
-    "test_bug562515.js",
-    "test_bug535326.js",
-    "test_bug501528.js",
-    "test_bug575423.js",
-    "test_bug546807.js",
-    "test_history_collision.js",
-    "test_privbrw_passwords.js",
-    "test_privbrw_tabs.js",
-    "test_bookmarks_in_same_named_folder.js",
-    "test_client_wipe.js",
-    "test_special_tabs.js",
-    "test_addon_restartless_xpi.js",
-    "test_addon_webext_xpi.js",
-    "test_addon_reconciling.js",
-    "test_addon_wipe.js",
-    "test_existing_bookmarks.js",
-    "test_addresses.js",
-    "test_creditcards.js"
-  ]
+{ "tests": {
+    "test_bookmark_conflict.js": {},
+    "test_sync.js": {},
+    "test_prefs.js": {},
+    "test_tabs.js": {},
+    "test_passwords.js": {},
+    "test_history.js": {},
+    "test_formdata.js": {},
+    "test_bug530717.js": {},
+    "test_bug531489.js": {},
+    "test_bug538298.js": {},
+    "test_bug556509.js": {},
+    "test_bug562515.js": {},
+    "test_bug535326.js": {},
+    "test_bug501528.js": {},
+    "test_bug575423.js": {},
+    "test_bug546807.js": {},
+    "test_history_collision.js": {},
+    "test_privbrw_passwords.js": {},
+    "test_privbrw_tabs.js": {},
+    "test_bookmarks_in_same_named_folder.js": {},
+    "test_client_wipe.js": {},
+    "test_special_tabs.js": {},
+    "test_addon_restartless_xpi.js": {"disabled": "Bug 1498974"},
+    "test_addon_webext_xpi.js": {"disabled": "Bug 1498974"},
+    "test_addon_reconciling.js": {"disabled": "Bug 1498974"},
+    "test_addon_wipe.js": {},
+    "test_existing_bookmarks.js": {},
+    "test_addresses.js": {},
+    "test_creditcards.js": {}
+  }
 }


--- a/services/sync/tests/tps/test_addon_reconciling.js
+++ b/services/sync/tests/tps/test_addon_reconciling.js
@ -12,7 +12,7 @@ var phases = {
  "phase03": "profile1",
  "phase04": "profile2",
  "phase05": "profile1",
-  "phase06": "profile2",
+  "phase06": "profile2"
 };

 const id = "restartless-xpi@tests.mozilla.org";
--- a/services/sync/tests/tps/test_addon_restartless_xpi.js
+++ b/services/sync/tests/tps/test_addon_restartless_xpi.js
@ -13,7 +13,7 @@ var phases = {
  "phase05": "profile1",
  "phase06": "profile2",
  "phase07": "profile1",
-  "phase08": "profile2",
+  "phase08": "profile2"
 };

 const id = "restartless-xpi@tests.mozilla.org";
--- a/services/sync/tests/tps/test_addon_webext_xpi.js
+++ b/services/sync/tests/tps/test_addon_webext_xpi.js
@ -14,7 +14,7 @@ var phases = {
  "phase05": "profile1",
  "phase06": "profile2",
  "phase07": "profile1",
-  "phase08": "profile2",
+  "phase08": "profile2"
 };

 const id = "test-webext@quality.mozilla.org";
--- a/services/sync/tests/tps/test_addon_wipe.js
+++ b/services/sync/tests/tps/test_addon_wipe.js
@ -11,7 +11,7 @@ EnableEngines(["addons"]);
 var phases = {
  "phase01": "profile1",
  "phase02": "profile1",
-  "phase03": "profile1",
+  "phase03": "profile1"
 };

 const id1 = "restartless-xpi@tests.mozilla.org";
--- a/services/sync/tests/tps/test_addresses.js
+++ b/services/sync/tests/tps/test_addresses.js
@ -9,7 +9,7 @@ EnableEngines(["addresses"]);
 var phases = {
  "phase1": "profile1",
  "phase2": "profile2",
-  "phase3": "profile1",
+  "phase3": "profile1"
 };

 const address1 = [{
--- a/services/sync/tests/tps/test_creditcards.js
+++ b/services/sync/tests/tps/test_creditcards.js
@ -9,7 +9,7 @@ EnableEngines(["creditcards"]);
 var phases = {
  "phase1": "profile1",
  "phase2": "profile2",
-  "phase3": "profile1",
+  "phase3": "profile1"
 };

 const cc1 = [{
--- a/services/sync/tests/tps/test_tabs.js
+++ b/services/sync/tests/tps/test_tabs.js
@ -18,7 +18,6 @@ var phases = { "phase1": "profile1",

 var tabs1 = [
  { uri: "https://www.mozilla.org/en-US/firefox/",
-    title: "Firefox",
    profile: "profile1",
  },
  { uri: "data:text/html,<html><head><title>Hello</title></head><body>Hello</body></html>",
@ -29,7 +28,6 @@ var tabs1 = [

 var tabs2 = [
  { uri: "https://www.mozilla.org/en-US/contribute/",
-    title: "Get Involved",
    profile: "profile2",
  },
  { uri: "data:text/html,<html><head><title>Bye</title></head><body>Bye</body></html>",
--- a/services/sync/tps/extensions/tps/api.js
+++ b/services/sync/tps/extensions/tps/api.js
@ -42,6 +42,7 @@ async function tpsStartup() {
      // Note: This calls quit() under the hood
      TPS.DumpError("Test initialization failed", e);
    }
+    dump(`TPS test initialization failed: ${e} - ${e.stack}\n`);
    // Try and quit right away, no reason to wait around for python
    // to kill us if initialization failed.
    Services.startup.quit(Ci.nsIAppStartup.eForceQuit);
--- a/services/sync/tps/extensions/tps/resource/modules/tabs.jsm
+++ b/services/sync/tps/extensions/tps/resource/modules/tabs.jsm
@ -40,7 +40,7 @@ var BrowserTabs = {
  async Add(uri) {
    let mainWindow = Services.wm.getMostRecentWindow("navigator:browser");
    let browser = mainWindow.getBrowser();
-    let newtab = browser.addTab(uri);
+    let newtab = browser.addTrustedTab(uri);

    // Wait for the tab to load.
    await new Promise(resolve => {
@ -84,9 +84,10 @@ var BrowserTabs = {
            return true;
          }
        }
-        }
-        Logger.logInfo(`Dumping tabs for ${client.clientName}...\n` + JSON.stringify(client.tabs));
      }
+      Logger.logInfo(`Dumping tabs for ${client.name}...\n` +
+                     JSON.stringify(tabClient.tabs, null, 2));
+    }
    return false;
  },
 };
--- a/services/sync/tps/extensions/tps/resource/modules/windows.jsm
+++ b/services/sync/tps/extensions/tps/resource/modules/windows.jsm
@ -22,10 +22,12 @@ var BrowserWindows = {
   * @return nothing
   */
  Add(aPrivate, fn) {
-    let mainWindow = Services.wm.getMostRecentWindow("navigator:browser");
-    let win = mainWindow.OpenBrowserWindow({private: aPrivate});
-    win.addEventListener("load", function() {
-      fn.call(win);
-    }, {once: true});
+    return new Promise(resolve => {
+      let mainWindow = Services.wm.getMostRecentWindow("navigator:browser");
+      let win = mainWindow.OpenBrowserWindow({private: aPrivate});
+      win.addEventListener("load", function() {
+        resolve(win);
+      }, {once: true});
+    });
  },
 };
--- a/services/sync/tps/extensions/tps/resource/tps.jsm
+++ b/services/sync/tps/extensions/tps/resource/tps.jsm
@ -16,7 +16,6 @@ var EXPORTED_SYMBOLS = [
 var module = this;

 // Global modules
-ChromeUtils.import("resource://formautofill/FormAutofillSync.jsm");
 ChromeUtils.import("resource://gre/modules/Log.jsm");
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.import("resource://gre/modules/Services.jsm");
@ -26,6 +25,7 @@ ChromeUtils.import("resource://gre/modules/FileUtils.jsm");
 ChromeUtils.import("resource://gre/modules/Timer.jsm");
 ChromeUtils.import("resource://gre/modules/PromiseUtils.jsm");
 ChromeUtils.import("resource://gre/modules/osfile.jsm");
+ChromeUtils.import("resource:///modules/sessionstore/SessionStore.jsm");
 ChromeUtils.import("resource://services-common/async.js");
 ChromeUtils.import("resource://services-common/utils.js");
 ChromeUtils.import("resource://services-sync/constants.js");
@ -95,7 +95,6 @@ const ACTIONS = [
 const OBSERVER_TOPICS = ["fxaccounts:onlogin",
                         "fxaccounts:onlogout",
                         "profile-before-change",
-                         "sessionstore-windows-restored",
                         "weave:service:tracking-started",
                         "weave:service:tracking-stopped",
                         "weave:service:login:error",
@ -114,7 +113,6 @@ var TPS = {
  _enabledEngines: null,
  _errors: 0,
  _isTracking: false,
-  _operations_pending: 0,
  _phaseFinished: false,
  _phaselist: {},
  _setupComplete: false,
@ -133,7 +131,6 @@ var TPS = {
  shouldValidateBookmarks: false,
  shouldValidatePasswords: false,
  shouldValidateForms: false,
-  _windowsUpDeferred: PromiseUtils.defer(),
  _placesInitDeferred: PromiseUtils.defer(),

  _init: function TPS__init() {
@ -145,6 +142,16 @@ var TPS = {

    /* global Authentication */
    ChromeUtils.import("resource://tps/auth/fxaccounts.jsm", module);
+
+    // Some engines bump their score during their sync, which then causes
+    // another sync immediately (notably, prefs and addons). We don't want
+    // this to happen, and there's no obvious preference to kill it - so
+    // we do this nasty hack to ensure the global score is always zero.
+    Services.prefs.addObserver("services.sync.globalScore", () => {
+      if (Weave.Service.scheduler.globalScore != 0) {
+        Weave.Service.scheduler.globalScore = 0;
+      }
+    });
  },

  DumpError(msg, exc = null) {
@ -177,10 +184,6 @@ var TPS = {

          break;

-        case "sessionstore-windows-restored":
-          this._windowsUpDeferred.resolve();
-          break;
-
        case "places-browser-init-complete":
          this._placesInitDeferred.resolve();
          break;
@ -223,13 +226,6 @@ var TPS = {
          this._triggeredSync = false;

          this.delayAutoSync();
-
-          // Wait a second before continuing, otherwise we can get
-          // 'sync not complete' errors.
-          CommonUtils.namedTimer(function() {
-            this.FinishAsyncOperation();
-          }, 1000, this, "postsync");
-
          break;

        case "weave:service:sync:start":
@ -256,7 +252,7 @@ var TPS = {
  },

  /**
-   * Given that we cannot complely disable the automatic sync operations, we
+   * Given that we cannot completely disable the automatic sync operations, we
   * massively delay the next sync. Sync operations have to only happen when
   * directly called via TPS.Sync()!
   */
@ -267,44 +263,18 @@ var TPS = {
    Weave.Svc.Prefs.set("syncThreshold", 10000000);
  },

-  StartAsyncOperation: function TPS__StartAsyncOperation() {
-    this._operations_pending++;
-  },
-
-  FinishAsyncOperation: function TPS__FinishAsyncOperation() {
-    // 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++;
-      CommonUtils.nextTick(function() {
-        this.RunNextTestAction().catch(err => {
-          this.DumpError("RunNextTestActionFailed", err);
-        });
-      }, this);
-    }
-  },
-
  quit: function TPS__quit() {
+    Logger.logInfo("quitting");
    this._requestedQuit = true;
    this.goQuitApplication();
  },

-  HandleWindows(aWindow, action) {
+  async HandleWindows(aWindow, action) {
    Logger.logInfo("executing action " + action.toUpperCase() +
                   " on window " + JSON.stringify(aWindow));
    switch (action) {
      case ACTION_ADD:
-        BrowserWindows.Add(aWindow.private, win => {
-          Logger.logInfo("window finished loading");
-          this.FinishAsyncOperation();
-        });
+        await BrowserWindows.Add(aWindow.private);
        break;
    }
    Logger.logPass("executing action " + action.toUpperCase() + " on windows");
@ -784,9 +754,11 @@ var TPS = {
  },

  async RunNextTestAction() {
+    Logger.logInfo("Running next test action");
    try {
      if (this._currentAction >= this._phaselist[this._currentPhase].length) {
        // Run necessary validations and then finish up
+        Logger.logInfo("No more actions - running validations...");
        if (this.shouldValidateBookmarks) {
          await this.ValidateBookmarks();
        }
@ -826,11 +798,6 @@ var TPS = {
      Logger.logInfo("starting action: " + action[0].name);
      await action[0].apply(this, action.slice(1));

-      // if we're in an async operation, don't continue on to the next action
-      if (this._operations_pending) {
-        return;
-      }
-
      this._currentAction++;
    } catch (e) {
      if (Async.isShutdownException(e)) {
@ -1017,7 +984,7 @@ var TPS = {

      // start processing the test actions
      this._currentAction = 0;
-      await this._windowsUpDeferred.promise;
+      await SessionStore.promiseAllWindowsRestored;
      await this.RunNextTestAction();
    } catch (e) {
      this.DumpError("_executeTestPhase failed", e);
@ -1079,8 +1046,9 @@ var TPS = {
   */
  Phase: function Test__Phase(phasename, fnlist) {
    if (Object.keys(this._phaselist).length === 0) {
-      // This is the first phase, add that we need to login.
-      fnlist.unshift([this.Login]);
+      // This is the first phase we should wipe the server - this has the
+      // side-effect of forcing a login, which we also need.
+      fnlist.unshift([this.WipeServer]);
    }
    this._phaselist[phasename] = fnlist;
  },
@ -1167,7 +1135,7 @@ var TPS = {
   * Waits for Sync to be finished before returning
   */
  async waitForSyncFinished() {
-    if (this._syncActive) {
+    if (Weave.Service.locked) {
      await this.waitForEvent("weave:service:resyncs-finished");
    }
  },
@ -1184,20 +1152,16 @@ var TPS = {
  /**
   * Login on the server
   */
-  async Login(force) {
-    if ((await Authentication.isReady()) && !force) {
+  async Login() {
+    if ((await Authentication.isReady())) {
      return;
    }

-    // This might come during Authentication.signIn
-    this._triggeredSync = true;
    Logger.logInfo("Setting client credentials and login.");
    await Authentication.signIn(this.config.fx_account);
    await this.waitForSetupComplete();
    Logger.AssertEqual(Weave.Status.service, Weave.STATUS_OK, "Weave status OK");
    await this.waitForTracking();
-    // We might get an initial sync at login time - let that complete.
-    await this.waitForSyncFinished();
  },

  /**
@ -1209,8 +1173,8 @@ var TPS = {
   */
  async Sync(wipeAction) {
    if (this._syncActive) {
-      Logger.logInfo("WARNING: Sync currently active! Waiting, before triggering another");
-      await this.waitForSyncFinished();
+      this.DumpError("Sync currently active which should be impossible");
+      return;
    }
    Logger.logInfo("Executing Sync" + (wipeAction ? ": " + wipeAction : ""));

@ -1225,20 +1189,25 @@ var TPS = {
    }
    if (!await Weave.Service.login()) {
      // We need to complete verification.
-      await this.Login(false);
+      Logger.logInfo("Logging in before performing sync");
+      await this.Login();
    }
    ++this._syncCount;

+    Logger.logInfo("Executing Sync" + (wipeAction ? ": " + wipeAction : ""));
    this._triggeredSync = true;
-    this.StartAsyncOperation();
    await Weave.Service.sync();
    Logger.logInfo("Sync is complete");
+    // wait a second for things to settle...
+    await new Promise(resolve => {
+      CommonUtils.namedTimer(resolve, 1000, this, "postsync");
+    });
  },

  async WipeServer() {
    Logger.logInfo("Wiping data from server.");

-    await this.Login(false);
+    await this.Login();
    await Weave.Service.login();
    await Weave.Service.wipeServer();
  },
@ -1247,7 +1216,7 @@ var TPS = {
   * Action which ensures changes are being tracked before returning.
   */
  async EnsureTracking() {
-    await this.Login(false);
+    await this.Login();
    await this.waitForTracking();
  },
 };
@ -1403,9 +1372,8 @@ var Tabs = {
 };

 var Windows = {
-  add: function Window__add(aWindow) {
-    TPS.StartAsyncOperation();
-    TPS.HandleWindows(aWindow, ACTION_ADD);
+  async add(aWindow) {
+    await TPS.HandleWindows(aWindow, ACTION_ADD);
  },
 };

--- a/testing/tps/create_venv.py
+++ b/testing/tps/create_venv.py
@ -105,6 +105,10 @@ def update_configfile(source, target, replacements):

 def main():
    parser = optparse.OptionParser('Usage: %prog [options] path_to_venv')
+    parser.add_option('--keep-config',
+                      dest='keep_config',
+                      action='store_true',
+                      help='Keep the existing config file.')
    parser.add_option('--password',
                      type='string',
                      dest='password',
@ -170,21 +174,22 @@ def main():
        testdir = os.path.join(here, 'tests')
        extdir = os.path.join(here, 'extensions')

-    update_configfile(os.path.join(here, 'config', 'config.json.in'),
-                      os.path.join(target, 'config.json'),
-                      replacements={
-                      '__TESTDIR__': testdir.replace('\\','/'),
-                      '__EXTENSIONDIR__': extdir.replace('\\','/'),
-                      '__FX_ACCOUNT_USERNAME__': options.username,
-                      '__FX_ACCOUNT_PASSWORD__': options.password,
-                      '__SYNC_ACCOUNT_USERNAME__': options.sync_username,
-                      '__SYNC_ACCOUNT_PASSWORD__': options.sync_password,
-                      '__SYNC_ACCOUNT_PASSPHRASE__': options.sync_passphrase})
+    if not options.keep_config:
+        update_configfile(os.path.join(here, 'config', 'config.json.in'),
+                          os.path.join(target, 'config.json'),
+                          replacements={
+                          '__TESTDIR__': testdir.replace('\\','/'),
+                          '__EXTENSIONDIR__': extdir.replace('\\','/'),
+                          '__FX_ACCOUNT_USERNAME__': options.username,
+                          '__FX_ACCOUNT_PASSWORD__': options.password,
+                          '__SYNC_ACCOUNT_USERNAME__': options.sync_username,
+                          '__SYNC_ACCOUNT_PASSWORD__': options.sync_password,
+                          '__SYNC_ACCOUNT_PASSPHRASE__': options.sync_passphrase})

-    if not (options.username and options.password):
-        print '\nFirefox Account credentials not specified.'
-    if not (options.sync_username and options.sync_password and options.passphrase):
-        print '\nFirefox Sync account credentials not specified.'
+        if not (options.username and options.password):
+            print '\nFirefox Account credentials not specified.'
+        if not (options.sync_username and options.sync_password and options.passphrase):
+            print '\nFirefox Sync account credentials not specified.'

    # Print the user instructions
    print usage_message.format(TARGET=target,
--- a/testing/tps/tps/testrunner.py
+++ b/testing/tps/tps/testrunner.py
@ -74,6 +74,7 @@ class TPSTestRunner(object):
        'extensions.update.notifyUser': False,
        'services.sync.firstSync': 'notReady',
        'services.sync.lastversion': '1.0',
+        'services.sync.autoconnectDelay': 60 * 60 * 10,
        'toolkit.startup.max_resumed_crashes': -1,
        # hrm - not sure what the release/beta channels will do?
        'xpinstall.signatures.required': False,
@ -437,7 +438,13 @@ class TPSTestRunner(object):
            jsondata = f.read()
            f.close()
            testfiles = json.loads(jsondata)
-            testlist = testfiles['tests']
+            testlist = []
+            for (filename, meta) in testfiles['tests'].items():
+                skip_reason = meta.get("disabled")
+                if skip_reason:
+                    print 'Skipping test %s - %s' % (filename, skip_reason)
+                else:
+                    testlist.append(filename)
        except ValueError:
            testlist = [os.path.basename(self.testfile)]
        testdir = os.path.dirname(self.testfile)