Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

DEV: Stop building test assets in production under Embroider (#23388) 

Until now, we have allowed testing themes in production environments via `/theme-qunit`. This was made possible by hacking the ember-cli build so that it would create the `tests.js` bundle in production. However, this is fundamentally problematic because a number of test-specific things are still optimized out of the Ember build in production mode. It also makes asset compilation significantly slower, and makes it more difficult for us to update our build pipeline (e.g. to introduce Embroider).

This commit removes the ability to run qunit tests in production builds of the JS app when the Embdroider flag is enabled. If a production instance of Discourse exists exclusively for the development of themes (e.g. discourse.theme-creator.io) then they can add `EMBER_ENV: development` to their `app.yml` file. This will build the entire app in development mode, and has a significant performance impact. This must not be used for real production sites.

This commit also refactors many of the request specs into system specs. This means that the tests are guaranteed to have Ember assets built, and is also a better end-to-end test than simply checking for the presence of certain `<script>` tags in the HTML.
This commit is contained in:
David Taylor 2023-09-11 09:12:37 +01:00 committed by GitHub
parent d28f113ce0
commit 9667485951
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 57 additions and 116 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
app/assets/javascripts/discourse/ember-cli-build.js
View File

@ -103,12 +103,9 @@ module.exports = function (defaults) {
"ember-cli-terser": { "ember-cli-terser": {
enabled: isProduction, enabled: isProduction,
exclude: [ exclude:
"**/test-*.js", ["**/highlightjs/*", "**/javascripts/*"] +
"**/core-tests*.js", (isEmbroider ? [] : ["**/test-*.js", "**/core-tests*.js"]),
"**/highlightjs/*",
"**/javascripts/*",
],
}, },
"ember-cli-babel": { "ember-cli-babel": {
@ -119,8 +116,9 @@ module.exports = function (defaults) {
plugins: [require.resolve("deprecation-silencer")], plugins: [require.resolve("deprecation-silencer")],
}, },
// We need to build tests in prod for theme tests // Was previously true so that we could run theme tests in production
tests: true, // but we're moving away from that as part of the Embroider migration
tests: isEmbroider ? !isProduction : true,
vendorFiles: { vendorFiles: {
// Freedom patch - includes bug fix and async stack support // Freedom patch - includes bug fix and async stack support

2
app/controllers/qunit_controller.rb
View File

@ -7,6 +7,8 @@ class QunitController < ApplicationController
def theme def theme
raise Discourse::NotFound.new if !can_see_theme_qunit? raise Discourse::NotFound.new if !can_see_theme_qunit?
@has_test_bundle = EmberCli.has_tests?
param_key = nil param_key = nil
@suggested_themes = nil @suggested_themes = nil
if (id = get_param(:id)).present? if (id = get_param(:id)).present?

11
app/views/qunit/theme.html.erb
View File

@ -5,7 +5,7 @@
<%= discourse_color_scheme_stylesheets %> <%= discourse_color_scheme_stylesheets %>
<meta name="color-scheme" content="light dark"> <meta name="color-scheme" content="light dark">
<%- if !@suggested_themes %> <%- if @has_test_bundle && !@suggested_themes %>
<%= preload_script "locales/#{I18n.locale}" %> <%= preload_script "locales/#{I18n.locale}" %>
<%= preload_script "vendor" %> <%= preload_script "vendor" %>
<%= preload_script "test-support" %> <%= preload_script "test-support" %>
@ -44,7 +44,10 @@
<%- end %> <%- end %>
</head> </head>
<body> <body>
<%- if @suggested_themes %> <%- if !@has_test_bundle %>
This is a production installation of Discourse, and cannot be used for theme testing.
For more information, see <a href="https://meta.discourse.org/t/66857">this guide</a>.
<% elsif @suggested_themes %>
<h2>Theme QUnit Test Runner</h2> <h2>Theme QUnit Test Runner</h2>
<%- if @suggested_themes.size == 0 %> <%- if @suggested_themes.size == 0 %>
@ -55,9 +58,7 @@
<h4><%= link_to name, theme_qunit_path(id: id) %></h4> <h4><%= link_to name, theme_qunit_path(id: id) %></h4>
<%- end %> <%- end %>
<%- end %> <%- end %>
<%- end %> <% else %>
<%- if !@suggested_themes %>
<%= preload_script "scripts/discourse-test-listen-boot" %> <%= preload_script "scripts/discourse-test-listen-boot" %>
<%= preload_script "scripts/discourse-boot" %> <%= preload_script "scripts/discourse-boot" %>
<%- end %> <%- end %>

4
lib/ember_cli.rb
View File

@ -103,4 +103,8 @@ module EmberCli
chunk_infos chunk_infos
end end
def self.has_tests?
File.exist?("#{dist_dir}/tests/index.html")
end
end end

4
lib/tasks/assets.rake
View File

@ -9,13 +9,15 @@ task "assets:precompile:before": "environment" do
end end
if ENV["EMBER_CLI_COMPILE_DONE"] != "1" if ENV["EMBER_CLI_COMPILE_DONE"] != "1"
compile_command = "yarn --cwd app/assets/javascripts/discourse run ember build -prod" compile_command = "yarn --cwd app/assets/javascripts/discourse run ember build"
if check_node_heap_size_limit < 1024 if check_node_heap_size_limit < 1024
STDERR.puts "Detected low Node.js heap_size_limit. Using --max-old-space-size=1024." STDERR.puts "Detected low Node.js heap_size_limit. Using --max-old-space-size=1024."
compile_command = "NODE_OPTIONS='--max-old-space-size=1024' #{compile_command}" compile_command = "NODE_OPTIONS='--max-old-space-size=1024' #{compile_command}"
end end
compile_command = "EMBER_ENV=production #{compile_command}" if ENV["EMBER_ENV"].nil?
only_assets_precompile_remaining = (ARGV.last == "assets:precompile") only_assets_precompile_remaining = (ARGV.last == "assets:precompile")
if only_assets_precompile_remaining if only_assets_precompile_remaining

122
spec/requests/qunit_controller_spec.rb
View File

@ -1,108 +1,30 @@
# frozen_string_literal: true # frozen_string_literal: true
RSpec.describe QunitController do RSpec.describe QunitController do
describe "#theme" do def production_sign_in(user)
let(:theme) { Fabricate(:theme, name: "main-theme") } # We need to call sign_in before stubbing the method because SessionController#become
let(:component) { Fabricate(:theme, component: true, name: "enabled-component") } # checks for the current env when the file is loaded.
let(:disabled_component) do # We need to make sure become is called once before stubbing, or the method
Fabricate(:theme, component: true, enabled: false, name: "disabled-component") # wont'be available for future tests if this one runs first.
end sign_in(user) if user
let(:theme_without_tests) { Fabricate(:theme, name: "no-tests-guy") } Rails.env.stubs(:production?).returns(true)
end
before do it "hides page for regular users in production" do
Theme.destroy_all production_sign_in(Fabricate(:user))
theme.set_default! get "/theme-qunit"
component.add_relative_theme!(:parent, theme) expect(response.status).to eq(404)
disabled_component.add_relative_theme!(:parent, theme) end
[theme, component, disabled_component].each do |t|
t.set_field(
target: :extra_js,
type: :js,
name: "discourse/initializers/my-#{t.id}-initializer.js",
value: "console.log(#{t.id});",
)
t.set_field(
target: :tests_js,
type: :js,
name: "acceptance/some-test-#{t.id}.js",
value: "assert.ok(#{t.id});",
)
t.save!
end
end
context "with non-admin users on production" do it "hides page for anon in production" do
before do production_sign_in(nil)
# We need to call sign_in before stubbing the method because SessionController#become get "/theme-qunit"
# checks for the current env when the file is loaded. expect(response.status).to eq(404)
# We need to make sure become is called once before stubbing, or the method end
# wont'be available for future tests if this one runs first.
sign_in(Fabricate(:user))
Rails.env.stubs(:production?).returns(true)
end
it "regular users cannot see the page" do it "shows page for admin in production" do
get "/theme-qunit" production_sign_in(Fabricate(:admin))
expect(response.status).to eq(404) get "/theme-qunit"
end expect(response.status).to eq(200)
it "anons cannot see the page" do
sign_out
get "/theme-qunit"
expect(response.status).to eq(404)
end
end
context "with admin users" do
before { sign_in(Fabricate(:admin)) }
context "when no theme is specified" do
it "renders a list of themes and components that have tests" do
get "/theme-qunit"
expect(response.status).to eq(200)
[theme, component, disabled_component].each do |t|
expect(response.body).to include(t.name)
expect(response.body).to include("/theme-qunit?id=#{t.id}")
end
expect(response.body).not_to include(theme_without_tests.name)
expect(response.body).not_to include("/theme-qunit?id=#{theme_without_tests.id}")
end
end
it "can specify theme by id" do
get "/theme-qunit?id=#{theme.id}"
expect(response.status).to eq(200)
expect(response.body).to include("/theme-javascripts/tests/#{theme.id}-")
end
it "can specify theme by name" do
get "/theme-qunit?name=#{theme.name}"
expect(response.status).to eq(200)
expect(response.body).to include("/theme-javascripts/tests/#{theme.id}-")
end
it "can specify theme by url" do
theme.build_remote_theme(remote_url: "git@github.com:discourse/discourse.git").save!
theme.save!
get "/theme-qunit?url=#{theme.remote_theme.remote_url}"
expect(response.status).to eq(200)
expect(response.body).to include("/theme-javascripts/tests/#{theme.id}-")
end
it "themes qunit page includes all the JS/CSS it needs" do
get "/theme-qunit?id=#{theme.id}"
expect(response.status).to eq(200)
expect(response.body).to include("/stylesheets/color_definitions_base_")
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.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/admin.js")
expect(response.body).to match(/\/theme-javascripts\/\h{40}\.js/)
expect(response.body).to include("/theme-javascripts/tests/#{theme.id}-")
end
end
end end
end end

16
spec/system/theme_qunit_spec.rb
View File

@ -1,8 +1,9 @@
# frozen_string_literal: true # frozen_string_literal: true
describe "Theme qunit testing", type: :system do describe "Theme qunit testing", type: :system do
let!(:theme_without_tests) { Fabricate(:theme, name: "no-tests-guy") }
let!(:theme_with_test) do let!(:theme_with_test) do
t = Fabricate(:theme, name: "My Theme") t = Fabricate(:theme, name: "Theme With Tests")
t.set_field(target: :tests_js, type: :js, name: "acceptance/some-test.js", value: <<~JS) t.set_field(target: :tests_js, type: :js, name: "acceptance/some-test.js", value: <<~JS)
import { module, test } from "qunit"; import { module, test } from "qunit";
@ -12,16 +13,27 @@ describe "Theme qunit testing", type: :system do
}); });
}); });
JS JS
t.build_remote_theme(remote_url: "https://example.com/mytheme")
t.save! t.save!
t t
end end
it "can run theme tests correctly" do it "lists themes and can run tests by id, name and url" do
visit "/theme-qunit" visit "/theme-qunit"
expect(page).to have_css("a[href^='/theme-qunit?id=']", count: 1)
find("a[href=\"/theme-qunit?id=#{theme_with_test.id}\"]").click find("a[href=\"/theme-qunit?id=#{theme_with_test.id}\"]").click
success_count = find("#qunit-testresult-display .passed").text success_count = find("#qunit-testresult-display .passed").text
expect(success_count).to eq("1") expect(success_count).to eq("1")
visit "/theme-qunit?name=#{theme_with_test.name}"
success_count = find("#qunit-testresult-display .passed").text
expect(success_count).to eq("1")
visit "/theme-qunit?url=#{theme_with_test.remote_theme.remote_url}"
success_count = find("#qunit-testresult-display .passed").text
expect(success_count).to eq("1")
end end
end end