Bug 1343093 - Add integration test for bookmark repair. r=markh, a=gchang
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 21 Mar 2017 11:54:06 -0700
changeset 375450 76541e589505e12e12007524119b35b6612fca47
parent 375449 f476aef8eb569627cd0ea6612533ebf98031cb84
child 375451 286da438411aeae4be119112f2cd25bd8f76c6f0
push id10950
push userryanvm@gmail.com
push dateFri, 24 Mar 2017 19:39:10 +0000
treeherdermozilla-aurora@286da438411a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, gchang
bugs1343093
milestone54.0a2
Bug 1343093 - Add integration test for bookmark repair. r=markh, a=gchang This test replaces the clients engine to simulate two different clients, and uses `SOURCE_SYNC` to bypass change tracking. MozReview-Commit-ID: IF3b3WZtain
services/sync/modules/bookmark_repair.js
services/sync/modules/engines/clients.js
services/sync/tests/unit/test_bookmark_repair.js
services/sync/tests/unit/test_bookmark_repair_requestor.js
services/sync/tests/unit/test_bookmark_repair_responder.js
--- a/services/sync/modules/bookmark_repair.js
+++ b/services/sync/modules/bookmark_repair.js
@@ -84,22 +84,16 @@ const PREF = Object.freeze({
 });
 
 class BookmarkRepairRequestor extends CollectionRepairRequestor {
   constructor(service = null) {
     super(service);
     this.prefs = new Preferences(PREF_BRANCH);
   }
 
-  /* Exposed incase another module needs to understand our state
-  */
-  get STATE() {
-    return STATE;
-  }
-
   /* Check if any other clients connected to our account are current performing
      a repair. A thin wrapper which exists mainly for mocking during tests.
   */
   anyClientsRepairing(flowID) {
     return Doctor.anyClientsRepairing(this.service, "bookmarks", flowID);
   }
 
   /* Return a set of IDs we should request.
@@ -181,36 +175,36 @@ class BookmarkRepairRequestor extends Co
      Returns true if a repair was started and false otherwise.
   */
   startRepairs(validationInfo, flowID) {
     if (this._currentState != STATE.NOT_REPAIRING) {
       log.info(`Can't start a repair - repair with ID ${this._flowID} is already in progress`);
       return false;
     }
 
-    if (this.anyClientsRepairing()) {
-      log.info("Can't start repair, since other clients are already repairing bookmarks");
-      let extra = { flowID, reason: "other clients repairing" };
-      this.service.recordTelemetryEvent("repair", "aborted", undefined, extra)
-      return false;
-    }
-
     let ids = this.getProblemIDs(validationInfo);
     if (ids.size > MAX_REQUESTED_IDS) {
       log.info("Not starting a repair as there are over " + MAX_REQUESTED_IDS + " problems");
       let extra = { flowID, reason: `too many problems: ${ids.size}` };
       this.service.recordTelemetryEvent("repair", "aborted", undefined, extra)
       return false;
     }
 
     if (ids.size == 0) {
       log.info("Not starting a repair as there are no problems");
       return false;
     }
 
+    if (this.anyClientsRepairing()) {
+      log.info("Can't start repair, since other clients are already repairing bookmarks");
+      let extra = { flowID, reason: "other clients repairing" };
+      this.service.recordTelemetryEvent("repair", "aborted", undefined, extra)
+      return false;
+    }
+
     log.info(`Starting a repair, looking for ${ids.size} missing item(s)`);
     // setup our prefs to indicate we are on our way.
     this._flowID = flowID;
     this._currentIDs = Array.from(ids);
     this._currentState = STATE.NEED_NEW_CLIENT;
     this.service.recordTelemetryEvent("repair", "started", undefined, { flowID, numIDs: ids.size.toString() });
     return this.continueRepairs();
   }
@@ -715,8 +709,13 @@ class BookmarkRepairResponder extends Co
       reason: why,
     };
     this.service.recordTelemetryEvent("repairResponse", "aborted", undefined, eventExtra);
     // We could also consider writing a response here so the requestor can take
     // some immediate action rather than timing out, but we abort only in cases
     // that should be rare, so let's wait and see what telemetry tells us.
   }
 }
