From c7dce90f43ec15f93b95e719d3fafca50e40edb1 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Mon, 4 Sep 2023 17:09:55 +0100
Subject: [PATCH] DEV: Switch to using standard ember-cli test bundle (#23337)

Previously we were patching ember-cli so that it would split the test bundle into two halves: the helpers, and the tests themselves. This was done so that we could use the helpers for `/theme-qunit` without needing to load all the core tests. This patch has proven problematic to maintain, and will become even harder under Embroider.

This commit removes the patch, so that ember-cli goes back to generating a single `tests.js` bundle. This means that core test definitions will now be included in the bundle when using `/theme-qunit`, and so this commit also updates our test module filter to exclude them from the run. This is the same way that we handle plugin tests on the regular `/tests` route, and is fully supported by qunit.

For now, this keeps `/theme-qunit` working in both development and production environments. However, we are very likely to drop support in production as part of the move to Embroider.
---
 .../javascripts/discourse/ember-cli-build.js  | 66 ++++---------------
 .../javascripts/discourse/tests/index.html    |  3 +-
 .../discourse/tests/setup-tests.js            | 13 ++--
 .../discourse/tests/test-boot-ember-cli.js    |  3 +-
 .../discourse/tests/test-helper.js            |  1 +
 app/helpers/qunit_helper.rb                   |  2 +-
 app/views/qunit/theme.html.erb                |  2 +-
 spec/requests/qunit_controller_spec.rb        |  6 +-
 8 files changed, 29 insertions(+), 67 deletions(-)
 create mode 100644 app/assets/javascripts/discourse/tests/test-helper.js

diff --git a/app/assets/javascripts/discourse/ember-cli-build.js b/app/assets/javascripts/discourse/ember-cli-build.js
index b1082e07c27..3a21935e0c3 100644
--- a/app/assets/javascripts/discourse/ember-cli-build.js
+++ b/app/assets/javascripts/discourse/ember-cli-build.js
@@ -110,60 +110,6 @@ module.exports = function (defaults) {
     },
   });
 
-  // Patching a private method is not great, but there's no other way for us to tell
-  // Ember CLI that we want the tests alone in a package without helpers/fixtures, since
-  // we re-use those in the theme tests.
-  app._defaultPackager.packageApplicationTests = function (tree) {
-    let appTestTrees = []
-      .concat(
-        this.packageEmberCliInternalFiles(),
-        this.packageTestApplicationConfig(),
-        tree
-      )
-      .filter(Boolean);
-
-    appTestTrees = mergeTrees(appTestTrees, {
-      overwrite: true,
-      annotation: "TreeMerger (appTestTrees)",
-    });
-
-    const tests = concat(appTestTrees, {
-      inputFiles: ["**/tests/**/*-test.js"],
-      headerFiles: ["vendor/ember-cli/tests-prefix.js"],
-      footerFiles: ["vendor/ember-cli/app-config.js"],
-      outputFile: "/assets/core-tests.js",
-      annotation: "Concat: Core Tests",
-      sourceMapConfig: false,
-    });
-
-    const testHelpers = concat(appTestTrees, {
-      inputFiles: [
-        "**/tests/loader-shims.js",
-        "**/tests/test-boot-ember-cli.js",
-        "**/tests/helpers/**/*.js",
-        "**/tests/fixtures/**/*.js",
-        "**/tests/setup-tests.js",
-      ],
-      outputFile: "/assets/test-helpers.js",
-      annotation: "Concat: Test Helpers",
-      sourceMapConfig: false,
-    });
-
-    if (isTest) {
-      return mergeTrees([
-        tests,
-        testHelpers,
-        discourseScss(`${discourseRoot}/app/assets/stylesheets`, "qunit.scss"),
-        discourseScss(
-          `${discourseRoot}/app/assets/stylesheets`,
-          "qunit-custom.scss"
-        ),
-      ]);
-    } else {
-      return mergeTrees([tests, testHelpers]);
-    }
-  };
-
   // WARNING: We should only import scripts here if they are not in NPM.
   // For example: our very specific version of bootstrap-modal.
   app.import(vendorJs + "bootbox.js");
@@ -191,6 +137,17 @@ module.exports = function (defaults) {
     .findAddonByName("pretty-text")
     .treeForMarkdownItBundle();
 
+  let testemStylesheetTree;
+  if (isTest) {
+    testemStylesheetTree = mergeTrees([
+      discourseScss(`${discourseRoot}/app/assets/stylesheets`, "qunit.scss"),
+      discourseScss(
+        `${discourseRoot}/app/assets/stylesheets`,
+        "qunit-custom.scss"
+      ),
+    ]);
+  }
+
   return app.toTree([
     createI18nTree(discourseRoot, vendorJs),
     parsePluginClientSettings(discourseRoot, vendorJs, app),
@@ -214,5 +171,6 @@ module.exports = function (defaults) {
     }),
     generateScriptsTree(app),
     discoursePluginsTree,
+    testemStylesheetTree,
   ]);
 };
diff --git a/app/assets/javascripts/discourse/tests/index.html b/app/assets/javascripts/discourse/tests/index.html
index 004e7613e16..f51840ddcf2 100644
--- a/app/assets/javascripts/discourse/tests/index.html
+++ b/app/assets/javascripts/discourse/tests/index.html
@@ -67,8 +67,7 @@
     <template id="dynamic-test-js">
       {{content-for "test-plugin-css"}}
       {{content-for "test-plugin-js"}}
