github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2024-11-25 09:42:07 +08:00
Code Issues Actions 6 Packages Projects Releases Wiki Activity

DEV: Avoid leaking new site setting states in test environment (#21713)

Browse Source 
What is the problem?

In the test environement, we were calling `SiteSetting.setting` directly
to introduce new site settings. However, this leads to changes in state of the SiteSettings
hash that is stored in memory as test runs. Changing or leaking states
when running tests is one of the major contributors of test flakiness.

An example of how this resulted in test flakiness is our `spec/integrity/i18n_spec.rb` spec file which
had a test case that would fail because a new "plugin_setting" site
setting was registered in another test case but the site setting did not
have translations for the site setting set.

What is the fix?

There are a couple of changes being introduced in this commit:

1. Make `SiteSetting.setting` a private method as it is not safe to be
   exposed as a public method of the `SiteSetting` class

2. Change test cases to use existing site settings in Discourse instead
   of creating custom site settings. Existing site settings are not
   removed often so we don't really need to dynamically add new site
   settings in test cases. Even if the site settings being used in test
   cases are removed, updating the test cases to rely on other site
   settings is a very easy change.

3. Set up a plugin instance in the test environment as a "fixture"
   instead of having each test create its own plugin instance.
This commit is contained in:
Alan Guo Xiang Tan 2023-05-25 08:53:57 +09:00 committed by GitHub
parent 62fe6a839f
commit 916495e0a1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 123 additions and 149 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
app/models/site_setting.rb
View File

@ -20,16 +20,22 @@ class SiteSetting < ActiveRecord::Base
end
end
def self.load_settings(file, plugin: nil)
SiteSettings::YamlLoader
.new(file)
.load do |category, name, default, opts|
setting(name, default, opts.merge(category: category, plugin: plugin))
end
end
load_settings(File.join(Rails.root, "config", "site_settings.yml"))
if Rails.env.test?
SAMPLE_TEST_PLUGIN =
Plugin::Instance.new(
Plugin::Metadata.new.tap { |metadata| metadata.name = "discourse-sample-plugin" },
)
Discourse.plugins_by_name[SAMPLE_TEST_PLUGIN.name] = SAMPLE_TEST_PLUGIN
load_settings(
File.join(Rails.root, "spec", "support", "sample_plugin_site_settings.yml"),
plugin: SAMPLE_TEST_PLUGIN.name,
)
end
if GlobalSetting.load_plugins?
Dir[File.join(Rails.root, "plugins", "*", "config", "settings.yml")].each do |file|
load_settings(file, plugin: file.split("/")[-3])

118
lib/site_setting_extension.rb
View File

@ -115,69 +115,12 @@ module SiteSettingExtension
@plugins ||= {}
end
def setting(name_arg, default = nil, opts = {})
name = name_arg.to_sym
if name == :default_locale
raise Discourse::InvalidParameters.new(
"Other settings depend on default locale, you can not configure it like this",
)
end
shadowed_val = nil
mutex.synchronize do
defaults.load_setting(name, default, opts.delete(:locale_default))
categories[name] = opts[:category] || :uncategorized
hidden_settings << name if opts[:hidden]
if GlobalSetting.respond_to?(name)
val = GlobalSetting.public_send(name)
unless val.nil? || (val == "")
shadowed_val = val
hidden_settings << name
shadowed_settings << name
end
def load_settings(file, plugin: nil)
SiteSettings::YamlLoader
.new(file)
.load do |category, name, default, opts|
setting(name, default, opts.merge(category: category, plugin: plugin))
end
refresh_settings << name if opts[:refresh]
client_settings << name.to_sym if opts[:client]
previews[name] = opts[:preview] if opts[:preview]
secret_settings << name if opts[:secret]
plugins[name] = opts[:plugin] if opts[:plugin]
type_supervisor.load_setting(
name,
opts.extract!(*SiteSettings::TypeSupervisor::CONSUMED_OPTS),
)
if !shadowed_val.nil?
setup_shadowed_methods(name, shadowed_val)
else
setup_methods(name)
end
end
end
def remove_setting(name_arg)
raise if !Rails.env.test?
name = name_arg.to_sym
categories.delete(name)
hidden_settings.delete(name)
refresh_settings.delete(name)
client_settings.delete(name)
previews.delete(name)
secret_settings.delete(name)
plugins.delete(name)
end
def settings_hash
@ -620,6 +563,57 @@ module SiteSettingExtension
private
def setting(name_arg, default = nil, opts = {})
name = name_arg.to_sym
if name == :default_locale
raise Discourse::InvalidParameters.new(
"Other settings depend on default locale, you can not configure it like this",
)
end
shadowed_val = nil
mutex.synchronize do
defaults.load_setting(name, default, opts.delete(:locale_default))
categories[name] = opts[:category] || :uncategorized
hidden_settings << name if opts[:hidden]
if GlobalSetting.respond_to?(name)
val = GlobalSetting.public_send(name)
unless val.nil? || (val == "")
shadowed_val = val
hidden_settings << name
shadowed_settings << name
end
end
refresh_settings << name if opts[:refresh]
client_settings << name.to_sym if opts[:client]
previews[name] = opts[:preview] if opts[:preview]
secret_settings << name if opts[:secret]
plugins[name] = opts[:plugin] if opts[:plugin]
type_supervisor.load_setting(
name,
opts.extract!(*SiteSettings::TypeSupervisor::CONSUMED_OPTS),
)
if !shadowed_val.nil?
setup_shadowed_methods(name, shadowed_val)
else
setup_methods(name)
end
end
end
def default_uploads
@default_uploads ||= {}

2
spec/integrity/i18n_spec.rb
View File

@ -31,7 +31,7 @@ end
RSpec.describe "i18n integrity checks" do
it "has an i18n key for each Site Setting" do
SiteSetting.all_settings.each do |s|
next if s[:setting][/^test_/]
next if s[:plugin] == SiteSetting::SAMPLE_TEST_PLUGIN.name
expect(s[:description]).not_to match(/translation missing/)
end
end

108
spec/requests/admin/site_settings_controller_spec.rb
View File

@ -48,12 +48,6 @@ RSpec.describe Admin::SiteSettingsController do
fab!(:staged_user) { Fabricate(:staged) }
let(:tracking) { NotificationLevels.all[:tracking] }
before do
SiteSetting.setting(:test_setting, "default")
SiteSetting.setting(:test_upload, "", type: :upload)
SiteSetting.refresh!
end
context "when logged in as an admin" do
before { sign_in(admin) }
@ -75,8 +69,6 @@ RSpec.describe Admin::SiteSettingsController do
}
expect(response.parsed_body["user_count"]).to eq(User.real.where(staged: false).count - 1)
SiteSetting.setting(:default_categories_watching, "")
end
it "should return correct user count for default tags change" do
@ -97,8 +89,6 @@ RSpec.describe Admin::SiteSettingsController do
}
expect(response.parsed_body["user_count"]).to eq(User.real.where(staged: false).count - 1)
SiteSetting.setting(:default_tags_watching, "")
end
context "for sidebar defaults" do
@ -228,19 +218,13 @@ RSpec.describe Admin::SiteSettingsController do
end
describe "#update" do
before do
SiteSetting.setting(:test_setting, "default")
SiteSetting.setting(:test_upload, "", type: :upload)
SiteSetting.refresh!
end
context "when logged in as an admin" do
before { sign_in(admin) }
it "sets the value when the param is present" do
put "/admin/site_settings/test_setting.json", params: { test_setting: "hello" }
put "/admin/site_settings/title.json", params: { title: "hello" }
expect(response.status).to eq(200)
expect(SiteSetting.test_setting).to eq("hello")
expect(SiteSetting.title).to eq("hello")
end
it "works for deprecated settings" do
@ -280,9 +264,9 @@ RSpec.describe Admin::SiteSettingsController do
end
it "allows value to be a blank string" do
put "/admin/site_settings/test_setting.json", params: { test_setting: "" }
put "/admin/site_settings/title.json", params: { title: "" }
expect(response.status).to eq(200)
expect(SiteSetting.test_setting).to eq("")
expect(SiteSetting.title).to eq("")
end
context "with default user options" do
@ -400,7 +384,8 @@ RSpec.describe Admin::SiteSettingsController do
let(:category_ids) { 3.times.collect { Fabricate(:category).id } }
before do
SiteSetting.setting(:default_categories_watching, category_ids.first(2).join("|"))
SiteSetting.default_categories_watching = category_ids.first(2).join("|")
CategoryUser.create!(
category_id: category_ids.last,
notification_level: tracking,
@ -408,8 +393,6 @@ RSpec.describe Admin::SiteSettingsController do
)
end
after { SiteSetting.setting(:default_categories_watching, "") }
it "should update existing users user preference" do
put "/admin/site_settings/default_categories_watching.json",
params: {
@ -515,12 +498,10 @@ RSpec.describe Admin::SiteSettingsController do
let(:tags) { 3.times.collect { Fabricate(:tag) } }
before do
SiteSetting.setting(:default_tags_watching, tags.first(2).pluck(:name).join("|"))
SiteSetting.default_tags_watching = tags.first(2).pluck(:name).join("|")
TagUser.create!(tag_id: tags.last.id, notification_level: tracking, user: user2)
end
after { SiteSetting.setting(:default_tags_watching, "") }
it "should update existing users user preference" do
put "/admin/site_settings/default_tags_watching.json",
params: {
@ -550,31 +531,40 @@ RSpec.describe Admin::SiteSettingsController do
context "with upload site settings" do
it "can remove the site setting" do
SiteSetting.test_upload = Fabricate(:upload)
SiteSetting.push_notifications_icon = Fabricate(:upload)
put "/admin/site_settings/test_upload.json", params: { test_upload: nil }
put "/admin/site_settings/push_notifications_icon.json",
params: {
push_notifications_icon: nil,
}
expect(response.status).to eq(200)
expect(SiteSetting.test_upload).to eq(nil)
expect(SiteSetting.push_notifications_icon).to eq(nil)
end
it "can reset the site setting to the default" do
SiteSetting.test_upload = nil
SiteSetting.push_notifications_icon = nil
default_upload = Upload.find(-1)
put "/admin/site_settings/test_upload.json", params: { test_upload: default_upload.url }
put "/admin/site_settings/push_notifications_icon.json",
params: {
push_notifications_icon: default_upload.url,
}
expect(response.status).to eq(200)
expect(SiteSetting.test_upload).to eq(default_upload)
expect(SiteSetting.push_notifications_icon).to eq(default_upload)
end
it "can update the site setting" do
upload = Fabricate(:upload)
put "/admin/site_settings/test_upload.json", params: { test_upload: upload.url }
put "/admin/site_settings/push_notifications_icon.json",
params: {
push_notifications_icon: upload.url,
}
expect(response.status).to eq(200)
expect(SiteSetting.test_upload).to eq(upload)
expect(SiteSetting.push_notifications_icon).to eq(upload)
user_history = UserHistory.last
@ -586,58 +576,30 @@ RSpec.describe Admin::SiteSettingsController do
end
it "logs the change" do
SiteSetting.test_setting = "previous"
SiteSetting.title = "previous"
expect do
put "/admin/site_settings/test_setting.json", params: { test_setting: "hello" }
end.to change {
expect do put "/admin/site_settings/title.json", params: { title: "hello" } end.to change {
UserHistory.where(action: UserHistory.actions[:change_site_setting]).count
}.by(1)
expect(response.status).to eq(200)
expect(SiteSetting.test_setting).to eq("hello")
expect(SiteSetting.title).to eq("hello")
end
it "does not allow changing of hidden settings" do
SiteSetting.setting(:hidden_setting, "hidden", hidden: true)
SiteSetting.refresh!
SiteSetting.max_category_nesting = 3
put "/admin/site_settings/hidden_setting.json", params: { hidden_setting: "not allowed" }
put "/admin/site_settings/max_category_nesting.json", params: { max_category_nesting: 2 }
expect(SiteSetting.hidden_setting).to eq("hidden")
expect(response.status).to eq(422)
end
it "does not allow changing of hidden settings" do
SiteSetting.setting(:hidden_setting, "hidden", hidden: true)
SiteSetting.refresh!
put "/admin/site_settings/hidden_setting.json", params: { hidden_setting: "not allowed" }
expect(SiteSetting.hidden_setting).to eq("hidden")
expect(SiteSetting.max_category_nesting).to eq(3)
expect(response.status).to eq(422)
end
context "with an plugin" do
let(:plugin) do
metadata = Plugin::Metadata.new
metadata.name = "discourse-plugin"
Plugin::Instance.new(metadata)
end
before do
Discourse.plugins_by_name[plugin.name] = plugin
SiteSetting.setting(:plugin_setting, "default value", plugin: "discourse-plugin")
SiteSetting.refresh!
end
after do
Discourse.plugins_by_name.delete(plugin.name)
SiteSetting.remove_setting(:plugin_setting)
end
it "allows changing settings of configurable plugins" do
plugin.stubs(:configurable?).returns(true)
SiteSetting::SAMPLE_TEST_PLUGIN.stubs(:configurable?).returns(true)
expect(SiteSetting.plugin_setting).to eq("some value")
put "/admin/site_settings/plugin_setting.json", params: { plugin_setting: "new value" }
@ -646,11 +608,11 @@ RSpec.describe Admin::SiteSettingsController do
end
it "does not allow changing of unconfigurable settings" do
plugin.stubs(:configurable?).returns(false)
SiteSetting::SAMPLE_TEST_PLUGIN.stubs(:configurable?).returns(false)
put "/admin/site_settings/plugin_setting.json", params: { plugin_setting: "not allowed" }
expect(SiteSetting.plugin_setting).to eq("default value")
expect(SiteSetting.plugin_setting).to eq("some value")
expect(response.status).to eq(422)
end
end

11
spec/requests/static_controller_spec.rb
View File

@ -289,9 +289,9 @@ RSpec.describe StaticController do
Discourse::Application.routes.send(:eval_block, routes)
topic_id = Fabricate(:post, cooked: "contact info").topic_id
SiteSetting.setting(:test_contact_topic_id, topic_id)
SiteSetting.test_some_topic_id = topic_id
Plugin::Instance.new.add_topic_static_page("contact", topic_id: "test_contact_topic_id")
Plugin::Instance.new.add_topic_static_page("contact", topic_id: "test_some_topic_id")
get "/contact"
@ -301,14 +301,15 @@ RSpec.describe StaticController do
it "replaces existing topic-backed pages" do
topic_id = Fabricate(:post, cooked: "Regular FAQ").topic_id
SiteSetting.setting(:test_faq_topic_id, topic_id)
SiteSetting.test_some_topic_id = topic_id
polish_topic_id = Fabricate(:post, cooked: "Polish FAQ").topic_id
SiteSetting.setting(:test_polish_faq_topic_id, polish_topic_id)
SiteSetting.test_some_other_topic_id = polish_topic_id
Plugin::Instance
.new
.add_topic_static_page("faq") do
current_user&.locale == "pl" ? "test_polish_faq_topic_id" : "test_faq_topic_id"
current_user&.locale == "pl" ? "test_some_other_topic_id" : "test_some_topic_id"
end
get "/faq"

7
spec/support/sample_plugin_site_settings.yml Normal file
View File

@ -0,0 +1,7 @@
site_settings:
plugin_setting:
default: "some value"
test_some_topic_id:
default: 0
test_some_other_topic_id:
default: 0

4
spec/support/site_settings_helpers.rb
View File

@ -8,6 +8,10 @@ module SiteSettingsHelpers
# so we set listen_for_changes to false
self.listen_for_changes = false
self.provider = provider
def self.setting(*args)
super
end
end
end
end