Bug 1332024 - Finalize tracker storage and consolidate cleanup logic in engine tests. r=markh draft
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 24 Jan 2017 09:59:09 -0800
changeset 466219 a8a2ee2a02b7118a55eb9e9c9da66f904f67cbf0
parent 466218 ad08e72ac3812f3ad6169c35161bd46eddd32a65
child 543371 5152f8b8844adb343692a563bfd5000e3cfe4dcd
push id42841
push userbmo:kit@mozilla.com
push dateWed, 25 Jan 2017 16:45:00 +0000
reviewersmarkh
bugs1332024
milestone54.0a1
Bug 1332024 - Finalize tracker storage and consolidate cleanup logic in engine tests. r=markh MozReview-Commit-ID: 8t9bXFrLA1Z
services/sync/modules/engines.js
services/sync/tests/unit/test_clients_engine.js
services/sync/tests/unit/test_engine.js
services/sync/tests/unit/test_enginemanager.js
services/sync/tests/unit/test_telemetry.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -609,17 +609,21 @@ EngineManager.prototype = {
     }
   },
 
   unregister(val) {
     let name = val;
     if (val instanceof Engine) {
       name = val.name;
     }
-    delete this._engines[name];
+    if (name in this._engines) {
+      let engine = this._engines[name];
+      delete this._engines[name];
+      engine.finalize();
+    }
   },
 
   clear() {
     for (let name in this._engines) {
       delete this._engines[name];
     }
   },
 };
@@ -722,17 +726,22 @@ Engine.prototype = {
 
   /**
    * If one exists, initialize and return a validator for this engine (which
    * must have a `validate(engine)` method that returns a promise to an object
    * with a getSummary method). Otherwise return null.
    */
   getValidator() {
     return null;
-  }
+  },
+
+  finalize() {
+    // Ensure the tracker finishes persisting changed IDs to disk.
+    Async.promiseSpinningly(this._tracker._storage.finalize());
+  },
 };
 
 this.SyncEngine = function SyncEngine(name, service) {
   Engine.call(this, name || "SyncEngine", service);
 
   this.loadToFetch();
   this.loadPreviousFailed();
 }
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -44,16 +44,24 @@ function compareCommands(actual, expecte
   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");
 }
 
+function cleanup() {
+  Svc.Prefs.resetBranch("");
+  engine._tracker.clearChangedIDs();
+  engine._resetClient();
+  // We don't finalize storage at cleanup, since we use the same clients engine
+  // instance across all tests.
+}
+
 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: {}
   };
@@ -177,35 +185,32 @@ add_task(async function test_bad_hmac() 
     engine._sync();
     equal(deletedItems.length, 1);
     check_client_deleted(oldLocalID);
     check_clients_count(1);
     let newKey = Service.collectionKeys.keyForCollection();
     ok(!oldKey.equals(newKey));
 
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
+    cleanup();
     await promiseStopServer(server);
   }
 });
 
