Issue 4223 - Adapt filter storage read/write tests to work in adblockpluscore repository

test/filterStorage_readwrite.js

Index: test/filterStorage_readwrite.js
===================================================================
--- a/test/filterStorage_readwrite.js
+++ b/test/filterStorage_readwrite.js
@@ -1,358 +1,315 @@
-(function()
-{
-  module("Filter storage read/write", {
-    setup: function()
-    {
-      prepareFilterComponents.call(this);
-      preparePrefs.call(this);
+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2016 Eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
 
-      FilterStorage.addSubscription(Subscription.fromURL("~fl~"));
-    },
-    teardown: function()
-    {
-      restoreFilterComponents.call(this);
-      restorePrefs.call(this);
-    }
+"use strict";
+
+let {createSandbox, unexpectedError} = require("./_common");
+
+let Filter = null;
+let FilterNotifier = null;
+let FilterStorage = null;
+let IO = null;
+let Prefs = null;
+let Subscription = null;
+let ExternalSubscription = null;
+
+exports.setUp = function(callback)
+{
+  let sandboxedRequire = createSandbox();
+  (
+    {Filter} = sandboxedRequire("../lib/filterClasses"),
+    {FilterNotifier} = sandboxedRequire("../lib/filterNotifier"),
+    {FilterStorage} = sandboxedRequire("../lib/filterStorage"),
+    {IO} = sandboxedRequire("./stub-modules/io"),
+    {Prefs} = sandboxedRequire("./stub-modules/prefs"),
+    {Subscription, ExternalSubscription} = sandboxedRequire("../lib/subscriptionClasses")
+  );
+
+  FilterStorage.addSubscription(Subscription.fromURL("~fl~"));
+  callback();
+}
+
+let testData = new Promise((resolve, reject) =>
+{
+  let fs = require("fs");
+  let path = require("path");
+  let datapath = path.resolve(__dirname, "data", "patterns.ini");

[kzar, 2016/10/06 05:13:38]

Did you forget to add the data/patterns.ini file?

[Wladimir Palant, 2016/10/06 07:13:59]

No, I copied the files in a separate commit - what you have here is merely a
diff from the original files. The patterns.ini we use for tests wasn't modified.

[kzar, 2016/10/06 08:06:47]

Acknowledged.

+
+  fs.readFile(datapath, "utf-8", (error, data) =>
+  {
+    if (error)
+      reject(error);
+    else
+      resolve(data);
   });
+});
 
-  let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", null);
-  let {NetUtil} = Cu.import("resource://gre/modules/NetUtil.jsm", null);
-
-  function loadFilters(file, callback)
+function loadFilters(file)
+{
+  return new Promise((resolve, reject) =>
   {
     let listener = function(action)

[kzar, 2016/10/06 05:13:38]

Seems like this logic is nearly identical in saveFilters, perhaps reuse it?

[Wladimir Palant, 2016/10/06 07:13:59]

Reusing isn't quite trivial but with the new FilterNotifier API this is no
longer an issue.

     {
       if (action == "load")
       {
         FilterNotifier.removeListener(listener);
-        callback();
+        resolve();
       }
     };
     FilterNotifier.addListener(listener);
 
     FilterStorage.loadFromDisk(file);
-  }
+  });
+}
 
-  function writeToFile(file, data)
-  {
-    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Ci.nsIScriptableUnicodeConverter);
-    converter.charset = "utf-8";
-    data = converter.ConvertFromUnicode(data);
-
-    let stream = FileUtils.openFileOutputStream(file);
-    stream.write(data, data.length);
-    stream.close();
-  }
-
-  function saveFilters(file, callback)
+function saveFilters(file, callback)
+{
+  return new Promise((resolve, reject) =>
   {
     let listener = function(action)
     {
       if (action == "save")
       {
         FilterNotifier.removeListener(listener);
-        callback();
+        resolve();
       }
     };
     FilterNotifier.addListener(listener);
 
     FilterStorage.saveToDisk(file);
+  });
+}
+
+function testReadWrite(test, withExternal)
+{
+  let tempFile = IO.resolveFilePath("temp_patterns1.ini");
+  let tempFile2 = IO.resolveFilePath("temp_patterns2.ini");
+
+  function canonize(data)
+  {
+    let curSection = null;
+    let sections = [];
+    for (let line of (data + "\n[end]").split(/[\r\n]+/))
+    {
+      if (/^\[.*\]$/.test(line))
+      {
+        if (curSection)
+          sections.push(curSection);
+
+        curSection = {header: line, data: []};
+      }
+      else if (curSection && /\S/.test(line))
+        curSection.data.push(line);
+    }
+    for (let section of sections)
+    {
+      section.key = section.header + " " + section.data[0];
+      section.data.sort();
+    }
+    sections.sort(function(a, b)
+    {
+      if (a.key < b.key)
+        return -1;
+      else if (a.key > b.key)
+        return 1;
+      else
+        return 0;
+    });
+    return sections.map(function(section) {
+      return [section.header].concat(section.data).join("\n");
+    }).join("\n");
   }
 
-  function testReadWrite(withExternal)
+  return testData.then(data =>
   {
-    let tempFile = FileUtils.getFile("TmpD", ["temp_patterns1.ini"]);
-    let tempFile2 = FileUtils.getFile("TmpD", ["temp_patterns2.ini"]);
-    tempFile.createUnique(tempFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
-    createTempFile();
-
-    function canonize(data)
-    {
-      let curSection = null;
-      let sections = [];
-      for (let line of (data + "\n[end]").split(/[\r\n]+/))
-      {
-        if (/^\[.*\]$/.test(line))
-        {
-          if (curSection)
-            sections.push(curSection);
+    tempFile.contents = data;
+    return loadFilters(tempFile);
+  }).then(() =>
+  {
+    test.equal(FilterStorage.fileProperties.version, FilterStorage.formatVersion, "File format version");
 
-          curSection = {header: line, data: []};
-        }
-        else if (curSection && /\S/.test(line))
-          curSection.data.push(line);
-      }
-      for (let section of sections)
-      {
-        section.key = section.header + " " + section.data[0];
-        section.data.sort();
-      }
-      sections.sort(function(a, b)
-      {
-        if (a.key < b.key)
-          return -1;
-        else if (a.key > b.key)
-          return 1;
-        else
-          return 0;
-      });
-      return sections.map(function(section) {
-        return [section.header].concat(section.data).join("\n");
-      }).join("\n");
-    }
-
-    function createTempFile()
+    if (withExternal)
     {
-      let request = new XMLHttpRequest();
-      request.open("GET", "data/patterns.ini");
-      request.overrideMimeType("text/plain; charset=utf-8");
-      request.addEventListener("load", function()
-      {
-        writeToFile(tempFile, request.responseText);
-        loadFilters(tempFile, saveFile);
-      }, false);
-      request.send(null);
-    }
-
-    function saveFile()
-    {
-      equal(FilterStorage.fileProperties.version, FilterStorage.formatVersion, "File format version");
+      let subscription = new ExternalSubscription("~external~external subscription ID", "External subscription");
+      subscription.filters = [Filter.fromText("foo"), Filter.fromText("bar")];
+      FilterStorage.addSubscription(subscription);

[Wladimir Palant, 2016/10/05 20:10:22]

This was calling our public API in the original test, hence testing below that
the subscription was definitely added correctly. Now the subscription is being
added explicitly (public API isn't part of adblockpluscore) but I thought that
leaving the checks won't hurt - the point of this code is making sure that
external subscriptions aren't being saved, so we better be sure that we have an
external subscription in the data.

[kzar, 2016/10/06 05:13:38]

Acknowledged.

 
-      if (withExternal)
-      {
-        let {AdblockPlus} = Cu.import(Cc["@adblockplus.org/abp/public;1"].getService(Ci.nsIURI).spec, null);
-        AdblockPlus.updateExternalSubscription("~external~external subscription ID", "External subscription", ["foo", "bar"]);
-
-        let externalSubscriptions = FilterStorage.subscriptions.filter(function (subscription) subscription instanceof ExternalSubscription);
-        equal(externalSubscriptions.length, 1, "Number of external subscriptions after updateExternalSubscription");
+      let externalSubscriptions = FilterStorage.subscriptions.filter(subscription => subscription instanceof ExternalSubscription);
+      test.equal(externalSubscriptions.length, 1, "Number of external subscriptions after updateExternalSubscription");
 
-        if (externalSubscriptions.length == 1)
-        {
-          equal(externalSubscriptions[0].url, "~external~external subscription ID", "ID of external subscription");
-          equal(externalSubscriptions[0].filters.length, 2, "Number of filters in external subscription");
-        }
-      }
-
-      saveFilters(tempFile2, compareFile);
+      test.equal(externalSubscriptions[0].url, "~external~external subscription ID", "ID of external subscription");
+      test.equal(externalSubscriptions[0].filters.length, 2, "Number of filters in external subscription");
     }
 
-    function compareFile()
-    {
-      let stream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
-      stream.init(tempFile2, FileUtils.MODE_RDONLY, FileUtils.PERMS_FILE, Ci.nsIFileInputStream.DEFER_OPEN);
-
-      NetUtil.asyncFetch(stream, function(inputStream, nsresult)
-      {
-        let result = NetUtil.readInputStreamToString(inputStream, inputStream.available(), {charset: "utf-8"});
-
-        let request = new XMLHttpRequest();
-        request.open("GET", "data/patterns.ini");
-        request.overrideMimeType("text/plain");
-        request.addEventListener("load", function()
-        {
-          let expected = request.responseText;
-          equal(canonize(result), canonize(expected), "Read/write result");
-
-          tempFile.remove(false);
-          tempFile2.remove(false);
-          start();
-        }, false);
-        request.send(null);
-      });
-    }
-  }
-
-  asyncTest("Read and save to file", testReadWrite.bind(false));
-  asyncTest("Read, add external subscription and save to file", testReadWrite.bind(true));
-
-  let groupTests = [
-    ["~wl~", "whitelist"],
-    ["~fl~", "blocking"],
-    ["~eh~", "elemhide"]
-  ];
-  for (let i = 0; i < groupTests.length; i++)
+    return saveFilters(tempFile2);
+  }).then(() => testData).then(expected =>
   {
-    let [url, defaults] = groupTests[i];
-    asyncTest("Read empty legacy user-defined group (" + url + ")", function()
-    {
-      let data = "[Subscription]\nurl=" + url;
-      let tempFile = FileUtils.getFile("TmpD", ["temp_patterns1.ini"]);
-      writeToFile(tempFile, data);
-
-      loadFilters(tempFile, function()
-      {
-        tempFile.remove(false);
-        equal(FilterStorage.subscriptions.length, 0, "Number of filter subscriptions");
-        start();
-      });
-    });
-    asyncTest("Read non-empty legacy user-defined group (" + url + ")", function()
-    {
-      let data = "[Subscription]\nurl=" + url + "\n[Subscription filters]\nfoo";
-      let tempFile = FileUtils.getFile("TmpD", ["temp_patterns1.ini"]);
-      writeToFile(tempFile, data);
+    test.equal(canonize(tempFile2.contents), canonize(expected), "Read/write result");
+  }).catch(unexpectedError.bind(test)).then(() => test.done());
+}
 
-      loadFilters(tempFile, function()
-      {
-        tempFile.remove(false);
-        equal(FilterStorage.subscriptions.length, 1, "Number of filter subscriptions");
-        if (FilterStorage.subscriptions.length == 1)
-        {
-          let subscription = FilterStorage.subscriptions[0];
-          equal(subscription.url, url, "Subscription ID");
-          equal(subscription.title, Utils.getString(defaults + "Group_title"), "Subscription title");
-          deepEqual(subscription.defaults, [defaults], "Default types");
-          equal(subscription.filters.length, 1, "Number of subscription filters");
-          if (subscription.filters.length == 1)
-            equal(subscription.filters[0].text, "foo", "First filter");
-        }
-        start();
-      });
-    });
-  }
+exports.testReadAndSaveToFile = function(test)
+{
+  testReadWrite(test, false);
+};
 
-  asyncTest("Read legacy user-defined filters", function()
+exports.testReadAndSaveToFileWithExternalSubscription = function(test)
+{
+  testReadWrite(test, true);
+};
+
+exports.testLegacyGroups = {};
+
+for (let url of ["~wl~", "~fl~", "~eh~"])
+{
+  exports.testLegacyGroups["read empty " + url] = function(test)
   {
-    let data = "[Subscription]\nurl=~user~1234\ntitle=Foo\n[Subscription filters]\n[User patterns]\nfoo\n\\[bar]\nfoo#bar";
-    let tempFile = FileUtils.getFile("TmpD", ["temp_patterns1.ini"]);
-    writeToFile(tempFile, data);
+    let data = "[Subscription]\nurl=" + url;
+    let tempFile = IO.resolveFilePath("temp_patterns1.ini");
+    tempFile.contents = data;
 
     loadFilters(tempFile, function()
     {
-      tempFile.remove(false);
-      equal(FilterStorage.subscriptions.length, 1, "Number of filter subscriptions");
+      test.equal(FilterStorage.subscriptions.length, 0, "Number of filter subscriptions");
+    }).catch(unexpectedError.bind(test)).then(() => test.done());
+  };
+
+  exports.testLegacyGroups["read non-empty " + url] = function(test)
+  {
+    let data = "[Subscription]\nurl=" + url + "\n[Subscription filters]\nfoo";
+    let tempFile = IO.resolveFilePath("temp_patterns1.ini");
+    tempFile.contents = data;
+
+    loadFilters(tempFile).then(() =>
+    {
+      test.equal(FilterStorage.subscriptions.length, 1, "Number of filter subscriptions");
       if (FilterStorage.subscriptions.length == 1)
       {
         let subscription = FilterStorage.subscriptions[0];
-        equal(subscription.filters.length, 3, "Number of subscription filters");
-        if (subscription.filters.length == 3)
-        {
-          equal(subscription.filters[0].text, "foo", "First filter");
-          equal(subscription.filters[1].text, "[bar]", "Second filter");
-          equal(subscription.filters[2].text, "foo#bar", "Third filter");
-        }
+        test.equal(subscription.url, url, "Subscription ID");

[kzar, 2016/10/06 05:13:37]

Won't `url` always be "~eh~" by the time the test is run?

[Wladimir Palant, 2016/10/06 07:13:59]

Good question. I checked and this isn't the case - we actually run this test
with all three variants for `url`. However, changing `let url` into `var url`
will produce the behavior you expected.

[kzar, 2016/10/06 08:06:47]

Ah of course.

+        test.equal(subscription.title, null, "Subscription title");
+        test.deepEqual(subscription.defaults, null, "Default types");

[Wladimir Palant, 2016/10/05 20:10:22]

I had to modify this, the fallback for legacy groups is gone with
https://issues.adblockplus.org/ticket/3660 but this test wasn't adapted for some
reason. Default title is no more, and there are no implied defaults either.

[kzar, 2016/10/06 05:13:37]

Acknowledged.

+        test.equal(subscription.filters.length, 1, "Number of subscription filters");
+        if (subscription.filters.length == 1)
+          test.equal(subscription.filters[0].text, "foo", "First filter");
       }
-      start();
-    });
-  });
-
-  asyncTest("Saving without backups", function()
-  {
-    Prefs.patternsbackups = 0;
-    Prefs.patternsbackupinterval = 24;
-
-    let tempFile = FileUtils.getFile("TmpD", ["temp_patterns.ini"]);
-    tempFile.createUnique(tempFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
-    FilterStorage.__defineGetter__("sourceFile", () => tempFile.clone());
+    }).catch(unexpectedError.bind(test)).then(() => test.done());
+  };
+}
 
-    saveFilters(null, function()
+exports.testReadLegacyFilters = function(test)
+{
+  let data = "[Subscription]\nurl=~user~1234\ntitle=Foo\n[Subscription filters]\n[User patterns]\nfoo\n\\[bar]\nfoo#bar";
+  let tempFile = IO.resolveFilePath("temp_patterns1.ini");
+  tempFile.contents = data;
+
+  loadFilters(tempFile).then(() =>
+  {
+    test.equal(FilterStorage.subscriptions.length, 1, "Number of filter subscriptions");
+    if (FilterStorage.subscriptions.length == 1)
     {
-      saveFilters(null, function()
+      let subscription = FilterStorage.subscriptions[0];
+      test.equal(subscription.filters.length, 3, "Number of subscription filters");
+      if (subscription.filters.length == 3)
       {
-        let backupFile = tempFile.clone();
-        backupFile.leafName = backupFile.leafName.replace(/\.ini$/, "-backup1.ini");
-        ok(!backupFile.exists(), "Backup shouldn't be created");
-        start();
-      });
-    });
-  });
+        test.equal(subscription.filters[0].text, "foo", "First filter");
+        test.equal(subscription.filters[1].text, "[bar]", "Second filter");
+        test.equal(subscription.filters[2].text, "foo#bar", "Third filter");
+      }
+    }
+  }).catch(unexpectedError.bind(test)).then(() => test.done());
+};
 
