Bug 819262 - Additional tests for bookmark application in Sync. r=gps
authorRichard Newman <rnewman@mozilla.com>
Fri, 14 Dec 2012 14:00:40 -0800
changeset 117357 d6176d70331d5a1c50da4660621544e3d1ea0763
parent 117356 4eb4b9205aa4b7d61f95fd682c836aaf15d9af1e
child 117358 f686da9aa3ca4d1f5b4ff7a40b87d81604365bcd
push id24098
push userrnewman@mozilla.com
push dateThu, 03 Jan 2013 03:39:06 +0000
treeherdermozilla-central@6955309291ee [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgps
bugs819262
milestone20.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 819262 - Additional tests for bookmark application in Sync. r=gps
services/sync/modules-testing/utils.js
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
--- a/services/sync/modules-testing/utils.js
+++ b/services/sync/modules-testing/utils.js
@@ -56,17 +56,17 @@ this.setBasicCredentials =
 this.SyncTestingInfrastructure =
  function SyncTestingInfrastructure(username, password, syncKey) {
   let ns = {};
   Cu.import("resource://services-sync/service.js", ns);
 
   let auth = ns.Service.identity;
   auth.account = username || "foo";
   auth.basicPassword = password || "password";
-  auth.syncKey = syncKey || "foo";
+  auth.syncKey = syncKey || "abcdeabcdeabcdeabcdeabcdea";
 
   ns.Service.serverURL = TEST_SERVER_URL;
   ns.Service.clusterURL = TEST_CLUSTER_URL;
 
   this.logStats = initTestLogging();
   this.fakeFilesystem = new FakeFilesystemService({});
   this.fakeGUIDService = new FakeGUIDService();
   this.fakeCryptoService = new FakeCryptoService();
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -747,25 +747,40 @@ SyncEngine.prototype = {
 
     this._log.info(Object.keys(this._modified).length +
                    " outgoing items pre-reconciliation");
 
     // Keep track of what to delete at the end of sync
     this._delete = {};
   },
 
-  // Process incoming records
-  _processIncoming: function SyncEngine__processIncoming() {
+  /**
+   * A tiny abstraction to make it easier to test incoming record
+   * application.
+   */
+  _itemSource: function () {
+    return new Collection(this.engineURL, this._recordObj, this.service);
+  },
+
+  /**
+   * Process incoming records.
+   * In the most awful and untestable way possible.
+   * This now accepts something that makes testing vaguely less impossible.
+   */
+  _processIncoming: function (newitems) {
     this._log.trace("Downloading & applying server changes");
 
     // Figure out how many total items to fetch this sync; do less on mobile.
     let batchSize = Infinity;
-    let newitems = new Collection(this.engineURL, this._recordObj, this.service);
     let isMobile = (Svc.Prefs.get("client.type") == "mobile");
 
+    if (!newitems) {
+      newitems = this._itemSource();
+    }
+
     if (isMobile) {
       batchSize = MOBILE_BATCH_SIZE;
     }
     newitems.newer = this.lastSync;
     newitems.full  = true;
     newitems.limit = batchSize;
     
     // applied    => number of items that should be applied.
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -364,19 +364,19 @@ BookmarksEngine.prototype = {
       }
       delete this._guidMap;
       return this._guidMap = guidMap;
     });
 
     this._store._childrenToOrder = {};
   },
 
-  _processIncoming: function _processIncoming() {
+  _processIncoming: function (newitems) {
     try {
-      SyncEngine.prototype._processIncoming.call(this);
+      SyncEngine.prototype._processIncoming.call(this, newitems);
     } finally {
       // Reorder children.
       this._tracker.ignoreAll = true;
       this._store._orderChildren();
       this._tracker.ignoreAll = false;
       delete this._store._childrenToOrder;
     }
   },
@@ -387,29 +387,37 @@ BookmarksEngine.prototype = {
   },
 
   _syncCleanup: function _syncCleanup() {
     SyncEngine.prototype._syncCleanup.call(this);
     delete this._guidMap;
   },
 
   _createRecord: function _createRecord(id) {
-    // Create the record like normal but mark it as having dupes if necessary
+    // Create the record as usual, but mark it as having dupes if necessary.
     let record = SyncEngine.prototype._createRecord.call(this, id);
     let entry = this._mapDupe(record);
-    if (entry != null && entry.hasDupe)
+    if (entry != null && entry.hasDupe) {
       record.hasDupe = true;
+    }
     return record;
   },
 
   _findDupe: function _findDupe(item) {
-    // Don't bother finding a dupe if the incoming item has duplicates
-    if (item.hasDupe)
+    this._log.trace("Finding dupe for " + item.id +
+                    " (already duped: " + item.hasDupe + ").");
+
+    // Don't bother finding a dupe if the incoming item has duplicates.
+    if (item.hasDupe) {
+      this._log.trace(item.id + " already a dupe: not finding one.");
       return;
-    return this._mapDupe(item);
+    }
+    let mapped = this._mapDupe(item);
+    this._log.debug(item.id + " mapped to " + mapped);
+    return mapped;
   }
 };
 
 function BookmarksStore(name, engine) {
   Store.call(this, name, engine);
 
   // Explicitly nullify our references to our cached services so we don't leak
   Svc.Obs.add("places-shutdown", function() {
@@ -480,16 +488,17 @@ BookmarksStore.prototype = {
       }
     }
     finally {
       tags.containerOpen = false;
     }
   },
   
   applyIncoming: function BStore_applyIncoming(record) {
+    this._log.debug("Applying record " + record.id);
     let isSpecial = record.id in kSpecialIds;
 
     if (record.deleted) {
       if (isSpecial) {
         this._log.warn("Ignoring deletion for special record " + record.id);
         return;
       }
 
@@ -516,22 +525,24 @@ BookmarksStore.prototype = {
     // Preprocess the record before doing the normal apply.
     this.preprocessTagQuery(record);
 
     // Figure out the local id of the parent GUID if available
     let parentGUID = record.parentid;
     if (!parentGUID) {
       throw "Record " + record.id + " has invalid parentid: " + parentGUID;
     }
+    this._log.debug("Local parent is " + parentGUID);
 
     let parentId = this.idForGUID(parentGUID);
     if (parentId > 0) {
       // Save the parent id for modifying the bookmark later
       record._parent = parentId;
       record._orphan = false;
+      this._log.debug("Record " + record.id + " is not an orphan.");
     } else {
       this._log.trace("Record " + record.id +
                       " is an orphan: could not find parent " + parentGUID);
       record._orphan = true;
     }
 
     // Do the normal processing of incoming records
     Store.prototype.applyIncoming.call(this, record);
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -288,28 +288,28 @@ add_test(function test_restorePromptsReu
   } finally {
     store.wipe();
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
     server.stop(run_next_test);
   }
 });
 
+function FakeRecord(constructor, r) {
+  constructor.call(this, "bookmarks", r.id);
+  for (let x in r) {
+    this[x] = r[x];
+  }
+}
+
 // Bug 632287.
 add_test(function test_mismatched_types() {
   _("Ensure that handling a record that changes type causes deletion " +
     "then re-adding.");
 
-  function FakeRecord(constructor, r) {
-    constructor.call(this, "bookmarks", r.id);
-    for (let x in r) {
-      this[x] = r[x];
-    }
-  }
-
   let oldRecord = {
     "id": "l1nZZXfB8nC7",
     "type":"folder",
     "parentName":"Bookmarks Toolbar",
     "title":"Innerst i Sneglehode",
     "description":null,
     "parentid": "toolbar"
   };
@@ -463,18 +463,78 @@ add_test(function test_bookmark_tag_but_
 
   store.create(record);
   record.tags = ["bar"];
   store.update(record);
 
   run_next_test();
 });
 
+add_test(function test_misreconciled_root() {
+  _("Ensure that we don't reconcile an arbitrary record with a root.");
+
+  new SyncTestingInfrastructure();
+
+  let engine = new BookmarksEngine(Service);
+  let store = engine._store;
+
+  // Log real hard for this test.
+  store._log.trace = store._log.debug;
+  engine._log.trace = engine._log.debug;
+
+  engine._syncStartup();
+
+  // Let's find out where the toolbar is right now.
+  let toolbarBefore = store.createRecord("toolbar", "bookmarks");
+  let toolbarIDBefore = store.idForGUID("toolbar");
+  do_check_neq(-1, toolbarIDBefore);
+
+  let parentGUIDBefore = toolbarBefore.parentid;
+  let parentIDBefore = store.idForGUID(parentGUIDBefore);
+  do_check_neq(-1, parentIDBefore);
+  do_check_eq("string", typeof(parentGUIDBefore));
+
+  _("Current parent: " + parentGUIDBefore + " (" + parentIDBefore + ").");
+
+  let to_apply = {
+    id: "zzzzzzzzzzzz",
+    type: "folder",
+    title: "Bookmarks Toolbar",
+    description: "Now you're for it.",
+    parentName: "",
+    parentid: "mobile",   // Why not?
+    children: [],
+  };
+
+  let rec = new FakeRecord(BookmarkFolder, to_apply);
+  let encrypted = encryptPayload(rec.cleartext);
+  encrypted.decrypt = function () {
+    for (let x in rec) {
+      encrypted[x] = rec[x];
+    }
+  };
+
+  _("Applying record.");
+  engine._processIncoming({
+    get: function () {
+      this.recordHandler(encrypted);
+      return {success: true}
+    },
+  });
+
+  // Ensure that afterwards, toolbar is still there.
+  // As of 2012-12-05, this only passes because Places doesn't use "toolbar" as
+  // the real GUID, instead using a generated one. Sync does the translation.
+  let toolbarAfter = store.createRecord("toolbar", "bookmarks");
+  let parentGUIDAfter = toolbarAfter.parentid;
+  let parentIDAfter = store.idForGUID(parentGUIDAfter);
+  do_check_eq(store.GUIDForId(toolbarIDBefore), "toolbar");
+  do_check_eq(parentGUIDBefore, parentGUIDAfter);
+  do_check_eq(parentIDBefore, parentIDAfter);
+
+  run_next_test();
+});
+
 function run_test() {
   initTestLogging("Trace");
-  Log4Moz.repository.getLogger("Sync.Engine.Bookmarks").level  = Log4Moz.Level.Trace;
-  Log4Moz.repository.getLogger("Sync.Store.Bookmarks").level   = Log4Moz.Level.Trace;
-  Log4Moz.repository.getLogger("Sync.Tracker.Bookmarks").level = Log4Moz.Level.Trace;
-
   generateNewKeys(Service.collectionKeys);
-
   run_next_test();
 }