From 5ce33991f431fb2de51e52b2e716377d4721c488 Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Sat, 4 Jan 2025 12:55:22 +0100 Subject: [PATCH] DEV: Fix flaky deprecated setting specs (#30550) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …and remove obsolete (and already-disabled) TL-migration related specs --- .../site_settings/deprecated_test.yml | 9 - .../site_settings/deprecated_settings_spec.rb | 249 +++--------------- spec/models/trust_level_setting_spec.rb | 29 -- spec/rails_helper.rb | 17 ++ .../admin/site_settings_controller_spec.rb | 9 +- 5 files changed, 51 insertions(+), 262 deletions(-) diff --git a/spec/fixtures/site_settings/deprecated_test.yml b/spec/fixtures/site_settings/deprecated_test.yml index 47dc19c96f3..ab19f3cf826 100644 --- a/spec/fixtures/site_settings/deprecated_test.yml +++ b/spec/fixtures/site_settings/deprecated_test.yml @@ -1,13 +1,4 @@ category1: - use_https: true - min_trust_level_to_allow_invite: - default: 2 - enum: "TrustLevelSetting" - hidden: true - min_trust_level_to_allow_invite_tl_and_staff: - default: 2 - enum: "TrustLevelAndStaffSetting" - hidden: true old_one: default: false new_one: diff --git a/spec/lib/site_settings/deprecated_settings_spec.rb b/spec/lib/site_settings/deprecated_settings_spec.rb index 05c8274ead3..91d2db152a7 100644 --- a/spec/lib/site_settings/deprecated_settings_spec.rb +++ b/spec/lib/site_settings/deprecated_settings_spec.rb @@ -1,235 +1,52 @@ # frozen_string_literal: true -RSpec.xdescribe SiteSettings::DeprecatedSettings do - def deprecate_override!(settings, tl_group_overrides = []) - @original_settings = SiteSettings::DeprecatedSettings::SETTINGS.dup - SiteSettings::DeprecatedSettings::SETTINGS.clear - SiteSettings::DeprecatedSettings::SETTINGS.push(settings) - - if tl_group_overrides.any? - @original_override_tl_group = SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.dup - SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.clear - SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.push(*tl_group_overrides) - end - - SiteSetting.setup_deprecated_methods - end - - after do - if defined?(@original_settings) - SiteSettings::DeprecatedSettings::SETTINGS.clear - SiteSettings::DeprecatedSettings::SETTINGS.concat(@original_settings) - end - - if defined?(@original_override_tl_group) - SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.clear - SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.concat( - @original_override_tl_group, - ) - end - - SiteSetting.setup_deprecated_methods - end - +RSpec.describe SiteSettings::DeprecatedSettings do describe "when not overriding deprecated settings" do - let(:override) { false } + it "does not act as a proxy to the new methods" do + stub_deprecated_settings!(override: false) do + SiteSetting.old_one = true - # NOTE: This fixture has some completely made up settings (e.g. min_trust_level_to_allow_invite_tl_and_staff) - let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" } - - before do - SiteSetting.force_https = true - SiteSetting.load_settings(deprecated_test) + expect(SiteSetting.new_one).to eq(false) + expect(SiteSetting.new_one?).to eq(false) + end end - it "should not act as a proxy to the new methods" do - deprecate_override!(["use_https", "force_https", override, "0.0.1"]) + it "logs warnings when deprecated settings are called" do + stub_deprecated_settings!(override: false) do + logger = + track_log_messages do + expect(SiteSetting.old_one).to eq(false) + expect(SiteSetting.old_one?).to eq(false) + end + expect(logger.warnings.count).to eq(3) - SiteSetting.use_https = false - - expect(SiteSetting.force_https).to eq(true) - expect(SiteSetting.force_https?).to eq(true) - end - - it "should log warnings when deprecated settings are called" do - deprecate_override!(["use_https", "force_https", override, "0.0.1"]) - - logger = - track_log_messages do - expect(SiteSetting.use_https).to eq(true) - expect(SiteSetting.use_https?).to eq(true) - end - expect(logger.warnings.count).to eq(3) - - logger = track_log_messages { SiteSetting.use_https(warn: false) } - expect(logger.warnings.count).to eq(0) + logger = track_log_messages { SiteSetting.old_one(warn: false) } + expect(logger.warnings.count).to eq(0) + end end end describe "when overriding deprecated settings" do - let(:override) { true } - let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" } + it "acts as a proxy to the new methods" do + stub_deprecated_settings!(override: true) do + SiteSetting.old_one = true - before do - SiteSetting.force_https = true - SiteSetting.load_settings(deprecated_test) - end - - it "should act as a proxy to the new methods" do - deprecate_override!(["use_https", "force_https", override, "0.0.1"]) - - SiteSetting.use_https = false - - expect(SiteSetting.force_https).to eq(false) - expect(SiteSetting.force_https?).to eq(false) - end - - xit "should log warnings when deprecated settings are called" do - deprecate_override!(["use_https", "force_https", override, "0.0.1"]) - - logger = - track_log_messages do - expect(SiteSetting.use_https).to eq(true) - expect(SiteSetting.use_https?).to eq(true) - end - expect(logger.warnings.count).to eq(2) - - logger = track_log_messages { SiteSetting.use_https(warn: false) } - expect(logger.warnings.count).to eq(0) - end - end - - describe "when overriding a trust level setting with a group setting" do - let(:override) { false } - let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" } - - before { SiteSetting.load_settings(deprecated_test) } - - context "when getting an old TrustLevelSetting" do - before do - deprecate_override!( - ["min_trust_level_to_allow_invite", "invite_allowed_groups", override, "0.0.1"], - ) - end - - it "uses the minimum trust level from the trust level auto groups in the new group setting" do - SiteSetting.invite_allowed_groups = - "#{Group::AUTO_GROUPS[:trust_level_3]}|#{Group::AUTO_GROUPS[:trust_level_4]}" - expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[3]) - end - - it "returns TL4 if there are no trust level auto groups in the new group setting" do - SiteSetting.invite_allowed_groups = Fabricate(:group).id.to_s - expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4]) - end - - it "returns TL4 if there are only staff and admin auto groups in the new group setting" do - SiteSetting.invite_allowed_groups = - "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}" - expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4]) - end - - it "returns TL4 if there are no automated invite_allowed_groups" do - SiteSetting.invite_allowed_groups = Fabricate(:group).id.to_s - expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4]) + expect(SiteSetting.new_one).to eq(true) + expect(SiteSetting.new_one?).to eq(true) end end - context "when getting an old TrustLevelAndStaffSetting" do - before do - deprecate_override!( - [ - "min_trust_level_to_allow_invite_tl_and_staff", - "invite_allowed_groups", - override, - "0.0.1", - ], - ["min_trust_level_to_allow_invite_tl_and_staff"], - ) - end + it "logs warnings when deprecated settings are called" do + stub_deprecated_settings!(override: true) do + logger = + track_log_messages do + expect(SiteSetting.old_one).to eq(false) + expect(SiteSetting.old_one?).to eq(false) + end + expect(logger.warnings.count).to eq(2) - it "returns moderator if there is only the moderators auto group in the new group setting" do - SiteSetting.invite_allowed_groups = "#{Group::AUTO_GROUPS[:moderators]}" - expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("moderator") - end - - it "returns staff if there are staff and admin auto groups in the new group setting" do - SiteSetting.invite_allowed_groups = - "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}" - expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("staff") - end - - it "returns admin if there is only the admin auto group in the new group setting" do - SiteSetting.invite_allowed_groups = "#{Group::AUTO_GROUPS[:admins]}" - expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("admin") - end - - it "returns the min trust level if the admin auto group as well as lower TL auto groups in the new group setting" do - SiteSetting.invite_allowed_groups = - "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:trust_level_3]}" - expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq(TrustLevel[3]) - end - - it "returns admin if there are no automated invite_allowed_groups" do - SiteSetting.invite_allowed_groups = Fabricate(:group).id.to_s - expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("admin") - end - end - - context "when setting an old TrustLevelSetting" do - before do - deprecate_override!( - ["min_trust_level_to_allow_invite", "invite_allowed_groups", override, "0.0.1"], - ["min_trust_level_to_allow_invite"], - ) - end - - it "converts the provided trust level to the appropriate auto group" do - SiteSetting.min_trust_level_to_allow_invite = TrustLevel[4] - expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4]) - expect(SiteSetting.invite_allowed_groups).to eq(Group::AUTO_GROUPS[:trust_level_4].to_s) - end - - it "raises error with an invalid trust level" do - expect { SiteSetting.min_trust_level_to_allow_invite = 66 }.to raise_error( - Discourse::InvalidParameters, - ) - end - end - - context "when setting an old TrustLevelAndStaffSetting" do - before do - deprecate_override!( - [ - "min_trust_level_to_allow_invite_tl_and_staff", - "invite_allowed_groups", - override, - "0.0.1", - ], - ["min_trust_level_to_allow_invite_tl_and_staff"], - ) - end - - it "converts the provided trust level to the appropriate auto group" do - SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = "admin" - expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("admin") - expect(SiteSetting.invite_allowed_groups).to eq(Group::AUTO_GROUPS[:admins].to_s) - - SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = "staff" - expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("staff") - expect(SiteSetting.invite_allowed_groups).to eq(Group::AUTO_GROUPS[:staff].to_s) - - SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = TrustLevel[3] - expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq(TrustLevel[3]) - expect(SiteSetting.invite_allowed_groups).to eq( - "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}|#{Group::AUTO_GROUPS[:trust_level_3]}", - ) - end - - it "raises error with an invalid trust level" do - expect { SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = 66 }.to raise_error( - Discourse::InvalidParameters, - ) + logger = track_log_messages { SiteSetting.old_one(warn: false) } + expect(logger.warnings.count).to eq(0) end end end diff --git a/spec/models/trust_level_setting_spec.rb b/spec/models/trust_level_setting_spec.rb index 6dc42c98877..6a91368ce86 100644 --- a/spec/models/trust_level_setting_spec.rb +++ b/spec/models/trust_level_setting_spec.rb @@ -14,33 +14,4 @@ RSpec.describe TrustLevelSetting do expect(value[:value]).to eq(0) end end - - xdescribe ".valid_value?" do - let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" } - - before { SiteSetting.load_settings(deprecated_test) } - - it "allows all trust levels as valid values" do - expect(TrustLevelSetting.valid_value?(TrustLevel[0])).to eq(true) - expect(TrustLevelSetting.valid_value?(TrustLevel[1])).to eq(true) - expect(TrustLevelSetting.valid_value?(TrustLevel[2])).to eq(true) - expect(TrustLevelSetting.valid_value?(TrustLevel[3])).to eq(true) - expect(TrustLevelSetting.valid_value?(TrustLevel[4])).to eq(true) - expect(TrustLevelSetting.valid_value?(20)).to eq(false) - end - - it "does not allow 'admin' or 'staff' as valid values" do - expect(TrustLevelSetting.valid_value?("admin")).to eq(false) - expect(TrustLevelSetting.valid_value?("staff")).to eq(false) - end - - it "does not allow setting 'admin' or 'staff' as valid values" do - expect { SiteSetting.min_trust_level_to_allow_invite = "admin" }.to raise_error( - Discourse::InvalidParameters, - ) - expect { SiteSetting.min_trust_level_to_allow_invite = "staff" }.to raise_error( - Discourse::InvalidParameters, - ) - end - end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 47882ffac36..f99bccc6574 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -975,6 +975,23 @@ def has_trigger?(trigger_name) SQL end +def stub_deprecated_settings!(override:) + SiteSetting.load_settings("#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml") + + stub_const( + SiteSettings::DeprecatedSettings, + "SETTINGS", + [["old_one", "new_one", override, "0.0.1"]], + ) do + SiteSetting.setup_deprecated_methods + yield + end + + defaults = SiteSetting.defaults.instance_variable_get(:@defaults) + defaults.each { |_, hash| hash.delete(:old_one) } + defaults.each { |_, hash| hash.delete(:new_one) } +end + def silence_stdout STDOUT.stubs(:write) yield diff --git a/spec/requests/admin/site_settings_controller_spec.rb b/spec/requests/admin/site_settings_controller_spec.rb index f623f44bef6..f3a1e95d2b3 100644 --- a/spec/requests/admin/site_settings_controller_spec.rb +++ b/spec/requests/admin/site_settings_controller_spec.rb @@ -238,14 +238,7 @@ RSpec.describe Admin::SiteSettingsController do end it "works for deprecated settings" do - deprecated_test = "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" - SiteSetting.load_settings(deprecated_test) - - stub_const( - SiteSettings::DeprecatedSettings, - "SETTINGS", - [["old_one", "new_one", true, "3.0"]], - ) do + stub_deprecated_settings!(override: true) do put "/admin/site_settings/old_one.json", params: { old_one: true } expect(response.status).to eq(200)