Bug 1394525 - Reset tracker scores before syncing. r=markh

MozReview-Commit-ID: 1wHdOeiZOyl

--HG--
extra : rebase_source : 765094feecdbe79cfa30f5c5a2886f8113890077

services/sync/modules/engines.js

@@ -1047,6 +1047,7 @@ SyncEngine.prototype = {
     // Clear the tracker now. If the sync fails we'll add the ones we failed
     // to upload back.
     this._tracker.clearChangedIDs();
+    this._tracker.resetScore();
 
     this._log.info(this._modified.count() +
                    " outgoing items pre-reconciliation");
@@ -1729,7 +1730,6 @@ SyncEngine.prototype = {
   // Save the current snapshot so as to calculate changes at next sync
   async _syncFinish() {
     this._log.trace("Finishing up sync");
-    this._tracker.resetScore();
 
     let doDelete = async (key, val) => {
       let coll = new Collection(this.engineURL, this._recordObj, this.service);

services/sync/tests/unit/test_engine_changes_during_sync.js

@@ -1,5 +1,6 @@
 Cu.import("resource://gre/modules/FormHistory.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
+Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/engines/bookmarks.js");
 Cu.import("resource://services-sync/engines/history.js");
@@ -43,19 +44,17 @@ add_task(async function test_history_change_during_sync() {
   await SyncTestingInfrastructure(server);
   let collection = server.user("foo").collection("history");
 
-  // Override `applyIncomingBatch` to insert a record while we're applying
+  // Override `uploadOutgoing` to insert a record while we're applying
   // changes. The tracker should ignore this change.
-  let { applyIncomingBatch } = engine._store;
-  engine._store.applyIncomingBatch = async function(records) {
-    _("Inserting local history visit");
-    engine._store.applyIncomingBatch = applyIncomingBatch;
-    let failed;
+  let uploadOutgoing = engine._uploadOutgoing;
+  engine._uploadOutgoing = async function() {
+    engine._uploadOutgoing = uploadOutgoing;
     try {
-      await addVisit("during_sync");
+      await uploadOutgoing.call(this);
     } finally {
-      failed = await applyIncomingBatch.call(this, records);
+      _("Inserting local history visit");
+      await addVisit("during_sync");
     }
-    return failed;
   };
 
   Svc.Obs.notify("weave:engine:start-tracking");
@@ -81,12 +80,10 @@ add_task(async function test_history_change_during_sync() {
     strictEqual(Service.scheduler.globalScore, 0,
       "Should not bump global score during second history sync");
 
-    // ...But we still won't ever upload the visit to the server, since we
-    // cleared the tracker after the first sync.
-    equal(collection.count(), 1,
-      "New local visit won't exist on server after second sync");
+    equal(collection.count(), 2,
+      "New local visit should exist on server after second sync");
   } finally {
-    engine._store.applyIncomingBatch = applyIncomingBatch;
+    engine._uploadOutgoing = uploadOutgoing;
     await cleanup(engine, server);
   }
 });
@@ -101,19 +98,17 @@ add_task(async function test_passwords_change_during_sync() {
   await SyncTestingInfrastructure(server);
   let collection = server.user("foo").collection("passwords");
 
-  let { applyIncomingBatch } = engine._store;
-  engine._store.applyIncomingBatch = async function(records) {
-    _("Inserting local password");
-    engine._store.applyIncomingBatch = applyIncomingBatch;
-    let failed;
+  let uploadOutgoing = engine._uploadOutgoing;
+  engine._uploadOutgoing = async function() {
+    engine._uploadOutgoing = uploadOutgoing;
     try {
+      await uploadOutgoing.call(this);
+    } finally {
+      _("Inserting local password");
       let login = new LoginInfo("https://example.com", "", null, "username",
         "password", "", "");
       Services.logins.addLogin(login);
-    } finally {
-      failed = await applyIncomingBatch.call(this, records);
     }
-    return failed;
   };
 
   Svc.Obs.notify("weave:engine:start-tracking");
@@ -140,12 +135,10 @@ add_task(async function test_passwords_change_during_sync() {
     strictEqual(Service.scheduler.globalScore, 0,
       "Should not bump global score during second passwords sync");
 
-    // ...But we still won't ever upload the entry to the server, since we
-    // cleared the tracker after the first sync.
-    equal(collection.count(), 1,
-      "New local password won't exist on server after second sync");
+    equal(collection.count(), 2,
+      "New local password should exist on server after second sync");
   } finally {
-    engine._store.applyIncomingBatch = applyIncomingBatch;
+    engine._uploadOutgoing = uploadOutgoing;
     await cleanup(engine, server);
   }
 });
@@ -162,18 +155,16 @@ add_task(async function test_prefs_change_during_sync() {
   await SyncTestingInfrastructure(server);
   let collection = server.user("foo").collection("prefs");
 
-  let { applyIncomingBatch } = engine._store;
-  engine._store.applyIncomingBatch = async function(records) {
-    _("Updating local pref value");
-    engine._store.applyIncomingBatch = applyIncomingBatch;
-    let failed;
+  let uploadOutgoing = engine._uploadOutgoing;
+  engine._uploadOutgoing = async function() {
+    engine._uploadOutgoing = uploadOutgoing;
     try {
+      await uploadOutgoing.call(this);
+    } finally {
+      _("Updating local pref value");
       // Change the value of a synced pref.
       Services.prefs.setCharPref(TEST_PREF, "hello");
-    } finally {
-      failed = await applyIncomingBatch.call(this, records);
     }
-    return failed;
   };
 
   Svc.Obs.notify("weave:engine:start-tracking");
@@ -204,10 +195,10 @@ add_task(async function test_prefs_change_during_sync() {
     payloads = collection.payloads();
     equal(payloads.length, 1,
       "Should not upload multiple prefs records after second sync");
-    equal(payloads[0].value[TEST_PREF], "world",
-      "Should not upload changed pref value during second sync");
+    equal(payloads[0].value[TEST_PREF], "hello",
+      "Should upload changed pref value during second sync");
   } finally {
-    engine._store.applyIncomingBatch = applyIncomingBatch;
+    engine._uploadOutgoing = uploadOutgoing;
     await cleanup(engine, server);
     Services.prefs.clearUserPref(TEST_PREF);
   }
@@ -223,12 +214,13 @@ add_task(async function test_forms_change_during_sync() {
   await SyncTestingInfrastructure(server);
   let collection = server.user("foo").collection("forms");
 
-  let { applyIncomingBatch } = engine._store;
-  engine._store.applyIncomingBatch = async function(records) {
-    _("Inserting local form history entry");
-    engine._store.applyIncomingBatch = applyIncomingBatch;
-    let failed;
+  let uploadOutgoing = engine._uploadOutgoing;
+  engine._uploadOutgoing = async function() {
+    engine._uploadOutgoing = uploadOutgoing;
     try {
+      await uploadOutgoing.call(this);
+    } finally {
+      _("Inserting local form history entry");
       await new Promise(resolve => {
         FormHistory.update([{
           op: "add",
@@ -238,10 +230,7 @@ add_task(async function test_forms_change_during_sync() {
           handleCompletion: resolve,
         });
       });
-    } finally {
-      failed = await applyIncomingBatch.call(this, records);
     }
-    return failed;
   };
 
   Svc.Obs.notify("weave:engine:start-tracking");
@@ -265,12 +254,10 @@ add_task(async function test_forms_change_during_sync() {
     strictEqual(Service.scheduler.globalScore, 0,
       "Should not bump global score during second forms sync");
 
-    // ...But we still won't ever upload the entry to the server, since we
-    // cleared the tracker after the first sync.
-    equal(collection.count(), 1,
-      "New local form won't exist on server after second sync");
+    equal(collection.count(), 2,
+      "New local form should exist on server after second sync");
   } finally {
-    engine._store.applyIncomingBatch = applyIncomingBatch;
+    engine._uploadOutgoing = uploadOutgoing;
     await cleanup(engine, server);
   }
 });
@@ -301,21 +288,19 @@ add_task(async function test_bookmark_change_during_sync() {
 
   let bmk3; // New child of Folder 1, created locally during sync.
 
-  let { applyIncomingBatch } = engine._store;
-  engine._store.applyIncomingBatch = async function(records) {
-    _("Inserting bookmark into local store");
-    engine._store.applyIncomingBatch = applyIncomingBatch;
-    let failed;
+  let uploadOutgoing = engine._uploadOutgoing;
+  engine._uploadOutgoing = async function() {
+    engine._uploadOutgoing = uploadOutgoing;
     try {
+      await uploadOutgoing.call(this);
+    } finally {
+      _("Inserting bookmark into local store");
       bmk3 = await PlacesUtils.bookmarks.insert({
         parentGuid: folder1.guid,
         url: "https://mozilla.org/",
         title: "Mozilla",
       });
-    } finally {
-      failed = await applyIncomingBatch.call(this, records);
     }
-    return failed;
   };
 
   // New bookmarks that should be uploaded during the first sync.
@@ -348,8 +333,8 @@ add_task(async function test_bookmark_change_during_sync() {
       remoteBzBmk.description = "New description";
       remoteBzBmk.title       = "Bugzilla";
       remoteBzBmk.tags        = ["new", "tags"];
-      remoteBzBmk.parentName  = "Bookmarks Toolbar";
-      remoteBzBmk.parentid    = "toolbar";
+      remoteBzBmk.parentName  = "Bookmarks Menu";
+      remoteBzBmk.parentid    = "menu";
       collection.insert(bzBmk.guid, encryptPayload(remoteBzBmk.cleartext));
 
       let remoteFolder = new BookmarkFolder("bookmarks", folder2_guid);
@@ -396,7 +381,7 @@ add_task(async function test_bookmark_change_during_sync() {
 
     let pingsPromise = wait_for_pings(2);
 
-    let changes = await engine.pullNewChanges();
+    let changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(Object.keys(changes).sort(), [
       folder1.guid,
       tbBmk.guid,
@@ -429,7 +414,7 @@ add_task(async function test_bookmark_change_during_sync() {
       guid: bmk2_guid,
     });
     ok(bmk2, "Remote bookmark should be applied during first sync");
-    await assertChildGuids(folder1.guid, [tbBmk.guid, bmk3.guid, bmk2_guid],
+    await assertChildGuids(folder1.guid, [tbBmk.guid, bmk2_guid, bmk3.guid],
       "Folder 1 should have 3 children after first sync");
     await assertChildGuids(folder2_guid, [bmk4_guid, tagQuery_guid],
       "Folder 2 should have 2 children after first sync");
@@ -438,7 +423,7 @@ add_task(async function test_bookmark_change_during_sync() {
     equal(taggedURIs[0].spec, "https://example.org/",
       "Synced tagged bookmark should appear in tagged URI list");
 
-    changes = await engine.pullNewChanges();
+    changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(changes, {},
       "Should have already uploaded changes in follow-up sync");
 
@@ -450,7 +435,7 @@ add_task(async function test_bookmark_change_during_sync() {
     ok(!engineData[0].validation, "Should not validate after first sync");
     ok(engineData[1].validation, "Should validate after second sync");
   } finally {
-    engine._store.applyIncomingBatch = applyIncomingBatch;
+    engine._uploadOutgoing = uploadOutgoing;
     await cleanup(engine, server);
   }
 });

services/sync/tests/unit/test_syncengine_sync.js

@@ -1539,23 +1539,6 @@ add_task(async function test_uploadOutgoing_largeRecords() {
 });
 
 
-add_task(async function test_syncFinish_noDelete() {
-  _("SyncEngine._syncFinish resets tracker's score");
-
-  let server = httpd_setup({});
-
-  await SyncTestingInfrastructure(server);
-  let engine = makeRotaryEngine();
-  engine._delete = {}; // Nothing to delete
-  engine._tracker.score = 100;
-
-  // _syncFinish() will reset the engine's score.
-  await engine._syncFinish();
-  do_check_eq(engine.score, 0);
-  server.stop(run_next_test);
-});
-
-
 add_task(async function test_syncFinish_deleteByIds() {
   _("SyncEngine._syncFinish deletes server records slated for deletion (list of record IDs).");