-add_test(function test_properties() {
+add_task(async function test_properties() {
   _("Test lastRecordUpload property");
   try {
     equal(Svc.Prefs.get("clients.lastRecordUpload"), undefined);
     equal(engine.lastRecordUpload, 0);
 
     let now = Date.now();
     engine.lastRecordUpload = now / 1000;
     equal(engine.lastRecordUpload, Math.floor(now / 1000));
   } finally {
-    Svc.Prefs.resetBranch("");
-    engine._tracker.clearChangedIDs();
-    run_next_test();
+    cleanup();
   }
 });
 
 add_task(async function test_full_sync() {
   _("Ensure that Clients engine fetches all records for each sync.");
 
   let now = Date.now() / 1000;
   let contents = {
@@ -261,18 +266,17 @@ add_task(async function test_full_sync()
     engine.lastModified = now;
     engine._sync();
 
     _("Record should be updated");
     deepEqual(Object.keys(store.getAllIDs()).sort(),
               [activeID, engine.localID].sort(),
               "Deleted client should be removed on next sync");
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
+    cleanup();
 
     try {
       server.deleteCollections("foo");
     } finally {
       await promiseStopServer(server);
     }
   }
 });
@@ -320,23 +324,22 @@ add_task(async function test_sync() {
     _("Time travel one day back, no record uploaded.");
     engine.lastRecordUpload -= LESS_THAN_CLIENTS_TTL_REFRESH;
     let yesterday = engine.lastRecordUpload;
     engine._sync();
     equal(clientWBO().payload, undefined);
     equal(engine.lastRecordUpload, yesterday);
 
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
+    cleanup();
     await promiseStopServer(server);
   }
 });
 
-add_test(function test_client_name_change() {
+add_task(async function test_client_name_change() {
   _("Ensure client name change incurs a client record update.");
 
   let tracker = engine._tracker;
 
   engine.localID; // Needed to increase the tracker changedIDs count.
   let initialName = engine.localName;
 
   Svc.Obs.notify("weave:engine:start-tracking");
@@ -355,21 +358,20 @@ add_test(function test_client_name_chang
   notEqual(initialName, engine.localName);
   equal(Object.keys(tracker.changedIDs).length, 1);
   ok(engine.localID in tracker.changedIDs);
   ok(tracker.score > initialScore);
   ok(tracker.score >= SCORE_INCREMENT_XLARGE);
 
   Svc.Obs.notify("weave:engine:stop-tracking");
 
-  engine._tracker.clearChangedIDs();
-  run_next_test();
+  cleanup();
 });
 
-add_test(function test_send_command() {
+add_task(async function test_send_command() {
   _("Verifies _sendCommandToClient puts commands in the outbound queue.");
 
   let store = engine._store;
   let tracker = engine._tracker;
   let remoteId = Utils.makeGUID();
   let rec = new ClientsRec("clients", remoteId);
 
   store.create(rec);
@@ -388,21 +390,20 @@ add_test(function test_send_command() {
   let command = clientCommands[0];
   equal(command.command, action);
   equal(command.args.length, 2);
   deepEqual(command.args, args);
   ok(command.flowID);
 
   notEqual(tracker.changedIDs[remoteId], undefined);
 
-  engine._tracker.clearChangedIDs();
-  run_next_test();
+  cleanup();
 });
 
-add_test(function test_command_validation() {
+add_task(async function test_command_validation() {
   _("Verifies that command validation works properly.");
 
   let store = engine._store;
 
   let testCommands = [
     ["resetAll",    [],       true ],
     ["resetAll",    ["foo"],  false],
     ["resetEngine", ["tabs"], true ],
@@ -445,21 +446,20 @@ add_test(function test_command_validatio
       equal(clientCommands, undefined);
 
       if (store._tracker) {
         equal(engine._tracker[remoteId], undefined);
       }
     }
 
   }
-  engine._tracker.clearChangedIDs();
-  run_next_test();
+  cleanup();
 });
 
-add_test(function test_command_duplication() {
+add_task(async function test_command_duplication() {
   _("Ensures duplicate commands are detected and not added");
 
   let store = engine._store;
   let remoteId = Utils.makeGUID();
   let rec = new ClientsRec("clients", remoteId);
   store.create(rec);
   store.createRecord(remoteId, "clients");
 
@@ -480,60 +480,59 @@ add_test(function test_command_duplicati
   engine.sendCommand(action, [{ x: "bar" }], remoteId);
 
   _("Make sure we spot a real dupe argument.");
   engine.sendCommand(action, [{ x: "bar" }], remoteId);
 
   clientCommands = engine._readCommands()[remoteId];
   equal(clientCommands.length, 2);
 
-  engine._tracker.clearChangedIDs();
-  run_next_test();
+  cleanup();
 });
 
-add_test(function test_command_invalid_client() {
+add_task(async function test_command_invalid_client() {
   _("Ensures invalid client IDs are caught");
 
   let id = Utils.makeGUID();
   let error;
 
   try {
     engine.sendCommand("wipeAll", [], id);
   } catch (ex) {
     error = ex;
   }
 
   equal(error.message.indexOf("Unknown remote client ID: "), 0);
 
-  engine._tracker.clearChangedIDs();
-  run_next_test();
+  cleanup();
 });
 
-add_test(function test_process_incoming_commands() {
+add_task(async function test_process_incoming_commands() {
   _("Ensures local commands are executed");
 
   engine.localCommands = [{ command: "logout", args: [] }];
 
   let ev = "weave:service:logout:finish";
 
-  var handler = function() {
-    Svc.Obs.remove(ev, handler);
+  let logoutPromise = new Promise(resolve => {
+    var handler = function() {
+      Svc.Obs.remove(ev, handler);
 
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
-    engine._resetClient();
+      resolve();
+    };
 
-    engine._tracker.clearChangedIDs();
-    run_next_test();
-  };
-
-  Svc.Obs.add(ev, handler);
+    Svc.Obs.add(ev, handler);
+  });
 
   // logout command causes processIncomingCommands to return explicit false.
   ok(!engine.processIncomingCommands());
+
+  await logoutPromise;
+
+  cleanup();
 });
 
 add_task(async function test_filter_duplicate_names() {
   _("Ensure that we exclude clients with identical names that haven't synced in a week.");
 
   let now = Date.now() / 1000;
   let contents = {
     meta: {global: {engines: {clients: {version: engine.version,
@@ -671,18 +670,17 @@ add_task(async function test_filter_dupl
       names: [engine.localName, "My Phone", engine.localName, "My old desktop"],
       numClients: 4,
     });
 
     ok(engine.remoteClientExists(dupeID), "recently synced dupe ID should now exist");
     equal(engine.remoteClients.length, 3, "recently synced dupe should now be in remoteClients");
 
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
+    cleanup();
 
     try {
       server.deleteCollections("foo");
     } finally {
       await promiseStopServer(server);
     }
   }
 });
@@ -748,29 +746,28 @@ add_task(async function test_command_syn
     notEqual(engine.localCommands, undefined);
     equal(engine.localCommands.length, 1);
 
     let command = engine.localCommands[0];
     equal(command.command, "wipeAll");
     equal(command.args.length, 0);
 
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
+    cleanup();
 
     try {
       let collection = server.getCollection("foo", "clients");
       collection.remove(remoteId);
     } finally {
       await promiseStopServer(server);
     }
   }
 });
 
-add_test(function test_send_uri_to_client_for_display() {
+add_task(async function test_send_uri_to_client_for_display() {
   _("Ensure sendURIToClientForDisplay() sends command properly.");
 
   let tracker = engine._tracker;
   let store = engine._store;
 
   let remoteId = Utils.makeGUID();
   let rec = new ClientsRec("clients", remoteId);
   rec.name = "remote";
@@ -807,25 +804,20 @@ add_test(function test_send_uri_to_clien
   try {
     engine.sendURIToClientForDisplay(uri, unknownId);
   } catch (ex) {
     error = ex;
   }
 
   equal(error.message.indexOf("Unknown remote client ID: "), 0);
 
-  Svc.Prefs.resetBranch("");
-  Service.recordManager.clearCache();
-  engine._resetClient();
-
-  engine._tracker.clearChangedIDs();
-  run_next_test();
+  cleanup();
 });
 
-add_test(function test_receive_display_uri() {
+add_task(async function test_receive_display_uri() {
   _("Ensure processing of received 'displayURI' commands works.");
 
   // We don't set up WBOs and perform syncing because other tests verify
   // the command API works as advertised. This saves us a little work.
 
   let uri = "http://www.mozilla.org/";
   let remoteId = Utils.makeGUID();
   let title = "Page Title!";
@@ -836,38 +828,39 @@ add_test(function test_receive_display_u
   };
 
   engine.localCommands = [command];
 
   // Received 'displayURI' command should result in the topic defined below
   // being called.
   let ev = "weave:engine:clients:display-uris";
 
-  let handler = function(subject, data) {
-    Svc.Obs.remove(ev, handler);
+  let promiseDisplayURI = new Promise(resolve => {
+    let handler = function(subject, data) {
+      Svc.Obs.remove(ev, handler);
 
-    equal(subject[0].uri, uri);
-    equal(subject[0].clientId, remoteId);
-    equal(subject[0].title, title);
-    equal(data, null);
+      resolve({ subject, data });
+    };
 
-    engine._tracker.clearChangedIDs();
-    run_next_test();
-  };
-
-  Svc.Obs.add(ev, handler);
+    Svc.Obs.add(ev, handler);
+  });
 
   ok(engine.processIncomingCommands());
 
-  Svc.Prefs.resetBranch("");
-  Service.recordManager.clearCache();
-  engine._resetClient();
+  let { subject, data } = await promiseDisplayURI;
+
+  equal(subject[0].uri, uri);
+  equal(subject[0].clientId, remoteId);
+  equal(subject[0].title, title);
+  equal(data, null);
+
+  cleanup();
 });
 
-add_test(function test_optional_client_fields() {
+add_task(async function test_optional_client_fields() {
   _("Ensure that we produce records with the fields added in Bug 1097222.");
 
   const SUPPORTED_PROTOCOL_VERSIONS = ["1.1", "1.5"];
   let local = engine._store.createRecord(engine.localID, "clients");
   equal(local.name, engine.localName);
   equal(local.type, engine.localType);
   equal(local.version, Services.appinfo.version);
   deepEqual(local.protocols, SUPPORTED_PROTOCOL_VERSIONS);
@@ -880,19 +873,17 @@ add_test(function test_optional_client_f
   // ... and also that they're non-empty.
   ok(!!local.os);
   ok(!!local.appPackage);
   ok(!!local.application);
 
   // We don't currently populate device or formfactor.
   // See Bug 1100722, Bug 1100723.
 
-  engine._resetClient();
-  engine._tracker.clearChangedIDs();
-  run_next_test();
+  cleanup();
 });
 
 add_task(async function test_merge_commands() {
   _("Verifies local commands for remote clients are merged with the server's");
 
   let now = Date.now() / 1000;
   let contents = {
     meta: {global: {engines: {clients: {version: engine.version,
@@ -951,19 +942,17 @@ add_task(async function test_merge_comma
       command: "logout",
       args: [],
     }], "Should send the logout command to the desktop client");
 
     let mobilePayload = JSON.parse(JSON.parse(collection.payload(mobileID)).ciphertext);
     compareCommands(mobilePayload.commands, [{ command: "logout", args: [] }],
                     "Should not send a duplicate logout to the mobile client");
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
-    engine._resetClient();
+    cleanup();
 
     try {
       server.deleteCollections("foo");
     } finally {
       await promiseStopServer(server);
     }
   }
 });
@@ -1018,19 +1007,17 @@ add_task(async function test_duplicate_r
 
     let collection = server.getCollection("foo", "clients");
     let desktopPayload = JSON.parse(JSON.parse(collection.payload(desktopID)).ciphertext);
     compareCommands(desktopPayload.commands, [{
       command: "displayURI",
       args: ["https://foobar.com", engine.localID, "Foo bar!"],
     }], "Should only send the second command to the desktop client");
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
-    engine._resetClient();
+    cleanup();
 
     try {
       server.deleteCollections("foo");
     } finally {
       await promiseStopServer(server);
     }
   }
 });
@@ -1108,19 +1095,17 @@ add_task(async function test_upload_afte
     engine._sync();
 
     deviceBPayload = JSON.parse(JSON.parse(collection.payload(deviceBID)).ciphertext);
     compareCommands(deviceBPayload.commands, [{
       command: "displayURI",
       args: ["https://example.com", engine.localID, "Yak Herders Anonymous"],
     }], "Should only had written our outgoing command");
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
-    engine._resetClient();
+    cleanup();
 
     try {
       server.deleteCollections("foo");
     } finally {
       await promiseStopServer(server);
     }
   }
 });
@@ -1230,18 +1215,17 @@ add_task(async function test_keep_cleare
     engine._handleDisplayURIs = (uris) => { commandsProcessed = uris.length };
     engine._sync();
     engine.processIncomingCommands();
     equal(commandsProcessed, 1, "We processed one command (the other were cleared)");
 
     localRemoteRecord = JSON.parse(JSON.parse(collection.payload(deviceBID)).ciphertext);
     deepEqual(localRemoteRecord.commands, [], "Should be empty");
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
+    cleanup();
 
     // Reset service (remove mocks)
     engine = Service.clientsEngine = new ClientEngine(Service);
     engine._resetClient();
 
     try {
       server.deleteCollections("foo");
     } finally {
@@ -1299,19 +1283,17 @@ add_task(async function test_deleted_com
 
     deepEqual(collection.keys().sort(), [activeID, engine.localID].sort(),
       "Should not reupload deleted clients");
 
     let activePayload = JSON.parse(JSON.parse(collection.payload(activeID)).ciphertext);
     compareCommands(activePayload.commands, [{ command: "logout", args: [] }],
       "Should send the command to the active client");
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
-    engine._resetClient();
+    cleanup();
 
     try {
       server.deleteCollections("foo");
     } finally {
       await promiseStopServer(server);
     }
   }
 });
@@ -1362,19 +1344,17 @@ add_task(async function test_send_uri_ac
     }], "Should mark the commands as cleared after processing");
 
     _("Check that the command was removed on the server");
     engine._sync();
     ourPayload = JSON.parse(JSON.parse(collection.payload(engine.localID)).ciphertext);
     ok(ourPayload, "Should upload the synced client record");
     deepEqual(ourPayload.commands, [], "Should not reupload cleared commands");
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
-    engine._resetClient();
+    cleanup();
 
     try {
       server.deleteCollections("foo");
     } finally {
       await promiseStopServer(server);
     }
   }
 });
@@ -1430,18 +1410,17 @@ add_task(async function test_command_syn
     engine._notifyCollectionChanged = (ids) => (notifiedIds = ids);
     _("Syncing.");
     engine._sync();
     deepEqual(notifiedIds, ["fxa-fake-guid-00", "fxa-fake-guid-01"]);
     ok(!notifiedIds.includes(engine.getClientFxaDeviceId(engine.localID)),
       "We never notify the local device");
 
   } finally {
-    Svc.Prefs.resetBranch("");
-    Service.recordManager.clearCache();
+    cleanup();
     engine._tracker.clearChangedIDs();
 
     try {
       server.deleteCollections("foo");
     } finally {
       await promiseStopServer(server);
     }
   }
--- a/services/sync/tests/unit/test_engine.js
+++ b/services/sync/tests/unit/test_engine.js
@@ -59,16 +59,25 @@ var engineObserver = {
 };
 Observers.add("weave:engine:reset-client:start", engineObserver);
 Observers.add("weave:engine:reset-client:finish", engineObserver);
 Observers.add("weave:engine:wipe-client:start", engineObserver);
 Observers.add("weave:engine:wipe-client:finish", engineObserver);
 Observers.add("weave:engine:sync:start", engineObserver);
 Observers.add("weave:engine:sync:finish", engineObserver);
 
+async function cleanup(engine) {
+  Svc.Prefs.resetBranch("");
+  engine.wasReset = false;
+  engine.wasSynced = false;
+  engineObserver.reset();
+  engine._tracker.clearChangedIDs();
+  await engine._tracker._storage.finalize();
+}
+
 add_task(async function test_members() {
   _("Engine object members");
   let engine = new SteamEngine("Steam", Service);
   do_check_eq(engine.Name, "Steam");
   do_check_eq(engine.prefName, "steam");
   do_check_true(engine._store instanceof SteamStore);
   do_check_true(engine._tracker instanceof SteamTracker);
 });
@@ -95,19 +104,17 @@ add_task(async function test_resetClient
   let engine = new SteamEngine("Steam", Service);
   do_check_false(engine.wasReset);
 
   engine.resetClient();
   do_check_true(engine.wasReset);
   do_check_eq(engineObserver.topics[0], "weave:engine:reset-client:start");
   do_check_eq(engineObserver.topics[1], "weave:engine:reset-client:finish");
 
-  engine.wasReset = false;
-  engineObserver.reset();
-  engine._tracker.clearChangedIDs();
+  await cleanup(engine);
 });
 
 add_task(async function test_invalidChangedIDs() {
   _("Test that invalid changed IDs on disk don't end up live.");
   let engine = new SteamEngine("Steam", Service);
   let tracker = engine._tracker;
 
   await tracker._beforeSave();
@@ -116,17 +123,17 @@ add_task(async function test_invalidChan
 
   ok(!tracker._storage.dataReady);
   tracker.changedIDs.placeholder = true;
   deepEqual(tracker.changedIDs, { placeholder: true },
     "Accessing changed IDs should load changes from disk as a side effect");
   ok(tracker._storage.dataReady);
 
   do_check_true(tracker.changedIDs.placeholder);
-  engine._tracker.clearChangedIDs();
+  await cleanup(engine);
 });
 
 add_task(async function test_wipeClient() {
   _("Engine.wipeClient calls resetClient, wipes store, clears changed IDs");
   let engine = new SteamEngine("Steam", Service);
   do_check_false(engine.wasReset);
   do_check_false(engine._store.wasWiped);
   do_check_true(engine._tracker.addChangedID("a-changed-id"));
@@ -136,34 +143,31 @@ add_task(async function test_wipeClient(
   do_check_true(engine.wasReset);
   do_check_true(engine._store.wasWiped);
   do_check_eq(JSON.stringify(engine._tracker.changedIDs), "{}");
   do_check_eq(engineObserver.topics[0], "weave:engine:wipe-client:start");
   do_check_eq(engineObserver.topics[1], "weave:engine:reset-client:start");
   do_check_eq(engineObserver.topics[2], "weave:engine:reset-client:finish");
   do_check_eq(engineObserver.topics[3], "weave:engine:wipe-client:finish");
 
-  engine.wasReset = false;
-  engine._store.wasWiped = false;
-  engineObserver.reset();
-  engine._tracker.clearChangedIDs();
+  await cleanup(engine);
 });
 
 add_task(async function test_enabled() {
   _("Engine.enabled corresponds to preference");
   let engine = new SteamEngine("Steam", Service);
   try {
     do_check_false(engine.enabled);
     Svc.Prefs.set("engine.steam", true);
     do_check_true(engine.enabled);
 
     engine.enabled = false;
     do_check_false(Svc.Prefs.get("engine.steam"));
   } finally {
-    Svc.Prefs.resetBranch("");
+    await cleanup(engine);
   }
 });
 
 add_task(async function test_sync() {
   let engine = new SteamEngine("Steam", Service);
   try {
     _("Engine.sync doesn't call _sync if it's not enabled");
     do_check_false(engine.enabled);
@@ -175,20 +179,17 @@ add_task(async function test_sync() {
     _("Engine.sync calls _sync if it's enabled");
     engine.enabled = true;
 
     engine.sync();
     do_check_true(engine.wasSynced);
     do_check_eq(engineObserver.topics[0], "weave:engine:sync:start");
     do_check_eq(engineObserver.topics[1], "weave:engine:sync:finish");
   } finally {
-    Svc.Prefs.resetBranch("");
-    engine.wasSynced = false;
-    engineObserver.reset();
-    engine._tracker.clearChangedIDs();
+    await cleanup(engine);
   }
 });
 
 add_task(async function test_disabled_no_track() {
   _("When an engine is disabled, its tracker is not tracking.");
   let engine = new SteamEngine("Steam", Service);
   let tracker = engine._tracker;
   do_check_eq(engine, tracker.engine);
@@ -208,10 +209,10 @@ add_task(async function test_disabled_no
   do_check_empty(tracker.changedIDs);
 
   tracker.addChangedID("abcdefghijkl");
   do_check_true(0 < tracker.changedIDs["abcdefghijkl"]);
   Svc.Prefs.set("engine." + engine.prefName, false);
   do_check_false(tracker._isTracking);
   do_check_empty(tracker.changedIDs);
 
-  engine._tracker.clearChangedIDs();
+  await cleanup(engine);
 });
--- a/services/sync/tests/unit/test_enginemanager.js
+++ b/services/sync/tests/unit/test_enginemanager.js
@@ -5,22 +5,25 @@ Cu.import("resource://services-sync/engi
 Cu.import("resource://services-sync/service.js");
 
 function run_test() {
   run_next_test();
 }
 
 function PetrolEngine() {}
 PetrolEngine.prototype.name = "petrol";
+PetrolEngine.prototype.finalize = function() {};
 
 function DieselEngine() {}
 DieselEngine.prototype.name = "diesel";
+DieselEngine.prototype.finalize = function() {};
 
 function DummyEngine() {}
 DummyEngine.prototype.name = "dummy";
+DummyEngine.prototype.finalize = function() {};
 
 function ActualEngine() {}
 ActualEngine.prototype = {__proto__: Engine.prototype,
                           name: "actual"};
 
 add_test(function test_basics() {
   _("We start out with a clean slate");
 
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -347,18 +347,18 @@ add_task(async function test_generic_eng
       JSON.stringify(engine._tracker.changedIDs)}`);
     let ping = await sync_and_validate_telem(true);
     equal(ping.status.service, SYNC_FAILED_PARTIAL);
     deepEqual(ping.engines.find(e => e.name === "steam").failureReason, {
       name: "unexpectederror",
       error: String(e)
     });
   } finally {
+    await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
-    await cleanAndGo(engine, server);
   }
 });
 
 add_task(async function test_engine_fail_ioerror() {
   Service.engineManager.register(SteamEngine);
   let engine = Service.engineManager.get("steam");
   engine.enabled = true;
   let server = serverForUsers({"foo": "password"}, {
@@ -384,18 +384,18 @@ add_task(async function test_engine_fail
     let ping = await sync_and_validate_telem(true);
     equal(ping.status.service, SYNC_FAILED_PARTIAL);
     let failureReason = ping.engines.find(e => e.name === "steam").failureReason;
     equal(failureReason.name, "unexpectederror");
     // ensure the profile dir in the exception message has been stripped.
     ok(!failureReason.error.includes(OS.Constants.Path.profileDir), failureReason.error);
     ok(failureReason.error.includes("[profileDir]"), failureReason.error);
   } finally {
+    await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
-    await cleanAndGo(engine, server);
   }
 });
 
 add_task(async function test_initial_sync_engines() {
   Service.engineManager.register(SteamEngine);
   let engine = Service.engineManager.get("steam");
   engine.enabled = true;
   let engines = {};
@@ -424,16 +424,17 @@ add_task(async function test_initial_syn
       greaterOrEqual(e.took, 1);
       ok(!!e.outgoing)
       equal(e.outgoing.length, 1);
       notEqual(e.outgoing[0].sent, undefined);
       equal(e.outgoing[0].failed, undefined);
     }
   } finally {
     await cleanAndGo(engine, server);
+    Service.engineManager.unregister(engine);
   }
 });
 
 add_task(async function test_nserror() {
   Service.engineManager.register(SteamEngine);
   let engine = Service.engineManager.get("steam");
   engine.enabled = true;
   let server = serverForUsers({"foo": "password"}, {
@@ -452,18 +453,18 @@ add_task(async function test_nserror() {
       sync: LOGIN_FAILED_NETWORK_ERROR
     });
     let enginePing = ping.engines.find(e => e.name === "steam");
     deepEqual(enginePing.failureReason, {
       name: "nserror",
       code: Cr.NS_ERROR_UNKNOWN_HOST
     });
   } finally {
+    await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
-    await cleanAndGo(engine, server);
   }
 });
 
 add_identity_test(this, async function test_discarding() {
   let helper = track_collections_helper();
   let upd = helper.with_updated_collection;
   let telem = get_sync_test_telemetry();
   telem.maxPayloadCount = 2;
@@ -517,18 +518,18 @@ add_task(async function test_no_foreign_
   });
   engine._errToThrow = new Error("Oh no!");
   await SyncTestingInfrastructure(server);
   try {
     let ping = await sync_and_validate_telem(true);
     equal(ping.status.service, SYNC_FAILED_PARTIAL);
     ok(ping.engines.every(e => e.name !== "bogus"));
   } finally {
+    await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
-    await cleanAndGo(engine, server);
   }
 });
 
 add_task(async function test_sql_error() {
   Service.engineManager.register(SteamEngine);
   let engine = Service.engineManager.get("steam");
   engine.enabled = true;
   let server = serverForUsers({"foo": "password"}, {
@@ -544,18 +545,18 @@ add_task(async function test_sql_error()
   };
   try {
     _(`test_sql_error: Steam tracker contents: ${
       JSON.stringify(engine._tracker.changedIDs)}`);
     let ping = await sync_and_validate_telem(true);
     let enginePing = ping.engines.find(e => e.name === "steam");
     deepEqual(enginePing.failureReason, { name: "sqlerror", code: 1 });
   } finally {
+    await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
-    await cleanAndGo(engine, server);
   }
 });
 
 add_task(async function test_no_foreign_engines_in_success_ping() {
   Service.engineManager.register(BogusEngine);
   let engine = Service.engineManager.get("bogus");
   engine.enabled = true;
   let server = serverForUsers({"foo": "password"}, {
@@ -563,18 +564,18 @@ add_task(async function test_no_foreign_
     steam: {}
   });
 
   await SyncTestingInfrastructure(server);
   try {
     let ping = await sync_and_validate_telem();
     ok(ping.engines.every(e => e.name !== "bogus"));
   } finally {
+    await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
-    await cleanAndGo(engine, server);
   }
 });
 
 add_task(async function test_events() {
   Service.engineManager.register(BogusEngine);
   let engine = Service.engineManager.get("bogus");
   engine.enabled = true;
   let server = serverForUsers({"foo": "password"}, {
@@ -607,18 +608,18 @@ add_task(async function test_events() {
 
     Service.recordTelemetryEvent("object", "method", undefined, { foo: "bar" });
     ping = await wait_for_ping(() => Service.sync(), false, true);
     equal(ping.events.length, 1);
     equal(ping.events[0].length, 6);
     [timestamp, category, method, object, value, extra] = ping.events[0];
     equal(value, null);
   } finally {
+    await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
-    await cleanAndGo(engine, server);
   }
 });
 
 add_task(async function test_invalid_events() {
   Service.engineManager.register(BogusEngine);
   let engine = Service.engineManager.get("bogus");
   engine.enabled = true;
   let server = serverForUsers({"foo": "password"}, {
@@ -651,18 +652,18 @@ add_task(async function test_invalid_eve
     await checkNotRecorded("object", "method", "value", badextra);
     badextra = { "x": long86 };
     await checkNotRecorded("object", "method", "value", badextra);
     for (let i = 0; i < 10; i++) {
       badextra["name" + i] = "x";
     }
     await checkNotRecorded("object", "method", "value", badextra);
   } finally {
+    await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
-    await cleanAndGo(engine, server);
   }
 });
 
 add_task(async function test_no_ping_for_self_hosters() {
   let telem = get_sync_test_telemetry();
   let oldSubmit = telem.submit;
 
   Service.engineManager.register(BogusEngine);
@@ -683,12 +684,12 @@ add_task(async function test_no_ping_for
     });
     Service.sync();
     let pingSubmitted = await submitPromise;
     // The Sync testing infrastructure already sets up a custom token server,
     // so we don't need to do anything to simulate a self-hosted user.
     ok(!pingSubmitted, "Should not submit ping with custom token server URL");
   } finally {
     telem.submit = oldSubmit;
+    await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
-    await cleanAndGo(engine, server);
   }
 });