From e4f011df760b2eb6cd1142a14a4dbd17c4eed2b6 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sat, 8 May 2021 19:28:08 -0400
Subject: [PATCH] tests: Use `require()` to load frontend test specs

This makes core and plugin tests consistent with each other, makes it
possible to `require()` relative paths in spec files, simplifies the
code somewhat, and should make it easier to move away from
require-kernel.

Also:
  * Wrap plugin tests inside a `describe()` that contains the plugin
    name to make it easier to grep for a plugin's tests and for
    consistency with core tests.
  * Add "<core>" to the core test descriptions to make it easier to
    distinguish them from plugin tests.
---
 src/node/hooks/express/tests.js | 60 ++++++++++++++-------------------
 src/node/utils/Minify.js        |  2 ++
 src/tests/frontend/runner.js    | 14 ++++++--
 3 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/src/node/hooks/express/tests.js b/src/node/hooks/express/tests.js
index 26fcaaaac..c0f2fcdb6 100644
--- a/src/node/hooks/express/tests.js
+++ b/src/node/hooks/express/tests.js
@@ -31,19 +31,29 @@ const findSpecs = async (specDir) => {
 
 exports.expressCreateServer = (hookName, args, cb) => {
   args.app.get('/tests/frontend/frontendTestSpecs.js', async (req, res) => {
-    const [coreTests, pluginTests] = await Promise.all([getCoreTests(), getPluginTests()]);
-
-    // merge the two sets of results
-    let files = [].concat(coreTests, pluginTests).sort();
-
-    // remove admin tests if the setting to enable them isn't in settings.json
-    if (!settings.enableAdminUITests) {
-      files = files.filter((file) => file.indexOf('admin') !== 0);
-    }
-
-    console.debug('Sent browser the following test specs:', files);
+    const modules = [];
+    await Promise.all(Object.entries(plugins.plugins).map(async ([plugin, def]) => {
+      let {package: {path: pluginPath}} = def;
+      if (!pluginPath.endsWith(path.sep)) pluginPath += path.sep;
+      const specDir = `${plugin === 'ep_etherpad-lite' ? '' : 'static/'}tests/frontend/specs`;
+      for (const spec of await findSpecs(path.join(pluginPath, specDir))) {
+        if (plugin === 'ep_etherpad-lite' && !settings.enableAdminUITests &&
+            spec.startsWith('admin')) continue;
+        modules.push(`${plugin}/${specDir}/${spec.replace(/\.js$/, '')}`);
+      }
+    }));
+    // Sort plugin tests before core tests.
+    modules.sort((a, b) => {
+      a = String(a);
+      b = String(b);
+      const aCore = a.startsWith('ep_etherpad-lite/');
+      const bCore = b.startsWith('ep_etherpad-lite/');
+      if (aCore === bCore) return a.localeCompare(b);
+      return aCore ? 1 : -1;
+    });
+    console.debug('Sent browser the following test spec modules:', modules);
     res.setHeader('content-type', 'application/javascript');
-    res.end(`window.frontendTestSpecs = ${JSON.stringify(files, null, 2)};\n`);
+    res.end(`window.frontendTestSpecs = ${JSON.stringify(modules, null, 2)};\n`);
   });
 
   const rootTestFolder = path.join(settings.root, 'src/tests/frontend/');
@@ -57,16 +67,9 @@ exports.expressCreateServer = (hookName, args, cb) => {
   // version used with Express v4.x) interprets '.' and '*' differently than regexp.
   args.app.get('/tests/frontend/:file([\\d\\D]{0,})', (req, res, next) => {
     (async () => {
-      let relFile = sanitizePathname(req.params.file);
-      if (['', '.', './'].includes(relFile)) relFile = 'index.html';
-      const file = path.join(rootTestFolder, relFile);
-      if (relFile.startsWith('specs/') && file.endsWith('.js')) {
-        const content = await fsp.readFile(file);
-        res.setHeader('content-type', 'application/javascript');
-        res.send(`describe(${JSON.stringify(path.basename(file))}, function () {\n${content}\n});`);
-      } else {
-        res.sendFile(file);
-      }
+      let file = sanitizePathname(req.params.file);
+      if (['', '.', './'].includes(file)) file = 'index.html';
+      res.sendFile(path.join(rootTestFolder, file));
     })().catch((err) => next(err || new Error(err)));
   });
 
@@ -76,16 +79,3 @@ exports.expressCreateServer = (hookName, args, cb) => {
 
   return cb();
 };
-
-const getPluginTests = async (callback) => {
-  const specPath = 'static/tests/frontend/specs';
-  const specLists = await Promise.all(Object.entries(plugins.plugins).map(async ([plugin, def]) => {
-    if (plugin === 'ep_etherpad-lite') return [];
-    const {package: {path: pluginPath}} = def;
-    const specs = await findSpecs(path.join(pluginPath, specPath));
-    return specs.map((spec) => `/static/plugins/${plugin}/${specPath}/${spec}`);
-  }));
-  return [].concat(...specLists);
-};
-
-const getCoreTests = async () => await findSpecs('src/tests/frontend/specs');
diff --git a/src/node/utils/Minify.js b/src/node/utils/Minify.js
index 02728824d..ed107af7e 100644
--- a/src/node/utils/Minify.js
+++ b/src/node/utils/Minify.js
@@ -165,6 +165,8 @@ const minify = async (req, res) => {
       filename = path.join('../node_modules/', library, libraryPath);
     }
   }
+  const [, spec] = /^plugins\/ep_etherpad-lite\/(tests\/frontend\/specs\/.*)/.exec(filename) || [];
+  if (spec != null) filename = `../${spec}`;
 
   const contentType = mime.lookup(filename);
 
diff --git a/src/tests/frontend/runner.js b/src/tests/frontend/runner.js
index d8c542107..ebb956a44 100644
--- a/src/tests/frontend/runner.js
+++ b/src/tests/frontend/runner.js
@@ -141,9 +141,17 @@ $(() => {
   require.setLibraryURI(absUrl('../../javascripts/lib'));
   require.setGlobalKeyPath('require');
 
-  const $body = $('body');
-  for (const spec of window.frontendTestSpecs.map((spec) => encodeURI(spec))) {
-    $body.append($('<script>').attr('src', spec.startsWith('/') ? spec : `specs/${spec}`));
+  // This loads the test specs serially. While it is technically possible to load them in parallel,
+  // the code would be very complex (it involves wrapping require.define(), configuring
+  // require-kernel to use the wrapped .define() via require.setGlobalKeyPath(), and using the
+  // asynchronous form of require()). In addition, the performance gains would be minimal because
+  // require-kernel only loads 2 at a time by default. (Increasing the default could cause problems
+  // because browsers like to limit the number of concurrent fetches.)
+  for (const spec of window.frontendTestSpecs) {
+    const desc = spec
+        .replace(/^ep_etherpad-lite\/tests\/frontend\/specs\//, '<core> ')
+        .replace(/^([^/ ]*)\/static\/tests\/frontend\/specs\//, '<$1> ');
+    describe(`${desc}.js`, function () { require(spec); });
   }
 
   // initialize the test helper