Issue 491 - Validate custom filters

qunit/tests/filterValidation.js

-module(
-  "filter validation",
+(function()
+{
+  var filterValidation = require("filterValidation");
+  var parseFilter = filterValidation.parseFilter;
+  var parseFilters = filterValidation.parseFilters;
+
+  var filterClasses = require("filterClasses");
+  var BlockingFilter = filterClasses.BlockingFilter;
+  var ElemHideFilter = filterClasses.ElemHideFilter;
+  var CommentFilter = filterClasses.CommentFilter;
+
+  module("Filter validation");
+
+  test("Detecting invalid filters", function()
   {
-    setup: function()
-    {
-      var filterValidation = require("filterValidation");
-      this.parseFilter = filterValidation.parseFilter;
-      this.parseFilters = filterValidation.parseFilters;
+    throws(parseFilter.bind(null, "||example.com^$unknown"), "unknown option");
+    throws(parseFilter.bind(null, "[foobar]"), "filter list header");
+    throws(parseFilter.bind(null, "##[foo"), "invalid selector");
 
-      var filterClasses = require("filterClasses");
-      this.BlockingFilter = filterClasses.BlockingFilter;
-      this.ElemHideFilter = filterClasses.ElemHideFilter;
-      this.CommentFilter = filterClasses.CommentFilter;

[Wladimir Palant, 2014/11/20 14:10:06]

It makes relatively little sense to require modules for each test run
individually - these won't change from one test to another and don't need to be
cleaned up. You can do that at the top of the file instead.

[Sebastian Noack, 2014/11/20 15:36:29]

Done, but I used an IFEE, to don't pollute the global context.

[Wladimir Palant, 2014/11/20 16:58:20]

Right, they don't run in their own context... Actually, the way this is solved
for Firefox is a separate `common.js` file that does all the requires.

-    }
-  }
-);
+    throws(parseFilters.bind(null, "!comment\r\n||example.com^\n\n##/"), /\b4\b/, "error contains corresponding line number");
+  });
 
-test("detecting invalid filters", function() {

[Wladimir Palant, 2014/11/20 14:10:06]

Nit: Here and below, test names are usually capitalized - "Detecting invalid
filters"

Nit: Opening bracket on next line please (same for the next test).

[Sebastian Noack, 2014/11/20 15:36:29]

Done.

-  throws(this.parseFilter.bind(null, "||example.com$unknown"), "unkown option");

[Wladimir Palant, 2014/11/20 14:10:06]

Typo: "unkown"

[Sebastian Noack, 2014/11/20 15:36:29]

Done.

-  throws(this.parseFilter.bind(null, "[foobar]"), "filter list header");
-  throws(this.parseFilter.bind(null, "##[foo"), "invalid selector");
-});
+  test("Allowing valid filters", function()
+  {
+    var text, filter;
 
-test("allowing valid filters", function() {
-  var text, filter;
+    text = "||example.com^";
+    filter = parseFilter(text);
+    ok(filter instanceof BlockingFilter, "blocking filter parsed");
+    equal(filter.text, text, "blocking filter text matches");
 
-  text = "||example.com";

[Wladimir Palant, 2014/11/20 14:10:06]

Nit: Better "||example.com^" - a separator placeholder should normally be used
at the end of a domain name.

[Sebastian Noack, 2014/11/20 15:36:29]

Done.

-  filter = this.parseFilter(text);
-  ok(filter instanceof this.BlockingFilter && filter.text == text, "blocking filter");

[Wladimir Palant, 2014/11/20 14:10:06]

Here and everywhere else, please use a separate check for each requirement:

ok(filter instanceof this.BlockingFilter, "Blocking filter");
equal(filter.text, text, "Filter text");

This has two advantages:

* equal() will produce more useful output in case of failure - it will show you
the expected and actual value.
* You can immediately see which check failed.

[Sebastian Noack, 2014/11/20 15:36:29]

Done.

+    text = '##div:first-child a[src="http://example.com"] > .foo + #bar'
+    filter = parseFilter(text);
+    ok(filter instanceof ElemHideFilter, "elemhide filter parsed");
+    equal(filter.text, text, "elemhide filter text matches");
 
-  text = '##div:first-child a[src="http://example.com"] > .foo + #bar'
-  filter = this.parseFilter(text);
-  ok(filter instanceof this.ElemHideFilter && filter.text == text, "elemhide filter");
+    text = "! foo bar"
+    filter = parseFilter(text);
+    ok(filter instanceof CommentFilter, "comment filter parsed");
+    equal(filter.text, text, "comment filter text matches");
 
-  text = "! foo bar"
-  filter = this.parseFilter(text);
-  ok(filter instanceof this.CommentFilter && filter.text == text, "comment filter");
+    equal(parseFilter(""), null, "empty filter parsed as 'null'");
+  });
 
-  equal(this.parseFilter(""), null, "empty filter");

[Wladimir Palant, 2014/11/20 14:10:06]

Might be a good idea to test that filter normalization is applied, e.g. leading
and trailing spaces are being removed for all filter types. Note: testing filter
normalization itself here isn't necessary, that's core functionality and should
be tested there.

[Sebastian Noack, 2014/11/20 15:36:29]

Done.

-});
+  test("Normalizing filters", function()
+  {
+    var ws = " \t\r\n";
 
-test("parsing multiple filters", function()
-{
-  var filters;
+    equal(parseFilter(ws + "@@" + ws + "||" + ws + "example.com" + ws + "^" + ws).text, "@@||example.com^", "unnecessary spaces");
+    equal(parseFilter(ws), null, "only spaces");
+  });
 
-  filters = this.parseFilters("||example.com\n \n###foobar\r\n! foo bar\n");
-  ok(
-    filters.length == 3 &&
+  test("Parsing multiple filters", function()
+  {
+    var filters = parseFilters("||example.com^\n \n###foobar\r\n! foo bar\n");
 
-    filters[0] instanceof this.BlockingFilter &&
-    filters[0].text == "||example.com" &&
+    equal(filters.length, 3, "all filters parsed");
 
-    filters[1] instanceof this.ElemHideFilter &&
-    filters[1].text == "###foobar" &&
+    ok(filters[0] instanceof BlockingFilter, "1st filter is blocking");
+    equal(filters[0].text, "||example.com^", "1st filter text matches");
 
-    filters[2] instanceof this.CommentFilter &&
-    filters[2].text == "! foo bar",
+    ok(filters[1] instanceof ElemHideFilter, "2nd filter is elemhide");
+    equal(filters[1].text, "###foobar",      "2nd filter text matches");
 
-    "parse filters and comments, stripping empty lines"
-  );
+    ok(filters[2] instanceof CommentFilter,  "3rd filter is comment");
+    equal(filters[2].text, "! foo bar",      "3rd filter text matches");
+  });
 
-  filters = this.parseFilters("[foobar]\n \n||example.com\r\n! foo bar\n", true);
-  ok(
-    filters.length == 2 &&
+  test("Parsing multiple filters, stripping filter list headers", function()
+  {
+    var filters = parseFilters("[foobar]\n \n||example.com^\r\n! foo bar\n", true);
 
-    filters[0] instanceof this.BlockingFilter &&
-    filters[0].text == "||example.com" &&
+    equal(filters.length, 2, "all filters parsed");
 
-    filters[1] instanceof this.CommentFilter &&
-    filters[1].text == "! foo bar",
+    ok(filters[0] instanceof BlockingFilter, "1st filter is blocking");
+    equal(filters[0].text, "||example.com^", "1st filter text matches");
 
-    "parse filters and comments, stripping empty lines and filter list headers"
-  );
-
-  throws(
-    this.parseFilters.bind(null, "!comment\r\n||example.com\n\n##/"),
-    /^([^:]+ )?4( [^:]+)?:/,

[Wladimir Palant, 2014/11/20 14:10:06]

No point overcomplicating this, how about /\b4\b/? This has the advantage that
no assumptions about the string are made, merely the fact that the line number
is in there.

[Sebastian Noack, 2014/11/20 15:36:29]

My concern was that there can be digits occurring in the actual error message,
but fair enough.

-    "throw error with corresponding line number"
-  );
-});
+    ok(filters[1] instanceof CommentFilter,  "2nd filter is comment");
+    equal(filters[1].text, "! foo bar",      "2nd filter text matches");
+  });
+})();