Bug 1319323 (part 1) - improve sync unit tests; remove many ensureLegacyIdentityManager calls. r?tcsc draft
authorMark Hammond <mhammond@skippinet.com.au>
Mon, 21 Nov 2016 12:22:24 +1100
changeset 443664 aeac9f9181d5814f7476ef1adc6e0deb9dd328e5
parent 443602 bad312aefb42982f492ad2cf36f4c6c3d698f4f7
child 443665 7b520c95bed5c01577fadf64bcaadb0de847a3b5
push id37055
push userbmo:markh@mozilla.com
push dateThu, 24 Nov 2016 22:52:21 +0000
reviewerstcsc
bugs1319323
milestone53.0a1
Bug 1319323 (part 1) - improve sync unit tests; remove many ensureLegacyIdentityManager calls. r?tcsc MozReview-Commit-ID: CV6cpo2Tp5O
services/sync/modules-testing/utils.js
services/sync/modules/browserid_identity.js
services/sync/tests/unit/test_bookmark_record.js
services/sync/tests/unit/test_clients_engine.js
services/sync/tests/unit/test_clients_escape.js
services/sync/tests/unit/test_corrupt_keys.js
services/sync/tests/unit/test_errorhandler_1.js
services/sync/tests/unit/test_errorhandler_2.js
services/sync/tests/unit/test_hmac_error.js
services/sync/tests/unit/test_records_crypto.js
services/sync/tests/unit/test_records_wbo.js
services/sync/tests/unit/test_resource_ua.js
services/sync/tests/unit/test_service_attributes.js
--- a/services/sync/modules-testing/utils.js
+++ b/services/sync/modules-testing/utils.js
@@ -139,16 +139,18 @@ this.setBasicCredentials =
   let auth = ns.Service.identity;
   auth.username = username;
   auth.basicPassword = password;
   auth.syncKey = syncKey;
 }
 
 // Return an identity configuration suitable for testing with our identity
 // providers.  |overrides| can specify overrides for any default values.
