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

test/filterStorage_readwrite.js

 /*
  * 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
@@ skipping 29 matching lines @@
   );
 
   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);
   });
 });
 
 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);
-        resolve();
-      }
-    };
-    FilterNotifier.addListener(listener);
-
-    FilterStorage.loadFromDisk(file);
-  });
-}
-
-function saveFilters(file, callback)
-{
-  return new Promise((resolve, reject) =>
-  {
-    let listener = function(action)
-    {
-      if (action == "save")
-      {
-        FilterNotifier.removeListener(listener);
-        resolve();
-      }
-    };
-    FilterNotifier.addListener(listener);
-
-    FilterStorage.saveToDisk(file);
-  });
+  FilterStorage.loadFromDisk(file);
+  return FilterNotifier.once("load");
+}
+
+function saveFilters(file)
+{
+  FilterStorage.saveToDisk(file);
+  return FilterNotifier.once("save");
 }
 
 function testReadWrite(test, withExternal)
 {
   let tempFile = IO.resolveFilePath("temp_patterns1.ini");
   let tempFile2 = IO.resolveFilePath("temp_patterns2.ini");
 
   function canonize(data)
   {
     let curSection = null;
@@ skipping 34 matching lines @@
     tempFile.contents = data;
     return loadFilters(tempFile);
   }).then(() =>
   {
     test.equal(FilterStorage.fileProperties.version, FilterStorage.formatVersion, "File format version");
 
     if (withExternal)
     {
       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.

 
       let externalSubscriptions = FilterStorage.subscriptions.filter(subscription => subscription instanceof ExternalSubscription);
       test.equal(externalSubscriptions.length, 1, "Number of external subscriptions after updateExternalSubscription");
 
       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");
     }
 
     return saveFilters(tempFile2);
   }).then(() => testData).then(expected =>
@@ skipping 33 matching lines @@
     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];
         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");
       }
     }).catch(unexpectedError.bind(test)).then(() => test.done());
   };
 }
 
 exports.testReadLegacyFilters = function(test)
 {
@@ skipping 90 matching lines @@
     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());
 };