Issue 491 - Validate custom filters

qunit/tests/filterValidation.js

Index: qunit/tests/filterValidation.js
===================================================================
new file mode 100644
--- /dev/null
+++ b/qunit/tests/filterValidation.js
@@ -0,0 +1,80 @@
+module(
+  "filter validation",
+  {
+    setup: function()
+    {
+      var filterValidation = require("filterValidation");
+      this.parseFilter = filterValidation.parseFilter;
+      this.parseFilters = filterValidation.parseFilters;
+
+      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.

+    }
+  }
+);
+
+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;
+
+  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 = this.parseFilter(text);
+  ok(filter instanceof this.ElemHideFilter && filter.text == text, "elemhide filter");
+
+  text = "! foo bar"
+  filter = this.parseFilter(text);
+  ok(filter instanceof this.CommentFilter && filter.text == text, "comment filter");
+
+  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("parsing multiple filters", function()
+{
+  var filters;
+
+  filters = this.parseFilters("||example.com\n \n###foobar\r\n! foo bar\n");
+  ok(
+    filters.length == 3 &&
+
+    filters[0] instanceof this.BlockingFilter &&
+    filters[0].text == "||example.com" &&
+
+    filters[1] instanceof this.ElemHideFilter &&
+    filters[1].text == "###foobar" &&
+
+    filters[2] instanceof this.CommentFilter &&
+    filters[2].text == "! foo bar",
+
+    "parse filters and comments, stripping empty lines"
+  );
+
+  filters = this.parseFilters("[foobar]\n \n||example.com\r\n! foo bar\n", true);
+  ok(
+    filters.length == 2 &&
+
+    filters[0] instanceof this.BlockingFilter &&
+    filters[0].text == "||example.com" &&
+
+    filters[1] instanceof this.CommentFilter &&
+    filters[1].text == "! foo bar",
+
+    "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"
+  );
+});