+// |server| is optional, but if specified, will be used to form the cluster
+// URL for the FxA identity.
 this.makeIdentityConfig = function(overrides) {
   // first setup the defaults.
   let result = {
     // Username used in both fxaccount and sync identity configs.
     username: "foo",
     // fxaccount specific credentials.
     fxaccount: {
       user: {
@@ -244,23 +246,38 @@ this.configureFxAccountIdentity = functi
   authService._fxaService = fxa;
   authService._tokenServerClient = mockTSC;
   // Set the "account" of the browserId manager to be the "email" of the
   // logged in user of the mockFXA service.
   authService._signedInUser = config.fxaccount.user;
   authService._account = config.fxaccount.user.email;
 }
 
-this.configureIdentity = async function(identityOverrides) {
-  let config = makeIdentityConfig(identityOverrides);
+this.configureIdentity = async function(identityOverrides, server) {
+  let config = makeIdentityConfig(identityOverrides, server);
   let ns = {};
   Cu.import("resource://services-sync/service.js", ns);
 
+  if (server) {
+    ns.Service.serverURL = server.baseURI;
+  }
+
   if (ns.Service.identity instanceof BrowserIDManager) {
     // do the FxAccounts thang...
+
+    // If a server was specified, ensure FxA has a correct cluster URL available.
+    if (server && !config.fxaccount.token.endpoint) {
+      let ep = server.baseURI;
+      if (!ep.endsWith("/")) {
+        ep += "/";
+      }
+      ep += "1.1/" + config.username + "/";
+      config.fxaccount.token.endpoint = ep;
+    }
+
     configureFxAccountIdentity(ns.Service.identity, config);
     await ns.Service.identity.initializeWithCurrentIdentity();
     // need to wait until this identity manager is readyToAuthenticate.
     await ns.Service.identity.whenReadyToAuthenticate.promise;
     return;
   }
   // old style identity provider.
   setBasicCredentials(config.username, config.sync.password, config.sync.syncKey);
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -372,17 +372,17 @@ this.BrowserIDManager.prototype = {
   },
 
   /**
    * Set the HTTP basic password to use.
    *
    * Changes will not persist unless persistSyncCredentials() is called.
    */
   set basicPassword(value) {
-    throw "basicPassword setter should be not used in BrowserIDManager";
+    throw new Error("basicPassword setter should be not used in BrowserIDManager");
   },
 
   /**
    * Obtain the Sync Key.
    *
    * This returns a 26 character "friendly" Base32 encoded string on success or
    * null if no Sync Key could be found.
    *
--- a/services/sync/tests/unit/test_bookmark_record.js
+++ b/services/sync/tests/unit/test_bookmark_record.js
@@ -10,20 +10,19 @@ Cu.import("resource://services-sync/util
 Cu.import("resource://testing-common/services/sync/utils.js");
 
 function prepareBookmarkItem(collection, id) {
   let b = new Bookmark(collection, id);
   b.cleartext.stuff = "my payload here";
   return b;
 }
 
-function run_test() {
-  ensureLegacyIdentityManager();
-  Service.identity.username = "john@example.com";
-  Service.identity.syncKey = "abcdeabcdeabcdeabcdeabcdea";
+add_task(async function test_bookmark_record() {
+  await configureIdentity();
+
   generateNewKeys(Service.collectionKeys);
   let keyBundle = Service.identity.syncKeyBundle;
 
   let log = Log.repository.getLogger("Test");
   Log.repository.rootLogger.addAppender(new Log.DumpAppender());
 
   log.info("Creating a record");
 
@@ -40,9 +39,9 @@ function run_test() {
   do_check_true(bookmarkItem.ciphertext != null);
 
   log.info("Decrypting the record");
 
   let payload = bookmarkItem.decrypt(keyBundle);
   do_check_eq(payload.stuff, "my payload here");
   do_check_eq(bookmarkItem.getTypeObject(bookmarkItem.type), Bookmark);
   do_check_neq(payload, bookmarkItem.payload); // wrap.data.payload is the encrypted one
-}
+});
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -32,17 +32,29 @@ function check_record_version(user, id) 
 
     _("Payload is " + JSON.stringify(cleartext));
     equal(Services.appinfo.version, cleartext.version);
     equal(2, cleartext.protocols.length);
     equal("1.1", cleartext.protocols[0]);
     equal("1.5", cleartext.protocols[1]);
 }
 
-add_test(function test_bad_hmac() {
+// compare 2 different command arrays, taking into account that a flowID
+// attribute must exist, be unique in the commands, but isn't specified in
+// "expected" as the value isn't known.
+function compareCommands(actual, expected, description) {
+  let tweakedActual = JSON.parse(JSON.stringify(actual));
+  tweakedActual.map(elt => delete elt.flowID);
+  deepEqual(tweakedActual, expected, description);
+  // each item must have a unique flowID.
+  let allIDs = new Set(actual.map(elt => elt.flowID).filter(fid => !!fid));
+  equal(allIDs.size, actual.length, "all items have unique IDs");
+}
+
+add_task(async function test_bad_hmac() {
   _("Ensure that Clients engine deletes corrupt records.");
   let contents = {
     meta: {global: {engines: {clients: {version: engine.version,
                                         syncID: engine.syncID}}}},
     clients: {},
     crypto: {}
   };
   let deletedCollections = [];
@@ -76,20 +88,18 @@ add_test(function test_bad_hmac() {
   function uploadNewKeys() {
     generateNewKeys(Service.collectionKeys);
     let serverKeys = Service.collectionKeys.asWBO("crypto", "keys");
     serverKeys.encrypt(Service.identity.syncKeyBundle);
     ok(serverKeys.upload(Service.resource(Service.cryptoKeysURL)).success);
   }
 
   try {
-    ensureLegacyIdentityManager();
-    let passphrase     = "abcdeabcdeabcdeabcdeabcdea";
-    Service.serverURL  = server.baseURI;
-    Service.login("foo", "ilovejane", passphrase);
+    await configureIdentity({username: "foo"}, server);
+    Service.login("foo");
 
     generateNewKeys(Service.collectionKeys);
 
     _("First sync, client record is uploaded");
     equal(engine.lastRecordUpload, 0);
     check_clients_count(0);
     engine._sync();
     check_clients_count(1);
@@ -169,17 +179,17 @@ add_test(function test_bad_hmac() {
     check_client_deleted(oldLocalID);
     check_clients_count(1);
     let newKey = Service.collectionKeys.keyForCollection();
     ok(!oldKey.equals(newKey));
 
   } finally {
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
-    server.stop(run_next_test);
+    await promiseStopServer(server);
   }
 });
 
 add_test(function test_properties() {
   _("Test lastRecordUpload property");
   try {
     equal(Svc.Prefs.get("clients.lastRecordUpload"), undefined);
     equal(engine.lastRecordUpload, 0);
--- a/services/sync/tests/unit/test_clients_escape.js
+++ b/services/sync/tests/unit/test_clients_escape.js
@@ -2,27 +2,24 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 Cu.import("resource://services-sync/keys.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 
-function run_test() {
+add_task(async function test_clients_escape() {
   _("Set up test fixtures.");
 
-  ensureLegacyIdentityManager();
-  Service.identity.username = "john@example.com";
-  Service.clusterURL = "http://fakebase/";
+  await configureIdentity();
   let baseUri = "http://fakebase/1.1/foo/storage/";
   let pubUri = baseUri + "keys/pubkey";
   let privUri = baseUri + "keys/privkey";
 
-  Service.identity.syncKey = "abcdeabcdeabcdeabcdeabcdea";
   let keyBundle = Service.identity.syncKeyBundle;
 
   let engine = Service.clientsEngine;
 
   try {
     _("Test that serializing client records results in uploadable ascii");
     engine.localID = "ascii";
     engine.localName = "wéävê";
@@ -56,9 +53,9 @@ function run_test() {
 
     _("Sanity check that creating the record also gives the same");
     record = engine._createRecord("ascii");
     do_check_eq(record.id, "ascii");
     do_check_eq(record.name, "wéävê");
   } finally {
     Svc.Prefs.resetBranch("");
   }
-}
+});
--- a/services/sync/tests/unit/test_corrupt_keys.js
+++ b/services/sync/tests/unit/test_corrupt_keys.js
@@ -9,18 +9,16 @@ Cu.import("resource://services-sync/engi
 Cu.import("resource://services-sync/engines/history.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/status.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 
 add_task(async function test_locally_changed_keys() {
-  let passphrase = "abcdeabcdeabcdeabcdeabcdea";
-
   let hmacErrorCount = 0;
   function counting(f) {
     return function() {
       hmacErrorCount++;
       return f.call(this);
     };
   }
 
@@ -48,19 +46,19 @@ add_task(async function test_locally_cha
                             image: "image"
                           }
                           }]}]};
     delete Svc.Session;
     Svc.Session = {
       getBrowserState: () => JSON.stringify(myTabs)
     };
 
-    setBasicCredentials("johndoe", "password", passphrase);
-    Service.serverURL = server.baseURI;
-    Service.clusterURL = server.baseURI;
+    await configureIdentity({ username: "johndoe" }, server);
+    // We aren't doing a .login yet, so fudge the cluster URL.
+    Service.clusterURL = Service.identity._token.endpoint;
 
     Service.engineManager.register(HistoryEngine);
     Service.engineManager.unregister("addons");
 
     function corrupt_local_keys() {
       Service.collectionKeys._default.keyPair = [Svc.Crypto.generateRandomKey(),
                                                  Svc.Crypto.generateRandomKey()];
     }
@@ -77,17 +75,17 @@ add_task(async function test_locally_cha
 
     // Upload keys.
     generateNewKeys(Service.collectionKeys);
     let serverKeys = Service.collectionKeys.asWBO("crypto", "keys");
     serverKeys.encrypt(Service.identity.syncKeyBundle);
     do_check_true(serverKeys.upload(Service.resource(Service.cryptoKeysURL)).success);
 
     // Check that login works.
-    do_check_true(Service.login("johndoe", "ilovejane", passphrase));
+    do_check_true(Service.login("johndoe"));
     do_check_true(Service.isLoggedIn);
 
     // Sync should upload records.
     await sync_and_validate_telem();
 
     // Tabs exist.
     _("Tabs modified: " + johndoe.modified("tabs"));
     do_check_true(johndoe.modified("tabs") > 0);
@@ -203,18 +201,16 @@ add_task(async function test_locally_cha
   }
 });
 
 function run_test() {
   let logger = Log.repository.rootLogger;
   Log.repository.rootLogger.addAppender(new Log.DumpAppender());
   validate_all_future_pings();
 
-  ensureLegacyIdentityManager();
-
   run_next_test();
 }
 
 /**
  * Asynchronously check a url is visited.
  * @param url the url
  * @return {Promise}
  * @resolves When the check has been added successfully.
--- a/services/sync/tests/unit/test_errorhandler_1.js
+++ b/services/sync/tests/unit/test_errorhandler_1.js
@@ -47,18 +47,16 @@ var errorHandler = Service.errorHandler;
 
 function run_test() {
   initTestLogging("Trace");
 
   Log.repository.getLogger("Sync.Service").level = Log.Level.Trace;
   Log.repository.getLogger("Sync.SyncScheduler").level = Log.Level.Trace;
   Log.repository.getLogger("Sync.ErrorHandler").level = Log.Level.Trace;
 
-  ensureLegacyIdentityManager();
-
   run_next_test();
 }
 
 
 function clean() {
   Service.startOver();
   Status.resetSync();
   Status.resetBackoff();
@@ -401,31 +399,29 @@ add_identity_test(this, function test_sh
   Status.login = LOGIN_FAILED_LOGIN_REJECTED;
   do_check_true(errorHandler.shouldReportError());
   // But any other status with a missing clusterURL is treated as a mid-sync
   // 401 (ie, should be treated as a node reassignment)
   Status.login = LOGIN_SUCCEEDED;
   do_check_false(errorHandler.shouldReportError());
 });
 
-// XXX - how to arrange for 'Service.identity.basicPassword = null;' in
-// an fxaccounts environment?
 add_task(async function test_login_syncAndReportErrors_non_network_error() {
   // Test non-network errors are reported
   // when calling syncAndReportErrors
   let server = EHTestsCommon.sync_httpd_setup();
   await EHTestsCommon.setUp(server);
-  Service.identity.basicPassword = null;
+  Service.identity.resetSyncKey();
 
   let promiseObserved = promiseOneObserver("weave:ui:login:error");
 
   setLastSync(NON_PROLONGED_ERROR_DURATION);
   errorHandler.syncAndReportErrors();
   await promiseObserved;
-  do_check_eq(Status.login, LOGIN_FAILED_NO_PASSWORD);
+  do_check_eq(Status.login, LOGIN_FAILED_NO_PASSPHRASE);
 
   clean();
   await promiseStopServer(server);
 });
 
 add_identity_test(this, async function test_sync_syncAndReportErrors_non_network_error() {
   // Test non-network errors are reported
   // when calling syncAndReportErrors
@@ -451,31 +447,29 @@ add_identity_test(this, async function t
   await promiseObserved;
 
   do_check_eq(Status.sync, CREDENTIALS_CHANGED);
   // If we clean this tick, telemetry won't get the right error
   await promiseStopServer(server);
   clean();
 });
 
-// XXX - how to arrange for 'Service.identity.basicPassword = null;' in
-// an fxaccounts environment?
 add_task(async function test_login_syncAndReportErrors_prolonged_non_network_error() {
   // Test prolonged, non-network errors are
   // reported when calling syncAndReportErrors.
   let server = EHTestsCommon.sync_httpd_setup();
   await EHTestsCommon.setUp(server);
-  Service.identity.basicPassword = null;
+  Service.identity.resetSyncKey();
 
   let promiseObserved = promiseOneObserver("weave:ui:login:error");
 
   setLastSync(PROLONGED_ERROR_DURATION);
   errorHandler.syncAndReportErrors();
   await promiseObserved;
-  do_check_eq(Status.login, LOGIN_FAILED_NO_PASSWORD);
+  do_check_eq(Status.login, LOGIN_FAILED_NO_PASSPHRASE);
 
   clean();
   await promiseStopServer(server);
 });
 
 add_identity_test(this, async function test_sync_syncAndReportErrors_prolonged_non_network_error() {
   // Test prolonged, non-network errors are
   // reported when calling syncAndReportErrors.
@@ -576,17 +570,17 @@ add_test(function test_sync_syncAndRepor
   setLastSync(PROLONGED_ERROR_DURATION);
   errorHandler.syncAndReportErrors();
 });
 
 add_task(async function test_login_prolonged_non_network_error() {
   // Test prolonged, non-network errors are reported
   let server = EHTestsCommon.sync_httpd_setup();
   await EHTestsCommon.setUp(server);
-  Service.identity.basicPassword = null;
+  Service.identity.resetSyncKey();
 
   let promiseObserved = promiseOneObserver("weave:ui:login:error");
 
   setLastSync(PROLONGED_ERROR_DURATION);
   Service.sync();
   await promiseObserved;
   do_check_eq(Status.sync, PROLONGED_SYNC_FAILURE);
   do_check_true(errorHandler.didReportProlongedError);
@@ -658,24 +652,24 @@ add_test(function test_sync_prolonged_ne
   setLastSync(PROLONGED_ERROR_DURATION);
   Service.sync();
 });
 
 add_task(async function test_login_non_network_error() {
   // Test non-network errors are reported
   let server = EHTestsCommon.sync_httpd_setup();
   await EHTestsCommon.setUp(server);
-  Service.identity.basicPassword = null;
+  Service.identity.resetSyncKey();
 
   let promiseObserved = promiseOneObserver("weave:ui:login:error");
 
   setLastSync(NON_PROLONGED_ERROR_DURATION);
   Service.sync();
   await promiseObserved;
-  do_check_eq(Status.login, LOGIN_FAILED_NO_PASSWORD);
+  do_check_eq(Status.login, LOGIN_FAILED_NO_PASSPHRASE);
   do_check_false(errorHandler.didReportProlongedError);
 
   clean();
   await promiseStopServer(server);
 });
 
 add_task(async function test_sync_non_network_error() {
   // Test non-network errors are reported
--- a/services/sync/tests/unit/test_errorhandler_2.js
+++ b/services/sync/tests/unit/test_errorhandler_2.js
@@ -47,18 +47,16 @@ var errorHandler = Service.errorHandler;
 
 function run_test() {
   initTestLogging("Trace");
 
   Log.repository.getLogger("Sync.Service").level = Log.Level.Trace;
   Log.repository.getLogger("Sync.SyncScheduler").level = Log.Level.Trace;
   Log.repository.getLogger("Sync.ErrorHandler").level = Log.Level.Trace;
 
-  ensureLegacyIdentityManager();
-
   run_next_test();
 }
 
 
 function clean() {
   Service.startOver();
   Status.resetSync();
   Status.resetBackoff();
--- a/services/sync/tests/unit/test_hmac_error.js
+++ b/services/sync/tests/unit/test_hmac_error.js
@@ -15,20 +15,16 @@ var hmacErrorCount = 0;
     hmacErrorCount++;
     return hHE.call(Service);
   };
 })();
 
 function shared_setup() {
   hmacErrorCount = 0;
 
-  // Do not instantiate SyncTestingInfrastructure; we need real crypto.
-  ensureLegacyIdentityManager();
-  setBasicCredentials("foo", "foo", "aabcdeabcdeabcdeabcdeabcde");
-
   // Make sure RotaryEngine is the only one we sync.
   Service.engineManager._engines = {};
   Service.engineManager.register(RotaryEngine);
   let engine = Service.engineManager.get("rotary");
   engine.enabled = true;
   engine.lastSync = 123; // Needs to be non-zero so that tracker is queried.
   engine._store.items = {flying: "LNER Class A3 4472",
                          scotsman: "Flying Scotsman"};
@@ -74,17 +70,19 @@ add_task(async function hmac_error_durin
     "/1.1/foo/info/collections": collectionsHelper.handler,
     "/1.1/foo/storage/meta/global": upd("meta", global.handler()),
     "/1.1/foo/storage/crypto/keys": upd("crypto", keys404Handler),
     "/1.1/foo/storage/clients": upd("clients", clientsColl.handler()),
     "/1.1/foo/storage/rotary": upd("rotary", rotaryColl.handler())
   };
 
   let server = sync_httpd_setup(handlers);
-  Service.serverURL = server.baseURI;
+  // Do not instantiate SyncTestingInfrastructure; we need real crypto.
+  await configureIdentity({ username: "foo" }, server);
+  Service.login();
 
   try {
     _("Syncing.");
     await sync_and_validate_telem();
 
     _("Partially resetting client, as if after a restart, and forcing redownload.");
     Service.collectionKeys.clear();
     engine.lastSync = 0;        // So that we redownload records.
@@ -97,17 +95,17 @@ add_task(async function hmac_error_durin
     do_check_eq(hmacErrorCount, 0)
   } finally {
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
     await promiseStopServer(server);
   }
 });
 
-add_test(function hmac_error_during_node_reassignment() {
+add_task(async function hmac_error_during_node_reassignment() {
   _("Attempt to replicate an HMAC error during node reassignment.");
   let [engine, rotaryColl, clientsColl, keysWBO, global] = shared_setup();
 
   let collectionsHelper = track_collections_helper();
   let upd = collectionsHelper.with_updated_collection;
 
   // We'll provide a 401 mid-way through the sync. This function
   // simulates shifting to a node which has no data.
@@ -151,17 +149,19 @@ add_test(function hmac_error_during_node
     "/1.1/foo/info/collections":    collectionsHelper.handler,
     "/1.1/foo/storage/meta/global": upd("meta", global.handler()),
     "/1.1/foo/storage/crypto/keys": upd("crypto", keysWBO.handler()),
     "/1.1/foo/storage/clients":     upd401("clients", clientsColl.handler()),
     "/1.1/foo/storage/rotary":      upd("rotary", rotaryColl.handler())
   };
 
   let server = sync_httpd_setup(handlers);
-  Service.serverURL = server.baseURI;
+  // Do not instantiate SyncTestingInfrastructure; we need real crypto.
+  await configureIdentity({ username: "foo" }, server);
+
   _("Syncing.");
   // First hit of clients will 401. This will happen after meta/global and
   // keys -- i.e., in the middle of the sync, but before RotaryEngine.
   should401 = true;
 
   // Use observers to perform actions when our sync finishes.
   // This allows us to observe the automatic next-tick sync that occurs after
   // an abort.
@@ -197,52 +197,54 @@ add_test(function hmac_error_during_node
 
     _("We correctly handle 401s by aborting the sync and starting again.");
     do_check_true(!hasData == !hasKeys);
 
     _("Be prepared for the second (automatic) sync...");
   }
 
   _("Make sure that syncing again causes recovery.");
-  onSyncFinished = function() {
-    _("== First sync done.");
-    _("---------------------------");
+  await new Promise(resolve => {
     onSyncFinished = function() {
-      _("== Second (automatic) sync done.");
-      hasData = rotaryColl.wbo("flying") ||
-                rotaryColl.wbo("scotsman");
-      hasKeys = keysWBO.modified;
-      do_check_true(!hasData == !hasKeys);
+      _("== First sync done.");
+      _("---------------------------");
+      onSyncFinished = function() {
+        _("== Second (automatic) sync done.");
+        hasData = rotaryColl.wbo("flying") ||
+                  rotaryColl.wbo("scotsman");
+        hasKeys = keysWBO.modified;
+        do_check_true(!hasData == !hasKeys);
 
-      // Kick off another sync. Can't just call it, because we're inside the
-      // lock...
-      Utils.nextTick(function() {
-        _("Now a fresh sync will get no HMAC errors.");
-        _("Partially resetting client, as if after a restart, and forcing redownload.");
-        Service.collectionKeys.clear();
-        engine.lastSync = 0;
-        hmacErrorCount = 0;
+        // Kick off another sync. Can't just call it, because we're inside the
+        // lock...
+        Utils.nextTick(function() {
+          _("Now a fresh sync will get no HMAC errors.");
+          _("Partially resetting client, as if after a restart, and forcing redownload.");
+          Service.collectionKeys.clear();
+          engine.lastSync = 0;
+          hmacErrorCount = 0;
 
-        onSyncFinished = function() {
-          // Two rotary items, one client record... no errors.
-          do_check_eq(hmacErrorCount, 0)
+          onSyncFinished = function() {
+            // Two rotary items, one client record... no errors.
+            do_check_eq(hmacErrorCount, 0)
 
-          Svc.Obs.remove("weave:service:sync:finish", obs);
-          Svc.Obs.remove("weave:service:sync:error", obs);
+            Svc.Obs.remove("weave:service:sync:finish", obs);
+            Svc.Obs.remove("weave:service:sync:error", obs);
 
-          Svc.Prefs.resetBranch("");
-          Service.recordManager.clearCache();
-          server.stop(run_next_test);
-        };
+            Svc.Prefs.resetBranch("");
+            Service.recordManager.clearCache();
+            server.stop(resolve);
+          };
 
-        Service.sync();
-      },
-      this);
+          Service.sync();
+        },
+        this);
+      };
     };
-  };
 
-  onwards();
+    onwards();
+  });
 });
 
 function run_test() {
   initTestLogging("Trace");
   run_next_test();
 }
--- a/services/sync/tests/unit/test_records_crypto.js
+++ b/services/sync/tests/unit/test_records_crypto.js
@@ -22,23 +22,20 @@ function crypted_resource_handler(metada
 function prepareCryptoWrap(collection, id) {
   let w = new CryptoWrapper();
   w.cleartext.stuff = "my payload here";
   w.collection = collection;
   w.id = id;
   return w;
 }
 
-function run_test() {
+add_task(async function test_records_crypto() {
   let server;
-  do_test_pending();
 
-  ensureLegacyIdentityManager();
-  Service.identity.username = "john@example.com";
-  Service.identity.syncKey = "a-abcde-abcde-abcde-abcde-abcde";
+  await configureIdentity({ username: "john@example.com" });
   let keyBundle = Service.identity.syncKeyBundle;
 
   try {
     let log = Log.repository.getLogger("Test");
     Log.repository.rootLogger.addAppender(new Log.DumpAppender());
 
     log.info("Setting up server and authenticator");
 
@@ -172,11 +169,11 @@ function run_test() {
       collections: {}
     };
     // Verify that not passing `modified` doesn't throw
     emptyKeys.setContents(payload, null);
 
     log.info("Done!");
   }
   finally {
-    server.stop(do_test_finished);
+    await promiseStopServer(server);
   }
-}
+});
--- a/services/sync/tests/unit/test_records_wbo.js
+++ b/services/sync/tests/unit/test_records_wbo.js
@@ -74,13 +74,12 @@ function test_fetch() {
 
   } finally {
     server.stop(do_test_finished);
   }
 }
 
 function run_test() {
   initTestLogging("Trace");
-  ensureLegacyIdentityManager();
 
   test_toJSON();
   test_fetch();
 }
--- a/services/sync/tests/unit/test_resource_ua.js
+++ b/services/sync/tests/unit/test_resource_ua.js
@@ -21,42 +21,40 @@ var expectedUA;
 var ua;
 function uaHandler(f) {
   return function(request, response) {
     ua = request.getHeader("User-Agent");
     return f(request, response);
   };
 }
 
-function run_test() {
+add_task(async function setup() {
+
   Log.repository.rootLogger.addAppender(new Log.DumpAppender());
   meta_global = new ServerWBO('global');
   server = httpd_setup({
     "/1.1/johndoe/info/collections": uaHandler(collectionsHelper.handler),
     "/1.1/johndoe/storage/meta/global": uaHandler(meta_global.handler()),
   });
 
-  ensureLegacyIdentityManager();
-  setBasicCredentials("johndoe", "ilovejane");
-  Service.serverURL = server.baseURI + "/";
-  Service.clusterURL = server.baseURI + "/";
+  await configureIdentity({ username: "johndoe" }, server);
   _("Server URL: " + server.baseURI);
 
   // Note this string is missing the trailing ".destkop" as the test
   // adjusts the "client.type" pref where that portion comes from.
   expectedUA = Services.appinfo.name + "/" + Services.appinfo.version +
                " (" + httpProtocolHandler.oscpu + ")" +
                " FxSync/" + WEAVE_VERSION + "." +
                Services.appinfo.appBuildID;
 
-  run_next_test();
-}
+})
 
 add_test(function test_fetchInfo() {
   _("Testing _fetchInfo.");
+  Service.login();
   Service._fetchInfo();
   _("User-Agent: " + ua);
   do_check_eq(ua, expectedUA + ".desktop");
   ua = "";
   run_next_test();
 });
 
 add_test(function test_desktop_post() {
--- a/services/sync/tests/unit/test_service_attributes.js
+++ b/services/sync/tests/unit/test_service_attributes.js
@@ -2,38 +2,37 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://testing-common/services/sync/fakeservices.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 
-function test_urls() {
+add_task(async function test_urls() {
   _("URL related Service properties correspond to preference settings.");
   try {
-    ensureLegacyIdentityManager();
     do_check_true(!!Service.serverURL); // actual value may change
     do_check_eq(Service.clusterURL, "");
-    do_check_eq(Service.userBaseURL, undefined);
+    do_check_false(Service.userBaseURL);
     do_check_eq(Service.infoURL, undefined);
     do_check_eq(Service.storageURL, undefined);
     do_check_eq(Service.metaURL, undefined);
 
     _("The 'clusterURL' attribute updates preferences and cached URLs.");
     Service.identity.username = "johndoe";
 
     // Since we don't have a cluster URL yet, these will still not be defined.
     do_check_eq(Service.infoURL, undefined);
-    do_check_eq(Service.userBaseURL, undefined);
+    do_check_false(Service.userBaseURL);
     do_check_eq(Service.storageURL, undefined);
     do_check_eq(Service.metaURL, undefined);
 
     Service.serverURL = "http://weave.server/";
-    Service.clusterURL = "http://weave.cluster/";
+    Service.clusterURL = "http://weave.cluster/1.1/johndoe/";
 
     do_check_eq(Service.userBaseURL, "http://weave.cluster/1.1/johndoe/");
     do_check_eq(Service.infoURL,
                 "http://weave.cluster/1.1/johndoe/info/collections");
     do_check_eq(Service.storageURL,
                 "http://weave.cluster/1.1/johndoe/storage/");
     do_check_eq(Service.metaURL,
                 "http://weave.cluster/1.1/johndoe/storage/meta/global");
@@ -55,31 +54,28 @@ function test_urls() {
                 "http://weave.server/weave-password-reset");
 
     _("Empty/false value for 'username' resets preference.");
     Service.identity.username = "";
     do_check_eq(Svc.Prefs.get("username"), undefined);
     do_check_eq(Service.identity.username, null);
 
     _("The 'serverURL' attributes updates/resets preferences.");
-    // Identical value doesn't do anything
-    Service.serverURL = Service.serverURL;
-    do_check_eq(Service.clusterURL, "http://weave.cluster/");
 
     Service.serverURL = "http://different.auth.node/";
     do_check_eq(Svc.Prefs.get("serverURL"), "http://different.auth.node/");
     do_check_eq(Service.clusterURL, "");
 
   } finally {
     Svc.Prefs.resetBranch("");
   }
-}
+});
 
 
-function test_syncID() {
+add_test(function test_syncID() {
   _("Service.syncID is auto-generated, corresponds to preference.");
   new FakeGUIDService();
 
   try {
     // Ensure pristine environment
     do_check_eq(Svc.Prefs.get("client.syncID"), undefined);
 
     // Performing the first get on the attribute will generate a new GUID.
@@ -87,32 +83,28 @@ function test_syncID() {
     do_check_eq(Svc.Prefs.get("client.syncID"), "fake-guid-00");
 
     Svc.Prefs.set("client.syncID", Utils.makeGUID());
     do_check_eq(Svc.Prefs.get("client.syncID"), "fake-guid-01");
     do_check_eq(Service.syncID, "fake-guid-01");
   } finally {
     Svc.Prefs.resetBranch("");
     new FakeGUIDService();
+    run_next_test();
   }
-}
+});
 
-function test_locked() {
+add_test(function test_locked() {
   _("The 'locked' attribute can be toggled with lock() and unlock()");
 
   // Defaults to false
   do_check_eq(Service.locked, false);
 
   do_check_eq(Service.lock(), true);
   do_check_eq(Service.locked, true);
 
   // Locking again will return false
   do_check_eq(Service.lock(), false);
 
   Service.unlock();
   do_check_eq(Service.locked, false);
-}
-
-function run_test() {
-  test_urls();
-  test_syncID();
-  test_locked();
-}
+  run_next_test();
+});