+
+/* Exposed in case another module needs to understand our state.
+*/
+BookmarkRepairRequestor.STATE = STATE;
+BookmarkRepairRequestor.PREF = PREF;
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -373,17 +373,17 @@ ClientEngine.prototype = {
       } else {
         const clientRecord = this._store._remoteClients[id];
         if (!commandChanges || !clientRecord) {
           // should be impossible, else we wouldn't have been writing it.
           this._log.warn("No command/No record changes for a client we uploaded");
           continue;
         }
         // fixup the client record, so our copy of _remoteClients matches what we uploaded.
-        clientRecord.commands = this._store.createRecord(id);
+        this._store._remoteClients[id] =  this._store.createRecord(id);
         // we could do better and pass the reference to the record we just uploaded,
         // but this will do for now
       }
     }
 
     // Re-add failed commands
     for (let id of failed) {
       const commandChanges = this._currentlySyncingCommands[id];
--- a/services/sync/tests/unit/test_bookmark_repair.js
+++ b/services/sync/tests/unit/test_bookmark_repair.js
@@ -1,45 +1,72 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 // Tests the bookmark repair requestor and responder end-to-end (ie, without
 // many mocks)
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
+Cu.import("resource://gre/modules/osfile.jsm");
+Cu.import("resource://services-sync/bookmark_repair.js");
 Cu.import("resource://services-sync/constants.js");
+Cu.import("resource://services-sync/doctor.js");
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/engines/clients.js");
 Cu.import("resource://services-sync/engines/bookmarks.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 
+const LAST_BOOKMARK_SYNC_PREFS = [
+  "bookmarks.lastSync",
+  "bookmarks.lastSyncLocal",
+];
+
+const BOOKMARK_REPAIR_STATE_PREFS = [
+  "client.GUID",
+  "doctor.lastRepairAdvance",
+  ...LAST_BOOKMARK_SYNC_PREFS,
+  ...Object.values(BookmarkRepairRequestor.PREF).map(name =>
+    `repairs.bookmarks.${name}`
+  ),
+];
+
 initTestLogging("Trace");
 Log.repository.getLogger("Sync.Engine.Bookmarks").level = Log.Level.Trace
 Log.repository.getLogger("Sync.Engine.Clients").level = Log.Level.Trace
 Log.repository.getLogger("Sqlite").level = Log.Level.Info; // less noisy
 
-const bms = PlacesUtils.bookmarks;
-
-//Service.engineManager.register(BookmarksEngine);
 let clientsEngine = Service.clientsEngine;
 let bookmarksEngine = Service.engineManager.get("bookmarks");
 
 generateNewKeys(Service.collectionKeys);
 
-function createFolder(parentId, title) {
-  let id = bms.createFolder(parentId, title, 0);
-  let guid = bookmarksEngine._store.GUIDForId(id);
-  return { id, guid };
+var recordedEvents = [];
+Service.recordTelemetryEvent = (object, method, value, extra = undefined) => {
+  recordedEvents.push({ object, method, value, extra });
+};
+
+function checkRecordedEvents(expected, message) {
+  deepEqual(recordedEvents, expected, message);
+  // and clear the list so future checks are easier to write.
+  recordedEvents = [];
 }
 
-function createBookmark(parentId, url, title, index = bms.DEFAULT_INDEX) {
-  let uri = Utils.makeURI(url);
-  let id = bms.insertBookmark(parentId, uri, index, title)
-  let guid = bookmarksEngine._store.GUIDForId(id);
-  return { id, guid };
+// Backs up and resets all preferences to their default values. Returns a
+// function that restores the preferences when called.
+function backupPrefs(names) {
+  let state = new Map();
+  for (let name of names) {
+    state.set(name, Svc.Prefs.get(name));
+    Svc.Prefs.reset(name);
+  }
+  return () => {
+    for (let [name, value] of state) {
+      Svc.Prefs.set(name, value);
+    }
+  };
 }
 
 async function promiseValidationDone(expected) {
   // wait for a validation to complete.
   let obs = promiseOneObserver("weave:engine:validate:finish");
   let { subject: validationResult } = await obs;
   // check the results - anything non-zero is checked against |expected|
   let summary = validationResult.problems.getSummary();
@@ -52,17 +79,17 @@ async function promiseValidationDone(exp
 async function cleanup(server) {
   bookmarksEngine._store.wipe();
   clientsEngine._store.wipe();
   Svc.Prefs.resetBranch("");
   Service.recordManager.clearCache();
   await promiseStopServer(server);
 }
 
-add_task(async function test_something() {
+add_task(async function test_bookmark_repair_integration() {
   enableValidationPrefs();
 
   _("Ensure that a validation error triggers a repair request.");
 
   let contents = {
     meta: {
       global: {
         engines: {
@@ -98,56 +125,200 @@ add_task(async function test_something()
       id: remoteID,
       name: "Remote client",
       type: "desktop",
       commands: [],
       version: "54",
       protocols: ["1.5"],
     }), Date.now() / 1000));
 
-    // Create a couple of bookmarks.
-    let folderInfo = createFolder(bms.toolbarFolder, "Folder 1");
-    let bookmarkInfo = createBookmark(folderInfo.id, "http://getfirefox.com/", "Get Firefox!");
-
-    let validationPromise = promiseValidationDone([]);
-    _("Syncing.");
-    Service.sync();
-    // should have 2 clients
-    equal(clientsEngine.stats.numClients, 2)
-    await validationPromise;
+    _("Create bookmark and folder");
+    let folderInfo = await PlacesUtils.bookmarks.insert({
+      parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      title: "Folder 1",
+    });
+    let bookmarkInfo = await PlacesUtils.bookmarks.insert({
+      parentGuid: folderInfo.guid,
+      url: "http://getfirefox.com/",
+      title: "Get Firefox!",
+    });
 
+    _(`Upload ${folderInfo.guid} and ${bookmarkInfo.guid} to server`);
+    let validationPromise = promiseValidationDone([]);
+    Service.sync();
+    equal(clientsEngine.stats.numClients, 2, "Clients collection should have 2 records");
+    await validationPromise;
+    checkRecordedEvents([], "Should not start repair after first sync");
+
+    _("Back up last sync timestamps for remote client");
+    let restoreRemoteLastBookmarkSync = backupPrefs(LAST_BOOKMARK_SYNC_PREFS);
+
+    _(`Delete ${bookmarkInfo.guid} locally and on server`);
     // Now we will reach into the server and hard-delete the bookmark
-    user.collection("bookmarks").wbo(bookmarkInfo.guid).delete();
+    user.collection("bookmarks").remove(bookmarkInfo.guid);
     // And delete the bookmark, but cheat by telling places that Sync did
-    // it, so we don't end up with an orphan.
-    // and use SQL to hard-delete the bookmark from the store.
-    await bms.remove(bookmarkInfo.guid, {
-      source: bms.SOURCE_SYNC,
+    // it, so we don't end up with a tombstone.
+    await PlacesUtils.bookmarks.remove(bookmarkInfo.guid, {
+      source: PlacesUtils.bookmarks.SOURCE_SYNC,
     });
-    // sanity check we aren't going to sync this removal.
-    do_check_empty(bookmarksEngine.pullNewChanges());
+    deepEqual(bookmarksEngine.pullNewChanges(), {},
+      `Should not upload tombstone for ${bookmarkInfo.guid}`);
 
     // sync again - we should have a few problems...
-    _("Syncing again.");
+    _("Sync again to trigger repair");
     validationPromise = promiseValidationDone([
       {"name":"missingChildren","count":1},
       {"name":"structuralDifferences","count":1},
     ]);
     Service.sync();
     await validationPromise;
+    let flowID = Svc.Prefs.get("repairs.bookmarks.flowID");
+    checkRecordedEvents([{
+      object: "repair",
+      method: "started",
+      value: undefined,
+      extra: {
+        flowID,
+        numIDs: "1",
+      },
+    }, {
+      object: "sendcommand",
+      method: "repairRequest",
+      value: undefined,
+      extra: {
+        flowID,
+        deviceID: Service.identity.hashedDeviceID(remoteID),
+      },
+    }, {
+      object: "repair",
+      method: "request",
+      value: "upload",
+      extra: {
+        deviceID: Service.identity.hashedDeviceID(remoteID),
+        flowID,
+        numIDs: "1",
+      },
+    }], "Should record telemetry events for repair request");
 
     // We should have started a repair with our second client.
-    equal(clientsEngine.getClientCommands(remoteID).length, 1);
-    _("Syncing so the outgoing client command is sent.");
+    equal(clientsEngine.getClientCommands(remoteID).length, 1,
+      "Should queue repair request for remote client after repair");
+    _("Sync to send outgoing repair request");
     Service.sync();
-    equal(clientsEngine.getClientCommands(remoteID).length, 0);
+    equal(clientsEngine.getClientCommands(remoteID).length, 0,
+      "Should send repair request to remote client after next sync");
+    checkRecordedEvents([],
+      "Should not record repair telemetry after sending repair request");
+
+    _("Back up repair state to restore later");
+    let restoreInitialRepairState = backupPrefs(BOOKMARK_REPAIR_STATE_PREFS);
 
     // so now let's take over the role of that other client!
+    _("Create new clients engine pretending to be remote client");
     let remoteClientsEngine = Service.clientsEngine = new ClientEngine(Service);
     remoteClientsEngine.localID = remoteID;
-    _("what could possibly go wrong?");
+
+    _("Restore missing bookmark");
+    // Pretend Sync wrote the bookmark, so that we upload it as part of the
+    // repair instead of the sync.
+    bookmarkInfo.source = PlacesUtils.bookmarks.SOURCE_SYNC;
+    await PlacesUtils.bookmarks.insert(bookmarkInfo);
+    restoreRemoteLastBookmarkSync();
+
+    _("Sync as remote client");
+    Service.sync();
+    checkRecordedEvents([{
+      object: "processcommand",
+      method: "repairRequest",
+      value: undefined,
+      extra: {
+        flowID,
+      },
+    }, {
+      object: "repairResponse",
+      method: "uploading",
+      value: undefined,
+      extra: {
+        flowID,
+        numIDs: "1",
+      },
+    }, {
+      object: "sendcommand",
+      method: "repairResponse",
+      value: undefined,
+      extra: {
+        flowID,
+        deviceID: Service.identity.hashedDeviceID(initialID),
+      },
+    }, {
+      object: "repairResponse",
+      method: "finished",
+      value: undefined,
+      extra: {
+        flowID,
+        numIDs: "1",
+      }
+    }], "Should record telemetry events for repair response");
+
+    // We should queue the repair response for the initial client.
+    equal(remoteClientsEngine.getClientCommands(initialID).length, 1,
+      "Should queue repair response for initial client after repair");
+    ok(user.collection("bookmarks").wbo(bookmarkInfo.guid),
+      "Should upload missing bookmark");
+
+    _("Sync to upload bookmark and send outgoing repair response");
     Service.sync();
+    equal(remoteClientsEngine.getClientCommands(initialID).length, 0,
+      "Should send repair response to initial client after next sync");
+    checkRecordedEvents([],
+      "Should not record repair telemetry after sending repair response");
+    ok(!Services.prefs.prefHasUserValue("services.sync.repairs.bookmarks.state"),
+      "Remote client should not be repairing");
 
-    // TODO - make the rest of this work!
+    _("Pretend to be initial client again");
+    Service.clientsEngine = clientsEngine;
+
+    _("Restore incomplete Places database and prefs");
+    await PlacesUtils.bookmarks.remove(bookmarkInfo.guid, {
+      source: PlacesUtils.bookmarks.SOURCE_SYNC,
+    });
+    restoreInitialRepairState();
+    ok(Services.prefs.prefHasUserValue("services.sync.repairs.bookmarks.state"),
+      "Initial client should still be repairing");
+
+    _("Sync as initial client");
+    let revalidationPromise = promiseValidationDone([]);
+    Service.sync();
+    let restoredBookmarkInfo = await PlacesUtils.bookmarks.fetch(bookmarkInfo.guid);
+    ok(restoredBookmarkInfo, "Missing bookmark should be downloaded to initial client");
+    checkRecordedEvents([{
+      object: "processcommand",
+      method: "repairResponse",
+      value: undefined,
+      extra: {
+        flowID,
+      },
+    }, {
+      object: "repair",
+      method: "response",
+      value: "upload",
+      extra: {
+        flowID,
+        deviceID: Service.identity.hashedDeviceID(remoteID),
+        numIDs: "1",
+      },
+    }, {
+      object: "repair",
+      method: "finished",
+      value: undefined,
+      extra: {
+        flowID,
+        numIDs: "0",
+      },
+    }]);
+    await revalidationPromise;
+    ok(!Services.prefs.prefHasUserValue("services.sync.repairs.bookmarks.state"),
+      "Should clear repair pref after successfully completing repair");
   } finally {
     await cleanup(server);
   }
 });
--- a/services/sync/tests/unit/test_bookmark_repair_requestor.js
+++ b/services/sync/tests/unit/test_bookmark_repair_requestor.js
@@ -128,26 +128,26 @@ add_task(async function test_requestor_o
       orphans: [],
     }
   }
   let flowID = Utils.makeGUID();
   requestor.startRepairs(validationInfo, flowID);
   // the command should now be outgoing.
   checkOutgoingCommand(mockService, "client-a");
 
-  checkState(requestor.STATE.SENT_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_REQUEST);
   // asking it to continue stays in that state until we timeout or the command
   // is removed.
   requestor.continueRepairs();
-  checkState(requestor.STATE.SENT_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_REQUEST);
 
   // now pretend that client synced.
   mockService.clientsEngine._sentCommands = {};
   requestor.continueRepairs();
-  checkState(requestor.STATE.SENT_SECOND_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_SECOND_REQUEST);
   // the command should be outgoing again.
   checkOutgoingCommand(mockService, "client-a");
 
   // pretend that client synced again without writing a command.
   mockService.clientsEngine._sentCommands = {};
   requestor.continueRepairs();
   // There are no more clients, so we've given up.
 
@@ -189,17 +189,17 @@ add_task(async function test_requestor_o
       orphans: [],
     }
   }
   let flowID = Utils.makeGUID();
   requestor.startRepairs(validationInfo, flowID);
   // the command should now be outgoing.
   checkOutgoingCommand(mockService, "client-a");
 
-  checkState(requestor.STATE.SENT_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_REQUEST);
 
   // pretend we are now in the future.
   let theFuture = Date.now() + 300000000;
   requestor._now = () => theFuture;
 
   requestor.continueRepairs();
 
   // We should be finished as we gave up in disgust.
@@ -240,17 +240,17 @@ add_task(async function test_requestor_l
         { parent: "x", child: "a" },
       ],
       orphans: [],
     }
   }
   requestor.startRepairs(validationInfo, Utils.makeGUID());
   // the repair command should be outgoing to the most-recent client.
   checkOutgoingCommand(mockService, "client-late");
-  checkState(requestor.STATE.SENT_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_REQUEST);
   // and this test is done - reset the repair.
   requestor.prefs.resetBranch();
 });
 
 add_task(async function test_requestor_client_vanishes() {
   let mockService = new MockService({
     "client-a": makeClientRecord("client-a"),
     "client-b": makeClientRecord("client-b"),
@@ -266,25 +266,25 @@ add_task(async function test_requestor_c
       orphans: [],
     }
   }
   let flowID = Utils.makeGUID();
   requestor.startRepairs(validationInfo, flowID);
   // the command should now be outgoing.
   checkOutgoingCommand(mockService, "client-a");
 
-  checkState(requestor.STATE.SENT_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_REQUEST);
 
   mockService.clientsEngine._sentCommands = {};
   // Now let's pretend the client vanished.
   delete mockService.clientsEngine._clientList["client-a"];
 
   requestor.continueRepairs();
   // We should have moved on to client-b.
-  checkState(requestor.STATE.SENT_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_REQUEST);
   checkOutgoingCommand(mockService, "client-b");
 
   // Now let's pretend client B wrote all missing IDs.
   let response = {
     collection: "bookmarks",
     request: "upload",
     flowID: requestor._flowID,
     clientID: "client-b",
@@ -344,30 +344,30 @@ add_task(async function test_requestor_s
       orphans: [],
     }
   }
   let flowID = Utils.makeGUID();
   requestor.startRepairs(validationInfo, flowID);
   // the command should now be outgoing.
   checkOutgoingCommand(mockService, "client-a");
 
-  checkState(requestor.STATE.SENT_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_REQUEST);
 
   mockService.clientsEngine._sentCommands = {};
   // Now let's pretend the client wrote a response.
   let response = {
     collection: "bookmarks",
     request: "upload",
     clientID: "client-a",
     flowID: requestor._flowID,
     ids: ["a", "b"],
   }
   requestor.continueRepairs(response);
   // We should have moved on to client 2.
-  checkState(requestor.STATE.SENT_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_REQUEST);
   checkOutgoingCommand(mockService, "client-b");
 
   // Now let's pretend client B write the missing ID.
   response = {
     collection: "bookmarks",
     request: "upload",
     clientID: "client-b",
     flowID: requestor._flowID,
@@ -464,17 +464,17 @@ add_task(async function test_requestor_a
       orphans: [],
     }
   }
   let flowID = Utils.makeGUID();
   requestor.startRepairs(validationInfo, flowID);
   // the command should now be outgoing.
   checkOutgoingCommand(mockService, "client-a");
 
-  checkState(requestor.STATE.SENT_REQUEST);
+  checkState(BookmarkRepairRequestor.STATE.SENT_REQUEST);
   mockService.clientsEngine._sentCommands = {};
 
   // Now let's pretend the client wrote a response (it doesn't matter what's in here)
   let response = {
     collection: "bookmarks",
     request: "upload",
     clientID: "client-a",
     flowID: requestor._flowID,
--- a/services/sync/tests/unit/test_bookmark_repair_responder.js
+++ b/services/sync/tests/unit/test_bookmark_repair_responder.js
@@ -16,17 +16,17 @@ initTestLogging("Trace");
 Log.repository.getLogger("Sync.Engine.Bookmarks").level = Log.Level.Trace;
 // sqlite logging generates lots of noise and typically isn't helpful here.
 Log.repository.getLogger("Sqlite").level = Log.Level.Error;
 
 // stub telemetry so we can easily check the right things are recorded.
 var recordedEvents = [];
 Service.recordTelemetryEvent = (object, method, value, extra = undefined) => {
   recordedEvents.push({ object, method, value, extra });
-}
+};
 
 function checkRecordedEvents(expected) {
   deepEqual(recordedEvents, expected);
   // and clear the list so future checks are easier to write.
   recordedEvents = [];
 }
 
 function getServerBookmarks(server) {