Issue 2040 - Adapted tests for chrome.storage.local

chrome/content/tests/prefs.js

Index: chrome/content/tests/prefs.js
===================================================================
--- a/chrome/content/tests/prefs.js
+++ b/chrome/content/tests/prefs.js
@@ -13,31 +13,41 @@
     }
   });
 
-  function prefExists(name)
+  function checkPrefExists(name, expectedValue, description, assert)
   {
     if ("chrome" in window)

[Wladimir Palant, 2015/02/26 14:43:11]

Looks like this test doesn't consider Safari. Is there already an issue to get
the Safari implementation tested as well?

[Sebastian Noack, 2015/02/26 16:03:43]

Tests are currently and ever since pretty much broken on Safari. The largest
issue there is that we can't access the background page there, at least if want
to get rid of the messaging proxy. Also some extension APIs that can be accessed
from extension pages on Chrome, can only be accessed from the background page on
Safari.

So eventually, if we want to run tests on Safari, we have to run the tests in
the background page, showing the results somehow on a page. I have the feeling
that this is getting difficult to impossible with QUnit.

-      return name in localStorage;
+    {
+      let done = assert.async();
+	  let key = "pref:" + name;

[Wladimir Palant, 2015/02/26 14:43:11]

Nit: no smart tabs please.

[Sebastian Noack, 2015/02/26 16:03:43]

Just regular tabs. I'm still not a fan of spaces. So I manually switch from tabs
to spaces when working on our files, and apparently forgot once. ;)

+      chrome.storage.local.get([key], function(items)

[Wladimir Palant, 2015/02/26 14:43:11]

According to documentation, passing in key without wrapping it in an array
should work as well.

[Sebastian Noack, 2015/02/26 16:03:43]

Yes, it does, but I find it weird. Since you always get an object of
key-value-pairs in the callback, it's kinda inconsistent when passing an
integral. But I don't mind, changed it.

+      {
+        equal(key in items, expectedValue, description);
+        done();
+      });
+    }
     else
-      return Services.prefs.prefHasUserValue("extensions.adblockplus." + name);
+    {
+      equal(Services.prefs.prefHasUserValue("extensions.adblockplus." + name), expectedValue, description);
+    }
   }
 
-  function checkPref(name, expectedValue, description)
+  function checkPref(name, expectedValue, description, assert)
   {
-    let value = null;
     if ("chrome" in window)
     {
-      try
+      let done = assert.async();
+	  let key = "pref:" + name;

[Wladimir Palant, 2015/02/26 14:43:11]

Nit: no smart tabs please.

[Sebastian Noack, 2015/02/26 16:03:43]

Done, here as well.

+      chrome.storage.local.get([key], function(items)

[Wladimir Palant, 2015/02/26 14:43:11]

As above, passing in key without wrapping it in an array should work as well.

[Sebastian Noack, 2015/02/26 16:03:43]

Done, here as wel.

       {
-        value = JSON.parse(localStorage[name]);
-      }
-      catch (e)
-      {
-        Cu.reportError(e);
-      }
+        deepEqual(items[key], expectedValue, description);
+        done();
+      });
     }
     else
     {
       let pref = "extensions.adblockplus." + name;
+      let value = null;
+
       switch (typeof expectedValue)
       {
         case "number":
@@ -53,60 +63,60 @@
           value = JSON.parse(Services.prefs.getComplexValue(pref, Ci.nsISupportsString).data);
           break;
       }
+
+      deepEqual(value, expectedValue, description);
     }
-    deepEqual(value, expectedValue, description);
   }
 
