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 125288 2c2f92101a694a75b453c76f07debf9959e7acbd
parent 125287 30de61bc83219b81cd5e3206181409462df482c5
child 125289 b59ec40e33924950f3b603069bb1665a0771c42a
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [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();
 }