1178453 - Change logging in storage-json.js to avoid string concatenation

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Matthew N. [:MattN]]

storage-json.js currently concatenates its arguments to the log function instead of letting Console.jsm do the concatenation and provide cleaner/coloured/inspectable output.

We should convert lines like:
`this.log("Getting login saving is enabled for " + hostname);`
to
`this.log("Getting login saving is enabled for ", hostname);`

to get the output and performance benefits.

Comment 1

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #0)
> to
> `this.log("Getting login saving is enabled for ", hostname);`

I meant to remove the space at the end of the string too:
`this.log("Getting login saving is enabled for", hostname);`

Comment 2

[Riadh Chtara[:rchtara]]

Attachment: Bug 1178453 - Change logging in storage-json.js to avoid string concatenation

Comment 3

[Riadh Chtara[:rchtara]]

Comment on attachment 8627885 [details] [diff] [review]
Bug 1178453 - Change logging in storage-json.js to avoid string concatenation

># HG changeset patch
># User Riadh Chtara <[e-mail]>
># Date 1435691212 25200
>#      Tue Jun 30 12:06:52 2015 -0700
># Node ID 978e40cfaad8d70c1f5ed26a86274b0b307bfd71
># Parent  aad95360a002fb54de07489313489743f5618cee
>Bug 1178453 - Change logging in storage-json.js to avoid string concatenation
>
>diff --git a/toolkit/components/passwordmgr/storage-json.js b/toolkit/components/passwordmgr/storage-json.js
>--- a/toolkit/components/passwordmgr/storage-json.js
>+++ b/toolkit/components/passwordmgr/storage-json.js
>@@ -55,17 +55,17 @@ this.LoginManagerStorage_json.prototype 
> 
>       // Set the reference to LoginStore synchronously.
>       let jsonPath = OS.Path.join(OS.Constants.Path.profileDir,
>                                   "logins.json");
>       this._store = new LoginStore(jsonPath);
> 
>       return Task.spawn(function () {
>         // Load the data asynchronously.
>-        this.log("Opening database at " + this._store.path);
>+        this.log("Opening database at", this._store.path);
>         yield this._store.load();
> 
>         // The import from previous versions operates the first time
>         // that this built-in storage back-end is used.  This may be
>         // later than expected, in case add-ons have registered an
>         // alternate storage that disabled the default one.
>         try {
>           if (Services.prefs.getBoolPref("signon.importedFromSqlite")) {
>@@ -87,17 +87,17 @@ this.LoginManagerStorage_json.prototype 
>           yield loginImport.import().catch(Cu.reportError);
>           this._store.saveSoon();
>         }
> 
>         // We won't attempt import again on next startup.
>         Services.prefs.setBoolPref("signon.importedFromSqlite", true);
>       }.bind(this)).catch(Cu.reportError);
>     } catch (e) {
>-      this.log("Initialization failed: " + e);
>+      this.log("Initialization failed:", e);
>       throw new Error("Initialization failed");
>     }
>   },
> 
> 
>   /*
>    * terminate
>    *
>@@ -252,17 +252,17 @@ this.LoginManagerStorage_json.prototype 
>    * Returns an array of nsILoginInfo.
>    */
>   getAllLogins : function (count) {
>     let [logins, ids] = this._searchLogins({});
> 
>     // decrypt entries for caller.
>     logins = this._decryptLogins(logins);
> 
>-    this.log("_getAllLogins: returning " + logins.length + " logins.");
>+    this.log("_getAllLogins: returning", logins.length, "logins.");
>     if (count)
>       count.value = logins.length; // needed for XPCOM
>     return logins;
>   },
> 
> 
>   /*
>    * searchLogins
>@@ -398,20 +398,20 @@ this.LoginManagerStorage_json.prototype 
>         } else {
>           fallbackLogins.push(login);
>           fallbackIds.push(loginItem.id);
>         }
>       }
>     }
> 
>     if (!foundLogins.length && fallbackLogins.length) {
>-      this.log("_searchLogins: returning " + fallbackLogins.length + " fallback logins");
>+      this.log("_searchLogins: returning", fallbackLogins.length, "fallback logins");
>       return [fallbackLogins, fallbackIds];
>     }
>-    this.log("_searchLogins: returning " + foundLogins.length + " logins");
>+    this.log("_searchLogins: returning", foundLogins.length, "logins");
>     return [foundLogins, foundIds];
>   },
> 
>   /*
>    * removeAllLogins
>    *
>    * Removes all logins from storage.
>    *
>@@ -432,46 +432,46 @@ this.LoginManagerStorage_json.prototype 
>    * getAllDisabledHosts
>    *
>    */
>   getAllDisabledHosts : function (count) {
>     this._store.ensureDataReady();
> 
>     let disabledHosts = this._store.data.disabledHosts.slice(0);
> 
>-    this.log("_getAllDisabledHosts: returning " + disabledHosts.length + " disabled hosts.");
>+    this.log("_getAllDisabledHosts: returning", disabledHosts.length, "disabled hosts.");
>     if (count)
>       count.value = disabledHosts.length; // needed for XPCOM
>     return disabledHosts;
>   },
> 
> 
>   /*
>    * getLoginSavingEnabled
>    *
>    */
>   getLoginSavingEnabled : function (hostname) {
>     this._store.ensureDataReady();
> 
>-    this.log("Getting login saving is enabled for " + hostname);
>+    this.log("Getting login saving is enabled for", hostname);
>     return this._store.data.disabledHosts.indexOf(hostname) == -1;
>   },
> 
> 
>   /*
>    * setLoginSavingEnabled
>    *
>    */
>   setLoginSavingEnabled : function (hostname, enabled) {
>     this._store.ensureDataReady();
> 
>     // Throws if there are bogus values.
>     LoginHelper.checkHostnameValue(hostname);
> 
>-    this.log("Setting login saving enabled for " + hostname + " to " + enabled);
>+    this.log("Setting login saving enabled for", hostname, "to", enabled);
>     let foundIndex = this._store.data.disabledHosts.indexOf(hostname);
>     if (enabled) {
>       if (foundIndex != -1) {
>         this._store.data.disabledHosts.splice(foundIndex, 1);
>         this._store.saveSoon();
>       }
>     } else {
>       if (foundIndex == -1) {
>@@ -498,17 +498,17 @@ this.LoginManagerStorage_json.prototype 
>     for each (let field in ["hostname", "formSubmitURL", "httpRealm"])
>       if (loginData[field] != '')
>         matchData[field] = loginData[field];
>     let [logins, ids] = this._searchLogins(matchData);
> 
>     // Decrypt entries found for the caller.
>     logins = this._decryptLogins(logins);
> 
>-    this.log("_findLogins: returning " + logins.length + " logins");
>+    this.log("_findLogins: returning", logins.length, "logins");
>     count.value = logins.length; // needed for XPCOM
>     return logins;
>   },
> 
> 
>   /*
>    * countLogins
>    *
>@@ -521,17 +521,17 @@ this.LoginManagerStorage_json.prototype 
>       httpRealm: httpRealm
>     };
>     let matchData = { };
>     for each (let field in ["hostname", "formSubmitURL", "httpRealm"])
>       if (loginData[field] != '')
>         matchData[field] = loginData[field];
>     let [logins, ids] = this._searchLogins(matchData);
> 
>-    this.log("_countLogins: counted logins: " + logins.length);
>+    this.log("_countLogins: counted logins:", logins.length);
>     return logins.length;
>   },
> 
> 
>   /*
>    * uiBusy
>    */
>   get uiBusy() {

Comment 4

[Riadh Chtara[:rchtara]]

Hello!
This the patch for this bug.
Could you please review it?
Thanxs

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for this change? :)

Comment 6

[Riadh Chtara[:rchtara]]

Attachment: Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields

Comment 7

[Riadh Chtara[:rchtara]]

Comment on attachment 8628338 [details] [diff] [review]
Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields

>diff --git a/services/sync/tests/unit/test_password_store.js b/services/sync/tests/unit/test_password_store.js
>--- a/services/sync/tests/unit/test_password_store.js
>+++ b/services/sync/tests/unit/test_password_store.js
>@@ -1,15 +1,203 @@
> /* Any copyright is dedicated to the Public Domain.
>    http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> Cu.import("resource://services-sync/engines/passwords.js");
> Cu.import("resource://services-sync/service.js");
> Cu.import("resource://services-sync/util.js");
> 
>+function checkRecord(hostname, formSubmitURL, httpRealm, name, record_count,
>+                     BOGUS_GUID) {
>+    let engine = Service.engineManager.get("passwords");
>+    let store = engine._store;
>+
>+    let count = {};
>+    let logins = Services.logins.findLogins(count, hostname, formSubmitURL,
>+                                            null);
>+
>+    _("Record" + name + ":" + JSON.stringify(logins));
>+    _("Count" + name + ":" +  count.value);
>+
>+    if (record_count > 0)
>+      do_check_true(!!store.getAllIDs()[BOGUS_GUID]);
>+    else
>+      do_check_true(!store.getAllIDs()[BOGUS_GUID]);
>+
>+}
>+
>+function time_test(timeCreated, timePasswordChanged) {
>+  initTestLogging("Trace");
>+  Log.repository.getLogger("Sync.Engine.Passwords").level = Log.Level.Trace;
>+  Log.repository.getLogger("Sync.Store.Passwords").level = Log.Level.Trace;
>+
>+  const BOGUS_GUID = "zzzzzzzzzzzz";
>+  let record = {id: BOGUS_GUID,
>+                  hostname: "http://foo.bar.com",
>+                  formSubmitURL: "http://foo.bar.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password"};
>+
>+  if (timeCreated !== null)
>+    record.timeCreated = timeCreated;
>+
>+  if (timePasswordChanged !== null)
>+    record.timePasswordChanged = timePasswordChanged;
>+
>+
>+  let engine = Service.engineManager.get("passwords");
>+  let store = engine._store;
>+  function applyEnsureNoFailures(records) {
>+    do_check_eq(store.applyIncomingBatch(records).length, 0);
>+  }
>+
>+  try {
>+    applyEnsureNoFailures([record]);
>+
>+    // The record needs always to be found.
>+    checkRecord(record.hostname, record.formSubmitURL, null, "", 1, BOGUS_GUID);
>+
>+  } finally {
>+    store.wipe();
>+  }
>+}
>+
>+function time_many_records_test() {
>+  initTestLogging("Trace");
>+  Log.repository.getLogger("Sync.Engine.Passwords").level = Log.Level.Trace;
>+  Log.repository.getLogger("Sync.Store.Passwords").level = Log.Level.Trace;
>+
>+  const BOGUS_GUID_A = "aaaaaaaaaaaa";
>+  const BOGUS_GUID_B = "bbbbbbbbbbbb";
>+  const BOGUS_GUID_C = "cccccccccccc";
>+  const BOGUS_GUID_D = "dddddddddddd";
>+
>+  const BOGUS_GUID_NOT_In_STORE_A = "zzzzzzzzzzzz";
>+  const BOGUS_GUID_NOT_In_STORE_B = "yyyyyyyyyyyy";
>+  const BOGUS_GUID_NOT_In_STORE_C = "xxxxxxxxxxxx";
>+  const BOGUS_GUID_NOT_In_STORE_D = "wwwwwwwwwwww";
>+
>+
>+  let recordA = {id: BOGUS_GUID_A,
>+                  hostname: "http://foo.a.com",
>+                  formSubmitURL: "http://foo.a.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password"};
>+
>+  let recordB = {id: BOGUS_GUID_B,
>+                  hostname: "http://foo.b.com",
>+                  formSubmitURL: "http://foo.b.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000};
>+
>+  let recordC = {id: BOGUS_GUID_C,
>+                  hostname: "http://foo.c.com",
>+                  formSubmitURL: "http://foo.c.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timePasswordChanged: 1000};
>+
>+  let recordD = {id: BOGUS_GUID_D,
>+                  hostname: "http://foo.d.com",
>+                  formSubmitURL: "http://foo.d.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000,
>+                  timePasswordChanged: 1000
>+                };
>+
>+  let recordNotInStoreA = {id: BOGUS_GUID_NOT_In_STORE_A,
>+                  hostname: "http://foo.aaaa.com",
>+                  formSubmitURL: "http://foo.aaaa.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password"};
>+
>+  let recordNotInStoreB = {id: BOGUS_GUID_NOT_In_STORE_B,
>+                  hostname: "http://foo.bbbb.com",
>+                  formSubmitURL: "http://foo.bbbb.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000};
>+
>+  let recordNotInStoreC = {id: BOGUS_GUID_NOT_In_STORE_C,
>+                  hostname: "http://foo.cccc.com",
>+                  formSubmitURL: "http://foo.cccc.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timePasswordChanged: 1000};
>+
>+  let recordNotInStoreD = {id: BOGUS_GUID_NOT_In_STORE_D,
>+                  hostname: "http://foo.dddd.com",
>+                  formSubmitURL: "http://foo.dddd.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000,
>+                  timePasswordChanged: 1000
>+                };
>+
>+
>+
>+  let engine = Service.engineManager.get("passwords");
>+  let store = engine._store;
>+  function applyEnsureNoFailures(records) {
>+    do_check_eq(store.applyIncomingBatch(records).length, 0);
>+  }
>+
>+  try {
>+    applyEnsureNoFailures([recordA, recordB, recordC, recordD]);
>+
>+    // The records need always to be found.
>+    checkRecord(recordA.hostname, recordA.formSubmitURL, null, "A", 1,
>+                BOGUS_GUID_A);
>+
>+    checkRecord(recordB.hostname, recordB.formSubmitURL, null, "B", 1,
>+                BOGUS_GUID_B);
>+
>+    checkRecord(recordC.hostname, recordC.formSubmitURL, null, "C", 1,
>+                BOGUS_GUID_C);
>+
>+    checkRecord(recordD.hostname, recordD.formSubmitURL, null, "D", 1,
>+                BOGUS_GUID_D);
>+
>+    // The records are not going to  be found.
>+    checkRecord(recordNotInStoreA.hostname, recordNotInStoreA.formSubmitURL,
>+                null, "NotInStoreA", 0, BOGUS_GUID_NOT_In_STORE_A);
>+
>+    checkRecord(recordNotInStoreB.hostname, recordNotInStoreB.formSubmitURL,
>+                null, "NotInStoreB", 0, BOGUS_GUID_NOT_In_STORE_B);
>+    checkRecord(recordNotInStoreC.hostname, recordNotInStoreC.formSubmitURL,
>+                null, "NotInStoreC", 0, BOGUS_GUID_NOT_In_STORE_C);
>+
>+    checkRecord(recordNotInStoreD.hostname, recordNotInStoreD.formSubmitURL,
>+                null, "NotInStoreD", 0, BOGUS_GUID_NOT_In_STORE_D);
>+
>+  } finally {
>+    store.wipe();
>+  }
>+}
>+
> function run_test() {
>   initTestLogging("Trace");
>   Log.repository.getLogger("Sync.Engine.Passwords").level = Log.Level.Trace;
>   Log.repository.getLogger("Sync.Store.Passwords").level = Log.Level.Trace;
> 
>   const BOGUS_GUID_A = "zzzzzzzzzzzz";
>   const BOGUS_GUID_B = "yyyyyyyyyyyy";
>   let recordA = {id: BOGUS_GUID_A,
>@@ -53,9 +241,17 @@ function run_test() {
>     do_check_eq(goodCount.value, 1);
>     do_check_eq(badCount.value, 0);
> 
>     do_check_true(!!store.getAllIDs()[BOGUS_GUID_B]);
>     do_check_true(!store.getAllIDs()[BOGUS_GUID_A]);
>   } finally {
>     store.wipe();
>   }
>-}
>+
>+  time_test(null, null);
>+  time_test(1000, null);
>+  time_test(null, 1000);
>+  time_test(1000, 1000);
>+
>+  time_many_records_test();
>+
>+}
>\ No newline at end of file

Comment 8

[Riadh Chtara[:rchtara]]

Comment on attachment 8628338 [details] [diff] [review]
Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields

>diff --git a/services/sync/tests/unit/test_password_store.js b/services/sync/tests/unit/test_password_store.js
>--- a/services/sync/tests/unit/test_password_store.js
>+++ b/services/sync/tests/unit/test_password_store.js
>@@ -1,15 +1,203 @@
> /* Any copyright is dedicated to the Public Domain.
>    http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> Cu.import("resource://services-sync/engines/passwords.js");
> Cu.import("resource://services-sync/service.js");
> Cu.import("resource://services-sync/util.js");
> 
>+function checkRecord(hostname, formSubmitURL, httpRealm, name, record_count,
>+                     BOGUS_GUID) {
>+    let engine = Service.engineManager.get("passwords");
>+    let store = engine._store;
>+
>+    let count = {};
>+    let logins = Services.logins.findLogins(count, hostname, formSubmitURL,
>+                                            null);
>+
>+    _("Record" + name + ":" + JSON.stringify(logins));
>+    _("Count" + name + ":" +  count.value);
>+
>+    if (record_count > 0)
>+      do_check_true(!!store.getAllIDs()[BOGUS_GUID]);
>+    else
>+      do_check_true(!store.getAllIDs()[BOGUS_GUID]);
>+
>+}
>+
>+function time_test(timeCreated, timePasswordChanged) {
>+  initTestLogging("Trace");
>+  Log.repository.getLogger("Sync.Engine.Passwords").level = Log.Level.Trace;
>+  Log.repository.getLogger("Sync.Store.Passwords").level = Log.Level.Trace;
>+
>+  const BOGUS_GUID = "zzzzzzzzzzzz";
>+  let record = {id: BOGUS_GUID,
>+                  hostname: "http://foo.bar.com",
>+                  formSubmitURL: "http://foo.bar.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password"};
>+
>+  if (timeCreated !== null)
>+    record.timeCreated = timeCreated;
>+
>+  if (timePasswordChanged !== null)
>+    record.timePasswordChanged = timePasswordChanged;
>+
>+
>+  let engine = Service.engineManager.get("passwords");
>+  let store = engine._store;
>+  function applyEnsureNoFailures(records) {
>+    do_check_eq(store.applyIncomingBatch(records).length, 0);
>+  }
>+
>+  try {
>+    applyEnsureNoFailures([record]);
>+
>+    // The record needs always to be found.
>+    checkRecord(record.hostname, record.formSubmitURL, null, "", 1, BOGUS_GUID);
>+
>+  } finally {
>+    store.wipe();
>+  }
>+}
>+
>+function time_many_records_test() {
>+  initTestLogging("Trace");
>+  Log.repository.getLogger("Sync.Engine.Passwords").level = Log.Level.Trace;
>+  Log.repository.getLogger("Sync.Store.Passwords").level = Log.Level.Trace;
>+
>+  const BOGUS_GUID_A = "aaaaaaaaaaaa";
>+  const BOGUS_GUID_B = "bbbbbbbbbbbb";
>+  const BOGUS_GUID_C = "cccccccccccc";
>+  const BOGUS_GUID_D = "dddddddddddd";
>+
>+  const BOGUS_GUID_NOT_In_STORE_A = "zzzzzzzzzzzz";
>+  const BOGUS_GUID_NOT_In_STORE_B = "yyyyyyyyyyyy";
>+  const BOGUS_GUID_NOT_In_STORE_C = "xxxxxxxxxxxx";
>+  const BOGUS_GUID_NOT_In_STORE_D = "wwwwwwwwwwww";
>+
>+
>+  let recordA = {id: BOGUS_GUID_A,
>+                  hostname: "http://foo.a.com",
>+                  formSubmitURL: "http://foo.a.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password"};
>+
>+  let recordB = {id: BOGUS_GUID_B,
>+                  hostname: "http://foo.b.com",
>+                  formSubmitURL: "http://foo.b.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000};
>+
>+  let recordC = {id: BOGUS_GUID_C,
>+                  hostname: "http://foo.c.com",
>+                  formSubmitURL: "http://foo.c.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timePasswordChanged: 1000};
>+
>+  let recordD = {id: BOGUS_GUID_D,
>+                  hostname: "http://foo.d.com",
>+                  formSubmitURL: "http://foo.d.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000,
>+                  timePasswordChanged: 1000
>+                };
>+
>+  let recordNotInStoreA = {id: BOGUS_GUID_NOT_In_STORE_A,
>+                  hostname: "http://foo.aaaa.com",
>+                  formSubmitURL: "http://foo.aaaa.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password"};
>+
>+  let recordNotInStoreB = {id: BOGUS_GUID_NOT_In_STORE_B,
>+                  hostname: "http://foo.bbbb.com",
>+                  formSubmitURL: "http://foo.bbbb.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000};
>+
>+  let recordNotInStoreC = {id: BOGUS_GUID_NOT_In_STORE_C,
>+                  hostname: "http://foo.cccc.com",
>+                  formSubmitURL: "http://foo.cccc.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timePasswordChanged: 1000};
>+
>+  let recordNotInStoreD = {id: BOGUS_GUID_NOT_In_STORE_D,
>+                  hostname: "http://foo.dddd.com",
>+                  formSubmitURL: "http://foo.dddd.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000,
>+                  timePasswordChanged: 1000
>+                };
>+
>+
>+
>+  let engine = Service.engineManager.get("passwords");
>+  let store = engine._store;
>+  function applyEnsureNoFailures(records) {
>+    do_check_eq(store.applyIncomingBatch(records).length, 0);
>+  }
>+
>+  try {
>+    applyEnsureNoFailures([recordA, recordB, recordC, recordD]);
>+
>+    // The records need always to be found.
>+    checkRecord(recordA.hostname, recordA.formSubmitURL, null, "A", 1,
>+                BOGUS_GUID_A);
>+
>+    checkRecord(recordB.hostname, recordB.formSubmitURL, null, "B", 1,
>+                BOGUS_GUID_B);
>+
>+    checkRecord(recordC.hostname, recordC.formSubmitURL, null, "C", 1,
>+                BOGUS_GUID_C);
>+
>+    checkRecord(recordD.hostname, recordD.formSubmitURL, null, "D", 1,
>+                BOGUS_GUID_D);
>+
>+    // The records are not going to  be found.
>+    checkRecord(recordNotInStoreA.hostname, recordNotInStoreA.formSubmitURL,
>+                null, "NotInStoreA", 0, BOGUS_GUID_NOT_In_STORE_A);
>+
>+    checkRecord(recordNotInStoreB.hostname, recordNotInStoreB.formSubmitURL,
>+                null, "NotInStoreB", 0, BOGUS_GUID_NOT_In_STORE_B);
>+    checkRecord(recordNotInStoreC.hostname, recordNotInStoreC.formSubmitURL,
>+                null, "NotInStoreC", 0, BOGUS_GUID_NOT_In_STORE_C);
>+
>+    checkRecord(recordNotInStoreD.hostname, recordNotInStoreD.formSubmitURL,
>+                null, "NotInStoreD", 0, BOGUS_GUID_NOT_In_STORE_D);
>+
>+  } finally {
>+    store.wipe();
>+  }
>+}
>+
> function run_test() {
>   initTestLogging("Trace");
>   Log.repository.getLogger("Sync.Engine.Passwords").level = Log.Level.Trace;
>   Log.repository.getLogger("Sync.Store.Passwords").level = Log.Level.Trace;
> 
>   const BOGUS_GUID_A = "zzzzzzzzzzzz";
>   const BOGUS_GUID_B = "yyyyyyyyyyyy";
>   let recordA = {id: BOGUS_GUID_A,
>@@ -53,9 +241,17 @@ function run_test() {
>     do_check_eq(goodCount.value, 1);
>     do_check_eq(badCount.value, 0);
> 
>     do_check_true(!!store.getAllIDs()[BOGUS_GUID_B]);
>     do_check_true(!store.getAllIDs()[BOGUS_GUID_A]);
>   } finally {
>     store.wipe();
>   }
>-}
>+
>+  time_test(null, null);
>+  time_test(1000, null);
>+  time_test(null, 1000);
>+  time_test(1000, 1000);
>+
>+  time_many_records_test();
>+
>+}
>\ No newline at end of file

Comment 9

[Riadh Chtara[:rchtara]]

Comment on attachment 8628338 [details] [diff] [review]
Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields

>diff --git a/services/sync/tests/unit/test_password_store.js b/services/sync/tests/unit/test_password_store.js
>--- a/services/sync/tests/unit/test_password_store.js
>+++ b/services/sync/tests/unit/test_password_store.js
>@@ -1,15 +1,203 @@
> /* Any copyright is dedicated to the Public Domain.
>    http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> Cu.import("resource://services-sync/engines/passwords.js");
> Cu.import("resource://services-sync/service.js");
> Cu.import("resource://services-sync/util.js");
> 
>+function checkRecord(hostname, formSubmitURL, httpRealm, name, record_count,
>+                     BOGUS_GUID) {
>+    let engine = Service.engineManager.get("passwords");
>+    let store = engine._store;
>+
>+    let count = {};
>+    let logins = Services.logins.findLogins(count, hostname, formSubmitURL,
>+                                            null);
>+
>+    _("Record" + name + ":" + JSON.stringify(logins));
>+    _("Count" + name + ":" +  count.value);
>+
>+    if (record_count > 0)
>+      do_check_true(!!store.getAllIDs()[BOGUS_GUID]);
>+    else
>+      do_check_true(!store.getAllIDs()[BOGUS_GUID]);
>+
>+}
>+
>+function time_test(timeCreated, timePasswordChanged) {
>+  initTestLogging("Trace");
>+  Log.repository.getLogger("Sync.Engine.Passwords").level = Log.Level.Trace;
>+  Log.repository.getLogger("Sync.Store.Passwords").level = Log.Level.Trace;
>+
>+  const BOGUS_GUID = "zzzzzzzzzzzz";
>+  let record = {id: BOGUS_GUID,
>+                  hostname: "http://foo.bar.com",
>+                  formSubmitURL: "http://foo.bar.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password"};
>+
>+  if (timeCreated !== null)
>+    record.timeCreated = timeCreated;
>+
>+  if (timePasswordChanged !== null)
>+    record.timePasswordChanged = timePasswordChanged;
>+
>+
>+  let engine = Service.engineManager.get("passwords");
>+  let store = engine._store;
>+  function applyEnsureNoFailures(records) {
>+    do_check_eq(store.applyIncomingBatch(records).length, 0);
>+  }
>+
>+  try {
>+    applyEnsureNoFailures([record]);
>+
>+    // The record needs always to be found.
>+    checkRecord(record.hostname, record.formSubmitURL, null, "", 1, BOGUS_GUID);
>+
>+  } finally {
>+    store.wipe();
>+  }
>+}
>+
>+function time_many_records_test() {
>+  initTestLogging("Trace");
>+  Log.repository.getLogger("Sync.Engine.Passwords").level = Log.Level.Trace;
>+  Log.repository.getLogger("Sync.Store.Passwords").level = Log.Level.Trace;
>+
>+  const BOGUS_GUID_A = "aaaaaaaaaaaa";
>+  const BOGUS_GUID_B = "bbbbbbbbbbbb";
>+  const BOGUS_GUID_C = "cccccccccccc";
>+  const BOGUS_GUID_D = "dddddddddddd";
>+
>+  const BOGUS_GUID_NOT_In_STORE_A = "zzzzzzzzzzzz";
>+  const BOGUS_GUID_NOT_In_STORE_B = "yyyyyyyyyyyy";
>+  const BOGUS_GUID_NOT_In_STORE_C = "xxxxxxxxxxxx";
>+  const BOGUS_GUID_NOT_In_STORE_D = "wwwwwwwwwwww";
>+
>+
>+  let recordA = {id: BOGUS_GUID_A,
>+                  hostname: "http://foo.a.com",
>+                  formSubmitURL: "http://foo.a.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password"};
>+
>+  let recordB = {id: BOGUS_GUID_B,
>+                  hostname: "http://foo.b.com",
>+                  formSubmitURL: "http://foo.b.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000};
>+
>+  let recordC = {id: BOGUS_GUID_C,
>+                  hostname: "http://foo.c.com",
>+                  formSubmitURL: "http://foo.c.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timePasswordChanged: 1000};
>+
>+  let recordD = {id: BOGUS_GUID_D,
>+                  hostname: "http://foo.d.com",
>+                  formSubmitURL: "http://foo.d.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000,
>+                  timePasswordChanged: 1000
>+                };
>+
>+  let recordNotInStoreA = {id: BOGUS_GUID_NOT_In_STORE_A,
>+                  hostname: "http://foo.aaaa.com",
>+                  formSubmitURL: "http://foo.aaaa.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password"};
>+
>+  let recordNotInStoreB = {id: BOGUS_GUID_NOT_In_STORE_B,
>+                  hostname: "http://foo.bbbb.com",
>+                  formSubmitURL: "http://foo.bbbb.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000};
>+
>+  let recordNotInStoreC = {id: BOGUS_GUID_NOT_In_STORE_C,
>+                  hostname: "http://foo.cccc.com",
>+                  formSubmitURL: "http://foo.cccc.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timePasswordChanged: 1000};
>+
>+  let recordNotInStoreD = {id: BOGUS_GUID_NOT_In_STORE_D,
>+                  hostname: "http://foo.dddd.com",
>+                  formSubmitURL: "http://foo.dddd.com/baz",
>+                  username: "john",
>+                  password: "smith",
>+                  usernameField: "username",
>+                  passwordField: "password",
>+                  timeCreated: 1000,
>+                  timePasswordChanged: 1000
>+                };
>+
>+
>+
>+  let engine = Service.engineManager.get("passwords");
>+  let store = engine._store;
>+  function applyEnsureNoFailures(records) {
>+    do_check_eq(store.applyIncomingBatch(records).length, 0);
>+  }
>+
>+  try {
>+    applyEnsureNoFailures([recordA, recordB, recordC, recordD]);
>+
>+    // The records need always to be found.
>+    checkRecord(recordA.hostname, recordA.formSubmitURL, null, "A", 1,
>+                BOGUS_GUID_A);
>+
>+    checkRecord(recordB.hostname, recordB.formSubmitURL, null, "B", 1,
>+                BOGUS_GUID_B);
>+
>+    checkRecord(recordC.hostname, recordC.formSubmitURL, null, "C", 1,
>+                BOGUS_GUID_C);
>+
>+    checkRecord(recordD.hostname, recordD.formSubmitURL, null, "D", 1,
>+                BOGUS_GUID_D);
>+
>+    // The records are not going to  be found.
>+    checkRecord(recordNotInStoreA.hostname, recordNotInStoreA.formSubmitURL,
>+                null, "NotInStoreA", 0, BOGUS_GUID_NOT_In_STORE_A);
>+
>+    checkRecord(recordNotInStoreB.hostname, recordNotInStoreB.formSubmitURL,
>+                null, "NotInStoreB", 0, BOGUS_GUID_NOT_In_STORE_B);
>+    checkRecord(recordNotInStoreC.hostname, recordNotInStoreC.formSubmitURL,
>+                null, "NotInStoreC", 0, BOGUS_GUID_NOT_In_STORE_C);
>+
>+    checkRecord(recordNotInStoreD.hostname, recordNotInStoreD.formSubmitURL,
>+                null, "NotInStoreD", 0, BOGUS_GUID_NOT_In_STORE_D);
>+
>+  } finally {
>+    store.wipe();
>+  }
>+}
>+
> function run_test() {
>   initTestLogging("Trace");
>   Log.repository.getLogger("Sync.Engine.Passwords").level = Log.Level.Trace;
>   Log.repository.getLogger("Sync.Store.Passwords").level = Log.Level.Trace;
> 
>   const BOGUS_GUID_A = "zzzzzzzzzzzz";
>   const BOGUS_GUID_B = "yyyyyyyyyyyy";
>   let recordA = {id: BOGUS_GUID_A,
>@@ -53,9 +241,17 @@ function run_test() {
>     do_check_eq(goodCount.value, 1);
>     do_check_eq(badCount.value, 0);
> 
>     do_check_true(!!store.getAllIDs()[BOGUS_GUID_B]);
>     do_check_true(!store.getAllIDs()[BOGUS_GUID_A]);
>   } finally {
>     store.wipe();
>   }
>-}
>+
>+  time_test(null, null);
>+  time_test(1000, null);
>+  time_test(null, 1000);
>+  time_test(1000, 1000);
>+
>+  time_many_records_test();
>+
>+}
>\ No newline at end of file

Comment 10

[Riadh Chtara[:rchtara]]

Hi Ryan,
Unfortunately, I don’t have try access yet.

Comment 11

[Riadh Chtara[:rchtara]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3413d17b3cc1

Comment 12

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/12405/

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7330514ef5e4

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7330514ef5e4