-  test("Numerical pref", function()
+  test("Numerical pref", function(assert)
   {
     Prefs.patternsbackups = 5;
     equal(Prefs.patternsbackups, 5, "Prefs object returns the correct value after setting pref to default value");
-    equal(prefExists("patternsbackups"), false, "User-defined pref has been removed");
+    checkPrefExists("patternsbackups", false, "User-defined pref has been removed", assert);
     Prefs.patternsbackups = 12;
     equal(Prefs.patternsbackups, 12, "Prefs object returns the correct value after setting pref to non-default value");
-    equal(prefExists("patternsbackups"), true, "User-defined pref has been created");
-    checkPref("patternsbackups", 12, "Value has been written");
+    checkPrefExists("patternsbackups", true, "User-defined pref has been created", assert);
+    checkPref("patternsbackups", 12, "Value has been written", assert);
   });
 
-  test("Boolean pref", function()
+  test("Boolean pref", function(assert)
   {
     Prefs.enabled = true;
     equal(Prefs.enabled, true, "Prefs object returns the correct value after setting pref to default value");
-    equal(prefExists("enabled"), false, "User-defined pref has been removed");
+    checkPrefExists("enabled", false, "User-defined pref has been removed", assert);
     Prefs.enabled = false;
     equal(Prefs.enabled, false, "Prefs object returns the correct value after setting pref to non-default value");
-    equal(prefExists("enabled"), true, "User-defined pref has been created");
-    checkPref("enabled", false, "Value has been written");
+    checkPrefExists("enabled", true, "User-defined pref has been created", assert);
+    checkPref("enabled", false, "Value has been written", assert);
   });
 
-  test("String pref", function()
+  test("String pref", function(assert)
   {
     let defaultValue = "https://notification.adblockplus.org/notification.json";
     Prefs.notificationurl = defaultValue;
     equal(Prefs.notificationurl, defaultValue, "Prefs object returns the correct value after setting pref to default value");
-    equal(prefExists("notificationurl"), false, "User-defined pref has been removed");
+    checkPrefExists("notificationurl", false, "User-defined pref has been removed", assert);
 
     let newValue = "https://notification.adblockplus.org/foo\u1234bar.json";
     Prefs.notificationurl = newValue;
     equal(Prefs.notificationurl, newValue, "Prefs object returns the correct value after setting pref to non-default value");
-    equal(prefExists("notificationurl"), true, "User-defined pref has been created");
-    checkPref("notificationurl", newValue, "Value has been written");
+    checkPrefExists("notificationurl", true, "User-defined pref has been created", assert);
+    checkPref("notificationurl", newValue, "Value has been written", assert);
   });
 
-  test("Object pref (complete replacement)", function()
+  test("Object pref (complete replacement)", function(assert)
   {
     Prefs.notificationdata = {};
     deepEqual(Prefs.notificationdata, {}, "Prefs object returns the correct value after setting pref to default value");
-    equal(prefExists("notificationdata"), false, "User-defined pref has been removed");

[Sebastian Noack, 2015/02/26 12:52:38]

Do we really need to make sure that the item is removed from the backend
storage, if the pref gets reverted to an empty object?

I removed that test and the respective logic from the Prefs implementation in
the platform module because:

1. There is no simple way to compare objects. The old code merely compared there
JSON representation, which only works relaibly if one of the objects is empty.
However, the new implementation doesn't need to decode/encode JSON anymore.
2. At least in the platform module, "notificationdata" and "stats_total" are the
only prefs using objects. And both are never reverted to an empty object.

[Wladimir Palant, 2015/02/26 14:43:11]

The point of this test isn't really ensuring that the preference is removed,
rather making sure that it is created even if it didn't exist before. As such,
it is a prerequisite for the following checks.

[Sebastian Noack, 2015/02/26 16:03:43]

So I suppose removing this line (and the same below) is fine? 

 
     let newValue = {foo:1, bar: "adsf\u1234"};
     Prefs.notificationdata = newValue;
     equal(Prefs.notificationdata, newValue, "Prefs object returns the correct value after setting pref to non-default value");
-    equal(prefExists("notificationdata"), true, "User-defined pref has been created");
-    checkPref("notificationdata", newValue, "Value has been written");
+    checkPrefExists("notificationdata", true, "User-defined pref has been created", assert);
+    checkPref("notificationdata", newValue, "Value has been written", assert);
   });
 
-  test("Property-wise modification", function()
+  test("Property-wise modification", function(assert)
   {
     Prefs.notificationdata = {};
 
@@ -114,13 +124,12 @@
     Prefs.notificationdata.bar = 2;
     Prefs.notificationdata = JSON.parse(JSON.stringify(Prefs.notificationdata));
     deepEqual(Prefs.notificationdata, {foo:1, bar: 2}, "Prefs object returns the correct value after setting pref to non-default value");
-    equal(prefExists("notificationdata"), true, "User-defined pref has been created");
-    checkPref("notificationdata", {foo:1, bar: 2}, "Value has been written");
+    checkPrefExists("notificationdata", true, "User-defined pref has been created", assert);
+    checkPref("notificationdata", {foo:1, bar: 2}, "Value has been written", assert);
 
     delete Prefs.notificationdata.foo;
     delete Prefs.notificationdata.bar;
     Prefs.notificationdata = JSON.parse(JSON.stringify(Prefs.notificationdata));
     deepEqual(Prefs.notificationdata, {}, "Prefs object returns the correct value after setting pref to default value");
-    equal(prefExists("notificationdata"), false, "User-defined pref has been removed");
   });
 })();