Bug 819262 - Additional tests for bookmark application in Sync. r=gps

services/sync/modules-testing/utils.js

@@ -61,7 +61,7 @@ this.SyncTestingInfrastructure =
   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;

services/sync/modules/engines.js

@@ -752,15 +752,30 @@ SyncEngine.prototype = {
     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;
     }

services/sync/modules/engines/bookmarks.js

@@ -369,9 +369,9 @@ BookmarksEngine.prototype = {
     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;
@@ -392,19 +392,27 @@ BookmarksEngine.prototype = {
   },
 
   _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;
   }
 };
 
@@ -485,6 +493,7 @@ BookmarksStore.prototype = {
   },
   
   applyIncoming: function BStore_applyIncoming(record) {
+    this._log.debug("Applying record " + record.id);
     let isSpecial = record.id in kSpecialIds;
 
     if (record.deleted) {
@@ -521,12 +530,14 @@ BookmarksStore.prototype = {
     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);

services/sync/tests/unit/test_bookmark_engine.js

@@ -293,18 +293,18 @@ add_test(function test_restorePromptsReupload() {
   }
 });
 
+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",
@@ -468,13 +468,73 @@ add_test(function test_bookmark_tag_but_no_uri() {
   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;
+add_test(function test_misreconciled_root() {
+  _("Ensure that we don't reconcile an arbitrary record with a root.");
 
-  generateNewKeys(Service.collectionKeys);
+  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");
+  generateNewKeys(Service.collectionKeys);
   run_next_test();
 }