-  asyncTest("Saving with backups", function()
+exports.testSavingWithoutBackups = function(test)
+{
+  Prefs.patternsbackups = 0;
+  Prefs.patternsbackupinterval = 24;
+
+  let tempFile = IO.resolveFilePath("temp_patterns.ini");
+  Object.defineProperty(FilterStorage, "sourceFile", {get: () => tempFile.clone()});
+
+  saveFilters(null).then(() =>
   {
-    Prefs.patternsbackups = 2;
-    Prefs.patternsbackupinterval = 24;
-
-    let tempFile = FileUtils.getFile("TmpD", ["temp_patterns.ini"]);
-    tempFile.createUnique(tempFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
-    FilterStorage.__defineGetter__("sourceFile", () => tempFile.clone());
-
+    return saveFilters(null);
+  }).then(() =>
+  {
     let backupFile = tempFile.clone();
     backupFile.leafName = backupFile.leafName.replace(/\.ini$/, "-backup1.ini");
-
-    let backupFile2 = tempFile.clone();
-    backupFile2.leafName = backupFile2.leafName.replace(/\.ini$/, "-backup2.ini");
-
-    let backupFile3 = tempFile.clone();
-    backupFile3.leafName = backupFile3.leafName.replace(/\.ini$/, "-backup3.ini");
-
-    let oldModifiedTime;
-
-    saveFilters(null, callback1);
-
-    function callback1()
-    {
-      // Save again immediately
-      saveFilters(null, callback2);
-    }
-
-    function callback2()
-    {
-      backupFile = backupFile.clone();  // File parameters are cached, clone to prevent this
-      ok(backupFile.exists(), "First backup created");
-
-      backupFile.lastModifiedTime -= 10000;
-      oldModifiedTime = backupFile.lastModifiedTime;
-      saveFilters(null, callback3);
-    }
-
-    function callback3()
-    {
-      backupFile = backupFile.clone();  // File parameters are cached, clone to prevent this
-      equal(backupFile.lastModifiedTime, oldModifiedTime, "Backup not overwritten if it is only 10 seconds old");
-
-      backupFile.lastModifiedTime -= 40*60*60*1000;
-      oldModifiedTime = backupFile.lastModifiedTime;
-      saveFilters(null, callback4);
-    }
-
-    function callback4()
-    {
-      backupFile = backupFile.clone();  // File parameters are cached, clone to prevent this
-      notEqual(backupFile.lastModifiedTime, oldModifiedTime, "Backup overwritten if it is 40 hours old");
+    test.ok(!backupFile.exists(), "Backup shouldn't be created");
+  }).catch(unexpectedError.bind(test)).then(() => test.done());
+};
 
-      backupFile2 = backupFile2.clone();  // File parameters are cached, clone to prevent this
-      ok(backupFile2.exists(), "Second backup created when first backup is overwritten");
-
-      backupFile.lastModifiedTime -= 20000;
-      oldModifiedTime = backupFile2.lastModifiedTime;
-      saveFilters(null, callback5);
-    }
-
-    function callback5()
-    {
-      backupFile2 = backupFile2.clone();  // File parameters are cached, clone to prevent this
-      equal(backupFile2.lastModifiedTime, oldModifiedTime, "Second backup not overwritten if first one is only 20 seconds old");
-
-      backupFile.lastModifiedTime -= 25*60*60*1000;
-      oldModifiedTime = backupFile2.lastModifiedTime;
-      saveFilters(null, callback6);
-    }
-
-    function callback6()
-    {
-      backupFile2 = backupFile2.clone();  // File parameters are cached, clone to prevent this
-      notEqual(backupFile2.lastModifiedTime, oldModifiedTime, "Second backup overwritten if first one is 25 hours old");
+exports.testSavingWithBackups = function(test)
+{
+  Prefs.patternsbackups = 2;
+  Prefs.patternsbackupinterval = 24;
 
-      ok(!backupFile3.exists(), "Third backup not created with patternsbackups = 2");
+  let tempFile = IO.resolveFilePath("temp_patterns.ini");
+  Object.defineProperty(FilterStorage, "sourceFile", {get: () => tempFile.clone()});
 
-      try
-      {
-        tempFile.remove(false);
-      } catch (e) {}
-      try
-      {
-        backupFile.remove(false);
-      } catch (e) {}
-      try
-      {
-        backupFile2.remove(false);
-      } catch (e) {}
-      try
-      {
-        backupFile3.remove(false);
-      } catch (e) {}
+  let backupFile = tempFile.clone();
+  backupFile.leafName = backupFile.leafName.replace(/\.ini$/, "-backup1.ini");
 
-      start();
-    }
-  });
-})();
+  let backupFile2 = tempFile.clone();
+  backupFile2.leafName = backupFile2.leafName.replace(/\.ini$/, "-backup2.ini");
+
+  let backupFile3 = tempFile.clone();
+  backupFile3.leafName = backupFile3.leafName.replace(/\.ini$/, "-backup3.ini");
+
+  let oldModifiedTime;
+
+  saveFilters(null).then(() =>
+  {
+    // Save again immediately
+    return saveFilters(null);
+  }).then(() =>
+  {
+    test.ok(backupFile.exists(), "First backup created");
+
+    backupFile.lastModifiedTime -= 10000;
+    oldModifiedTime = backupFile.lastModifiedTime;
+    return saveFilters(null);
+  }).then(() =>
+  {
+    test.equal(backupFile.lastModifiedTime, oldModifiedTime, "Backup not overwritten if it is only 10 seconds old");
+
+    backupFile.lastModifiedTime -= 40*60*60*1000;
+    oldModifiedTime = backupFile.lastModifiedTime;
+    return saveFilters(null);
+  }).then(() =>
+  {
+    test.notEqual(backupFile.lastModifiedTime, oldModifiedTime, "Backup overwritten if it is 40 hours old");
+
+    test.ok(backupFile2.exists(), "Second backup created when first backup is overwritten");
+
+    backupFile.lastModifiedTime -= 20000;
+    oldModifiedTime = backupFile2.lastModifiedTime;
+    return saveFilters(null);
+  }).then(() =>
+  {
+    test.equal(backupFile2.lastModifiedTime, oldModifiedTime, "Second backup not overwritten if first one is only 20 seconds old");
+
+    backupFile.lastModifiedTime -= 25*60*60*1000;
+    oldModifiedTime = backupFile2.lastModifiedTime;
+    return saveFilters(null);
+  }).then(() =>
+  {
+    test.notEqual(backupFile2.lastModifiedTime, oldModifiedTime, "Second backup overwritten if first one is 25 hours old");
+
+    test.ok(!backupFile3.exists(), "Third backup not created with patternsbackups = 2");
+  }).catch(unexpectedError.bind(test)).then(() => test.done());
+};