-      <script defer src="{{rootURL}}assets/test-helpers.js"></script>
-      <script defer src="{{rootURL}}assets/core-tests.js"></script>
+      <script defer src="{{rootURL}}assets/tests.js"></script>
       {{content-for "test-plugin-tests-js"}}
       <script defer src="{{rootURL}}assets/scripts/discourse-test-trigger-ember-cli-boot.js"></script>
       <script defer src="{{rootURL}}assets/scripts/discourse-boot.js"></script>
diff --git a/app/assets/javascripts/discourse/tests/setup-tests.js b/app/assets/javascripts/discourse/tests/setup-tests.js
index 45dd3b69e46..24968be7390 100644
--- a/app/assets/javascripts/discourse/tests/setup-tests.js
+++ b/app/assets/javascripts/discourse/tests/setup-tests.js
@@ -359,19 +359,25 @@ export default function setupTests(config) {
 
   const target = getUrlParameter("target") || "core";
 
+  const hasPluginJs = !!document.querySelector("script[data-discourse-plugin]");
+  const hasThemeJs = !!document.querySelector("script[data-theme-id]");
+
   const shouldLoadModule = (name) => {
     if (!/\-test/.test(name)) {
       return false;
     }
 
     const isPlugin = name.match(/\/plugins\//);
-    const isCore = !isPlugin;
+    const isTheme = name.match(/\/theme-\d+\//);
+    const isCore = !isPlugin && !isTheme;
     const pluginName = name.match(/\/plugins\/([\w-]+)\//)?.[1];
 
     const loadCore = target === "core" || target === "all";
     const loadAllPlugins = target === "plugins" || target === "all";
 
-    if (isCore && !loadCore) {
+    if (hasThemeJs) {
+      return isTheme;
+    } else if (isCore && !loadCore) {
       return false;
     } else if (isPlugin && !(loadAllPlugins || pluginName === target)) {
       return false;
@@ -389,9 +395,6 @@ export default function setupTests(config) {
   reportMemoryUsageAfterTests();
   patchFailedAssertion();
 
-  const hasPluginJs = !!document.querySelector("script[data-discourse-plugin]");
-  const hasThemeJs = !!document.querySelector("script[data-theme-id]");
-
   if (!hasPluginJs && !hasThemeJs) {
     configureRaiseOnDeprecation();
   }
diff --git a/app/assets/javascripts/discourse/tests/test-boot-ember-cli.js b/app/assets/javascripts/discourse/tests/test-boot-ember-cli.js
index fd8ffc4f209..7f8fa2be407 100644
--- a/app/assets/javascripts/discourse/tests/test-boot-ember-cli.js
+++ b/app/assets/javascripts/discourse/tests/test-boot-ember-cli.js
@@ -26,7 +26,8 @@ document.addEventListener("discourse-booted", () => {
 
   const params = new URLSearchParams(window.location.search);
   const target = params.get("target");
-  const testingCore = !target || target === "core";
+  const testingTheme = !!document.querySelector("script[data-theme-id]");
+  const testingCore = !testingTheme && (!target || target === "core");
   const disableAutoStart = params.get("qunit_disable_auto_start") === "1";
 
   Ember.ENV.LOG_STACKTRACE_ON_DEPRECATION = false;
diff --git a/app/assets/javascripts/discourse/tests/test-helper.js b/app/assets/javascripts/discourse/tests/test-helper.js
new file mode 100644
index 00000000000..6f3b3b60a7a
--- /dev/null
+++ b/app/assets/javascripts/discourse/tests/test-helper.js
@@ -0,0 +1 @@
+// We don't currently use this file, but it is require'd by ember-cli's test bundle
diff --git a/app/helpers/qunit_helper.rb b/app/helpers/qunit_helper.rb
index 2813ed1e9d6..c2eca924bb4 100644
--- a/app/helpers/qunit_helper.rb
+++ b/app/helpers/qunit_helper.rb
@@ -11,6 +11,6 @@ module QunitHelper
         "#{Discourse.base_path}" \
         "/theme-javascripts/tests/#{theme.id}-#{digest}.js" \
         "?__ws=#{Discourse.current_hostname}"
-    "<script defer src='#{src}'></script>".html_safe
+    "<script defer src='#{src}' data-theme-id='#{theme.id}'></script>".html_safe
   end
 end
diff --git a/app/views/qunit/theme.html.erb b/app/views/qunit/theme.html.erb
index 99560c118c4..be2e930c7bc 100644
--- a/app/views/qunit/theme.html.erb
+++ b/app/views/qunit/theme.html.erb
@@ -14,7 +14,7 @@
         <%= preload_script file %>
       <%- end %>
       <%= preload_script "test-support" %>
-      <%= preload_script "test-helpers" %>
+      <%= preload_script "tests" %>
       <%= preload_script "test-site-settings" %>
       <%= theme_translations_lookup %>
       <%= theme_js_lookup %>
diff --git a/spec/requests/qunit_controller_spec.rb b/spec/requests/qunit_controller_spec.rb
index a77ee41d054..c9ad03ffc29 100644
--- a/spec/requests/qunit_controller_spec.rb
+++ b/spec/requests/qunit_controller_spec.rb
@@ -96,9 +96,9 @@ RSpec.describe QunitController do
         expect(response.body).to include("/stylesheets/desktop_")
         expect(response.body).to include("* https://qunitjs.com/") # inlined QUnit CSS
         expect(response.body).to include("/assets/locales/en.js")
-        expect(response.body).to include("/test-support")
-        expect(response.body).to include("/test-helpers")
-        expect(response.body).to include("/test-site-settings")
+        expect(response.body).to include("/test-support.js")
+        expect(response.body).to include("/tests.js")
+        expect(response.body).to include("/test-site-settings.js")
         expect(response.body).to include("/assets/markdown-it-bundle.js")
         expect(response.body).to include("/assets/discourse.js")
         expect(response.body).to include("/assets/admin.js")