Issue 4223 - Migrate some more of adblockplustests

test_runner.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
  * 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/>.
  */
 
 "use strict";

[Wladimir Palant, 2016/10/04 11:02:58]

Gotta love Git, it considers your new test_wrapper.js a copy of lib/coreUtils.js
("similarity index 50%"). I suggest using -M100 -C100 when exporting your patch.

[kzar, 2016/10/04 12:06:24]

Acknowledged.

 
 let fs = require("fs");
 let path = require("path");
 let process = require("process");
 let nodeunit = require("nodeunit");
 
 let files = [];
-function addTestPaths(testPaths, depth)

[Wladimir Palant, 2016/10/04 11:02:57]

I wonder whether depth really needs to be a number. How about having a boolean
called recursive that will always be set to false on the recursive call? This
will also make sure that we only recurse once but be more obvious.

[kzar, 2016/10/04 12:06:23]

Done.

+function addTestPaths(testPaths, recurse)
 {
-  if (depth < 1)
-    return;
-
   for (let testPath of testPaths)
   {
     let stat = fs.statSync(testPath);
     if (stat.isDirectory())
-      addTestPaths(fs.readdirSync(testPath).map(file => path.join(testPath, file)),

[Wladimir Palant, 2016/10/04 11:02:58]

1) This is still longer than 80 characters. Maybe get directory entries in a
separate statement, with a temporary variable?

2) Please always put brackets around a block if it spans multiple lines - even
if it is a single statement.

[kzar, 2016/10/04 12:06:23]

Done.

-                   depth -1);
-    else if (path.extname(testPath) == ".js" && path.basename(testPath) != "common.js")

[Wladimir Palant, 2016/10/04 11:02:58]

How about something more generic than ignoring common.js explicitly? Maybe
rename it into _common.js and ignore anything starting with an underscore?

[kzar, 2016/10/04 12:06:23]

Done.

+    {
+      if (recurse)
+      {
+        addTestPaths(fs.readdirSync(testPath).map(
+          file => path.join(testPath, file)));
+      }
+    }
+    else if (path.extname(testPath) == ".js" &&
+             !path.basename(testPath).startsWith("_"))
+    {
       files.push(testPath);
+    }
   }
 }
-addTestPaths(process.argv.length > 2 ? process.argv.slice(2) : [path.join(__dirname, "test")], 2);

[Wladimir Palant, 2016/10/04 11:02:59]

No point having this overly long line. Please split it to multiple lines, this
will also make it easier to read:

  if (process.argv.length > 2)
    addTestPaths(process.argv.slice(2), 2);
  else
    addTestPaths([path.join(__dirname, "test")], 2);

[kzar, 2016/10/04 12:06:24]

Done.

+if (process.argv.length > 2)
+  addTestPaths(process.argv.slice(2), true);
+else
+  addTestPaths([path.join(__dirname, "test")], true);
 
 nodeunit.reporters.default.run(files);