DEV: Don’t replace Rails logger in specs (#29721)

Instead of replacing the Rails logger in specs, we can instead use
`#broadcast_to` which has been introduced in Rails 7.

plugins/automation/spec/scripts/topic_spec.rb

@@ -167,12 +167,11 @@ describe "Topic" do
     end
 
     context "when creating the post fails" do
-      before do
-        @orig_logger = Rails.logger
-        Rails.logger = @fake_logger = FakeLogger.new
-      end
+      let(:fake_logger) { FakeLogger.new }
 
-      after { Rails.logger = @orig_logger }
+      before { Rails.logger.broadcast_to(fake_logger) }
+
+      after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
       it "logs a warning" do
         expect { UserUpdater.new(user, user).update(location: "Japan") }.to change {

plugins/automation/spec/scripts/user_group_membership_through_badge_spec.rb

@@ -28,12 +28,11 @@ describe "UserGroupMembershipThroughBadge" do
   end
 
   context "with invalid field values" do
-    before do
-      @original_logger = Rails.logger
-      Rails.logger = @fake_logger = FakeLogger.new
-    end
+    let(:fake_logger) { FakeLogger.new }
 
-    after { Rails.logger = @original_logger }
+    before { Rails.logger.broadcast_to(fake_logger) }
+
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
     context "with an unknown badge" do
       let(:unknown_badge_id) { -1 }
@@ -47,7 +46,7 @@ describe "UserGroupMembershipThroughBadge" do
       end
 
       it "logs warning message and does nothing" do
-        expect(@fake_logger.warnings).to include(
+        expect(fake_logger.warnings).to include(
           "[discourse-automation] Couldn’t find badge with id #{unknown_badge_id}",
         )
         expect(user.reload.groups).to be_empty
@@ -68,7 +67,7 @@ describe "UserGroupMembershipThroughBadge" do
           "user" => user,
         )
 
-        expect(@fake_logger.warnings).to include(
+        expect(fake_logger.warnings).to include(
           "[discourse-automation] Couldn’t find group with id #{target_group.id}",
         )
         expect(user.reload.groups).to be_empty

plugins/automation/spec/scripts/zapier_webhook_spec.rb

@@ -22,12 +22,11 @@ describe "ZapierWebhook" do
   end
 
   context "with invalid webhook url" do
-    before do
-      @orig_logger = Rails.logger
-      Rails.logger = @fake_logger = FakeLogger.new
-    end
+    let(:fake_logger) { FakeLogger.new }
 
-    after { Rails.logger = @orig_logger }
+    before { Rails.logger.broadcast_to(fake_logger) }
+
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
     it "logs an error and do nothing" do
       expect { automation.trigger! }.not_to change {

plugins/chat/spec/lib/chat/channel_archive_service_spec.rb

@@ -413,7 +413,6 @@ describe Chat::ChannelArchiveService do
       end
 
       it "handles errors gracefully, sends a private message to the archiving user, and is idempotent on retry" do
-        Rails.logger = @fake_logger = FakeLogger.new
         create_messages(35) && start_archive
 
         Chat::ChannelArchiveService

spec/lib/discourse_hub_spec.rb

@@ -106,12 +106,11 @@ RSpec.describe DiscourseHub do
   end
 
   describe ".collection_action" do
-    before do
-      @orig_logger = Rails.logger
-      Rails.logger = @fake_logger = FakeLogger.new
-    end
+    let(:fake_logger) { FakeLogger.new }
 
-    after { Rails.logger = @orig_logger }
+    before { Rails.logger.broadcast_to(fake_logger) }
+
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
     it "should log correctly on error" do
       stub_request(:get, (ENV["HUB_BASE_URL"] || "http://local.hub:3000/api") + "/test").to_return(
@@ -123,9 +122,9 @@ RSpec.describe DiscourseHub do
 
       DiscourseHub.collection_action(:get, "/test")
 
-      expect(@fake_logger.warnings).to eq([DiscourseHub.response_status_log_message("/test", 500)])
+      expect(fake_logger.warnings).to eq([DiscourseHub.response_status_log_message("/test", 500)])
 
-      expect(@fake_logger.errors).to eq([DiscourseHub.response_body_log_message("")])
+      expect(fake_logger.errors).to eq([DiscourseHub.response_body_log_message("")])
     end
   end
 end

spec/lib/discourse_ip_info_spec.rb

@@ -62,8 +62,8 @@ RSpec.describe DiscourseIpInfo do
     end
 
     it "should not throw an error and instead log the exception when database file fails to download" do
-      original_logger = Rails.logger
-      Rails.logger = fake_logger = FakeLogger.new
+      fake_logger = FakeLogger.new
+      Rails.logger.broadcast_to(fake_logger)
 
       global_setting :maxmind_license_key, "license_key"
       global_setting :maxmind_account_id, "account_id"
@@ -81,7 +81,7 @@ RSpec.describe DiscourseIpInfo do
         "MaxMind database GeoLite2-City download failed. 500 Error",
       )
     ensure
-      Rails.logger = original_logger
+      Rails.logger.stop_broadcasting_to(fake_logger)
     end
   end
 end

spec/lib/discourse_spec.rb

@@ -446,12 +446,11 @@ RSpec.describe Discourse do
       old_method(m)
     end
 
-    before do
-      @orig_logger = Rails.logger
-      Rails.logger = @fake_logger = FakeLogger.new
-    end
+    let(:fake_logger) { FakeLogger.new }
 
-    after { Rails.logger = @orig_logger }
+    before { Rails.logger.broadcast_to(fake_logger) }
+
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
     it "can deprecate usage" do
       k = SecureRandom.hex
@@ -459,19 +458,19 @@ RSpec.describe Discourse do
       expect(old_method_caller(k)).to include("discourse_spec")
       expect(old_method_caller(k)).to include(k)
 
-      expect(@fake_logger.warnings).to eq([old_method_caller(k)])
+      expect(fake_logger.warnings).to eq([old_method_caller(k)])
     end
 
     it "can report the deprecated version" do
       Discourse.deprecate(SecureRandom.hex, since: "2.1.0.beta1")
 
-      expect(@fake_logger.warnings[0]).to include("(deprecated since Discourse 2.1.0.beta1)")
+      expect(fake_logger.warnings[0]).to include("(deprecated since Discourse 2.1.0.beta1)")
     end
 
     it "can report the drop version" do
       Discourse.deprecate(SecureRandom.hex, drop_from: "2.3.0")
 
-      expect(@fake_logger.warnings[0]).to include("(removal in Discourse 2.3.0)")
+      expect(fake_logger.warnings[0]).to include("(removal in Discourse 2.3.0)")
     end
 
     it "can raise deprecation error" do

spec/lib/email/processor_spec.rb

@@ -105,29 +105,27 @@ RSpec.describe Email::Processor do
     let(:mail2) do
       "Date: Fri, 15 Jan 2016 00:12:43 +0100\nFrom: #{from}\nTo: foo@foo.com\nSubject: BAR BAR\n\nBar bar bar bar?"
     end
+    let(:fake_logger) { FakeLogger.new }
+
+    before { Rails.logger.broadcast_to(fake_logger) }
+
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
     it "sends a rejection email on an unrecognized error" do
-      begin
-        @orig_logger = Rails.logger
-        Rails.logger = @fake_logger = FakeLogger.new
+      Email::Processor.any_instance.stubs(:can_send_rejection_email?).returns(true)
+      Email::Receiver.any_instance.stubs(:process_internal).raises("boom")
 
-        Email::Processor.any_instance.stubs(:can_send_rejection_email?).returns(true)
-        Email::Receiver.any_instance.stubs(:process_internal).raises("boom")
+      Email::Processor.process!(mail)
 
-        Email::Processor.process!(mail)
+      errors = fake_logger.errors
+      expect(errors.size).to eq(1)
+      expect(errors.first).to include("boom")
 
-        errors = @fake_logger.errors
-        expect(errors.size).to eq(1)
-        expect(errors.first).to include("boom")
+      incoming_email = IncomingEmail.last
+      expect(incoming_email.error).to eq("RuntimeError")
+      expect(incoming_email.rejection_message).to be_present
 
-        incoming_email = IncomingEmail.last
-        expect(incoming_email.error).to eq("RuntimeError")
-        expect(incoming_email.rejection_message).to be_present
-
-        expect(EmailLog.last.email_type).to eq("email_reject_unrecognized_error")
-      ensure
-        Rails.logger = @orig_logger
-      end
+      expect(EmailLog.last.email_type).to eq("email_reject_unrecognized_error")
     end
 
     it "sends more than one rejection email per day" do

spec/lib/middleware/discourse_public_exceptions_spec.rb

@@ -1,12 +1,11 @@
 # frozen_string_literal: true
 
 RSpec.describe Middleware::DiscoursePublicExceptions do
-  before do
-    @orig_logger = Rails.logger
-    Rails.logger = @fake_logger = FakeLogger.new
-  end
+  let(:fake_logger) { FakeLogger.new }
 
-  after { Rails.logger = @orig_logger }
+  before { Rails.logger.broadcast_to(fake_logger) }
+
+  after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
   def env(opts = {})
     {
@@ -27,6 +26,6 @@ RSpec.describe Middleware::DiscoursePublicExceptions do
       ),
     )
 
-    expect(@fake_logger.warnings.length).to eq(0)
+    expect(fake_logger.warnings.length).to eq(0)
   end
 end

spec/lib/middleware/request_tracker_spec.rb

@@ -505,19 +505,19 @@ RSpec.describe Middleware::RequestTracker do
   end
 
   describe "rate limiting" do
+    let(:fake_logger) { FakeLogger.new }
+
     before do
       RateLimiter.enable
       RateLimiter.clear_all_global!
 
-      @orig_logger = Rails.logger
-      Rails.logger = @fake_logger = FakeLogger.new
-
+      Rails.logger.broadcast_to(fake_logger)
       # rate limiter tests depend on checks for retry-after
       # they can be sensitive to clock skew during test runs
       freeze_time_safe
     end
 
-    after { Rails.logger = @orig_logger }
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
     let :middleware do
       app = lambda { |env| [200, {}, ["OK"]] }
@@ -559,7 +559,7 @@ RSpec.describe Middleware::RequestTracker do
         status, _ = middleware.call(env1)
         status, _ = middleware.call(env1)
 
-        expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(
+        expect(fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(
           warn_count,
         )
         expect(status).to eq(429)
@@ -675,9 +675,7 @@ RSpec.describe Middleware::RequestTracker do
         status, _ = middleware.call(env1)
         status, _ = middleware.call(env1)
 
-        expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(
-          0,
-        )
+        expect(fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(0)
         expect(status).to eq(200)
       end
     end
@@ -689,7 +687,7 @@ RSpec.describe Middleware::RequestTracker do
       status, _ = middleware.call(env)
       status, headers = middleware.call(env)
 
-      expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(1)
+      expect(fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(1)
       expect(status).to eq(429)
       expect(headers["Retry-After"]).to eq("10")
     end
@@ -701,7 +699,7 @@ RSpec.describe Middleware::RequestTracker do
       status, _ = middleware.call(env)
       status, _ = middleware.call(env)
 
-      expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(1)
+      expect(fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(1)
       expect(status).to eq(200)
     end
 
@@ -1077,12 +1075,11 @@ RSpec.describe Middleware::RequestTracker do
   end
 
   describe "error handling" do
-    before do
-      @original_logger = Rails.logger
-      Rails.logger = @fake_logger = FakeLogger.new
-    end
+    let(:fake_logger) { FakeLogger.new }
 
-    after { Rails.logger = @original_logger }
+    before { Rails.logger.broadcast_to(fake_logger) }
+
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
     it "logs requests even if they cause exceptions" do
       app = lambda { |env| raise RateLimiter::LimitExceeded, 1 }
@@ -1090,7 +1087,7 @@ RSpec.describe Middleware::RequestTracker do
       expect { tracker.call(env) }.to raise_error(RateLimiter::LimitExceeded)
       CachedCounting.flush
       expect(ApplicationRequest.stats).to include("http_total_total" => 1)
-      expect(@fake_logger.warnings).to be_empty
+      expect(fake_logger.warnings).to be_empty
     end
   end
 end

spec/lib/mini_scheduler_long_running_job_logger_spec.rb

@@ -38,12 +38,11 @@ RSpec.describe MiniSchedulerLongRunningJobLogger do
     manager.stop!
   end
 
-  before do
-    @orig_logger = Rails.logger
-    Rails.logger = @fake_logger = FakeLogger.new
-  end
+  let(:fake_logger) { FakeLogger.new }
 
-  after { Rails.logger = @orig_logger }
+  before { Rails.logger.broadcast_to(fake_logger) }
+
+  after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
   it "logs long running jobs" do
     with_running_scheduled_job(Every10MinutesJob) do
@@ -58,28 +57,28 @@ RSpec.describe MiniSchedulerLongRunningJobLogger do
 
         wait_for { loops == 1 }
 
-        expect(@fake_logger.warnings.size).to eq(1)
+        expect(fake_logger.warnings.size).to eq(1)
 
-        expect(@fake_logger.warnings.first).to match(
+        expect(fake_logger.warnings.first).to match(
           "Sidekiq scheduled job `Every10MinutesJob` has been running for more than 30 minutes",
         )
 
         # Matches the backtrace
-        expect(@fake_logger.warnings.first).to match("sleep")
+        expect(fake_logger.warnings.first).to match("sleep")
 
         # Check that the logger doesn't log repeated warnings after 2 loops
         expect do
           checker.thread.wakeup # Force the thread to run the next loop
 
           wait_for { loops == 2 }
-        end.not_to change { @fake_logger.warnings.size }
+        end.not_to change { fake_logger.warnings.size }
 
         # Check that the logger doesn't log repeated warnings after 3 loops
         expect do
           checker.thread.wakeup # Force the thread to run the next loop
 
           wait_for { loops == 3 }
-        end.not_to change { @fake_logger.warnings.size }
+        end.not_to change { fake_logger.warnings.size }
       ensure
         checker.stop
         expect(checker.thread).to eq(nil)
@@ -100,9 +99,9 @@ RSpec.describe MiniSchedulerLongRunningJobLogger do
 
         wait_for { loops == 1 }
 
-        expect(@fake_logger.warnings.size).to eq(1)
+        expect(fake_logger.warnings.size).to eq(1)
 
-        expect(@fake_logger.warnings.first).to match(
+        expect(fake_logger.warnings.first).to match(
           "Sidekiq scheduled job `DailyJob` has been running for more than 120 minutes",
         )
       ensure

spec/lib/sidekiq_long_running_job_logger_spec.rb

@@ -3,12 +3,11 @@
 require "sidekiq_long_running_job_logger"
 
 RSpec.describe SidekiqLongRunningJobLogger do
-  before do
-    @orig_logger = Rails.logger
-    Rails.logger = @fake_logger = FakeLogger.new
-  end
+  let(:fake_logger) { FakeLogger.new }
 
-  after { Rails.logger = @orig_logger }
+  before { Rails.logger.broadcast_to(fake_logger) }
+
+  after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
   it "logs long-running jobs" do
     hostname = Discourse.os_hostname
@@ -58,9 +57,9 @@ RSpec.describe SidekiqLongRunningJobLogger do
 
       wait_for { loops == 1 }
 
-      expect(@fake_logger.warnings.size).to eq(1)
+      expect(fake_logger.warnings.size).to eq(1)
 
-      expect(@fake_logger.warnings).to include(
+      expect(fake_logger.warnings).to include(
         "Sidekiq job `TestWorker` has been running for more than 10 minutes\nline\nlines\n",
       )
 
@@ -68,9 +67,9 @@ RSpec.describe SidekiqLongRunningJobLogger do
 
       wait_for { loops == 2 }
 
-      expect(@fake_logger.warnings.size).to eq(1)
+      expect(fake_logger.warnings.size).to eq(1)
 
-      expect(@fake_logger.warnings).to include(
+      expect(fake_logger.warnings).to include(
         "Sidekiq job `TestWorker` has been running for more than 10 minutes\nline\nlines\n",
       )
     ensure

spec/models/report_spec.rb

@@ -1036,12 +1036,11 @@ RSpec.describe Report do
   end
 
   describe "unexpected error on report initialization" do
-    before do
-      @orig_logger = Rails.logger
-      Rails.logger = @fake_logger = FakeLogger.new
-    end
+    let(:fake_logger) { FakeLogger.new }
 
-    after { Rails.logger = @orig_logger }
+    before { Rails.logger.broadcast_to(fake_logger) }
+
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
     it "returns no report" do
       class ReportInitError < StandardError
@@ -1053,7 +1052,7 @@ RSpec.describe Report do
 
       expect(report).to be_nil
 
-      expect(@fake_logger.errors).to eq(["Couldn’t create report `signups`: <ReportInitError x>"])
+      expect(fake_logger.errors).to eq(["Couldn’t create report `signups`: <ReportInitError x>"])
     end
   end
 

spec/rails_helper.rb

@@ -967,12 +967,12 @@ ensure
 end
 
 def track_log_messages
-  old_logger = Rails.logger
-  logger = Rails.logger = FakeLogger.new
+  logger = FakeLogger.new
+  Rails.logger.broadcast_to(logger)
   yield logger
   logger
 ensure
-  Rails.logger = old_logger
+  Rails.logger.stop_broadcasting_to(logger)
 end
 
 # this takes a string and returns a copy where 2 different

spec/requests/application_controller_spec.rb

@@ -426,12 +426,11 @@ RSpec.describe ApplicationController do
       end
 
       describe "no logspam" do
-        before do
-          @orig_logger = Rails.logger
-          Rails.logger = @fake_logger = FakeLogger.new
-        end
+        let(:fake_logger) { FakeLogger.new }
 
-        after { Rails.logger = @orig_logger }
+        before { Rails.logger.broadcast_to(fake_logger) }
+
+        after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
         it "should handle 404 to a css file" do
           Discourse.cache.delete("page_not_found_topics:#{I18n.locale}")
@@ -453,9 +452,9 @@ RSpec.describe ApplicationController do
           expect(response.body).to include(topic1.title)
           expect(response.body).to_not include(topic2.title)
 
-          expect(@fake_logger.fatals.length).to eq(0)
-          expect(@fake_logger.errors.length).to eq(0)
-          expect(@fake_logger.warnings.length).to eq(0)
+          expect(fake_logger.fatals.length).to eq(0)
+          expect(fake_logger.errors.length).to eq(0)
+          expect(fake_logger.warnings.length).to eq(0)
         end
       end
 

spec/requests/csp_reports_controller_spec.rb

@@ -2,15 +2,16 @@
 
 RSpec.describe CspReportsController do
   describe "#create" do
+    let(:fake_logger) { FakeLogger.new }
+
     before do
       SiteSetting.content_security_policy = true
       SiteSetting.content_security_policy_collect_reports = true
 
-      @orig_logger = Rails.logger
-      Rails.logger = @fake_logger = FakeLogger.new
+      Rails.logger.broadcast_to(fake_logger)
     end
 
-    after { Rails.logger = @orig_logger }
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
     def send_report
       post "/csp_reports",
@@ -73,7 +74,7 @@ RSpec.describe CspReportsController do
 
     it "logs the violation report" do
       send_report
-      expect(@fake_logger.warnings).to include(
+      expect(fake_logger.warnings).to include(
         "CSP Violation: 'http://suspicio.us/assets.js' \n\nconsole.log('unsafe')",
       )
     end

spec/services/user_stat_count_updater_spec.rb

@@ -6,32 +6,33 @@ RSpec.describe UserStatCountUpdater do
   fab!(:post)
   fab!(:post_2) { Fabricate(:post, topic: post.topic) }
 
+  let(:fake_logger) { FakeLogger.new }
+
   before do
-    @orig_logger = Rails.logger
-    Rails.logger = @fake_logger = FakeLogger.new
+    Rails.logger.broadcast_to(fake_logger)
     SiteSetting.verbose_user_stat_count_logging = true
   end
 
-  after { Rails.logger = @orig_logger }
+  after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
   it "should log the exception when a negative count is inserted" do
     UserStatCountUpdater.decrement!(post, user_stat: user_stat)
 
-    expect(@fake_logger.warnings.last).to match("topic_count")
-    expect(@fake_logger.warnings.last).to match(post.id.to_s)
+    expect(fake_logger.warnings.last).to match("topic_count")
+    expect(fake_logger.warnings.last).to match(post.id.to_s)
 
     UserStatCountUpdater.decrement!(post_2, user_stat: user_stat)
 
-    expect(@fake_logger.warnings.last).to match("post_count")
-    expect(@fake_logger.warnings.last).to match(post_2.id.to_s)
+    expect(fake_logger.warnings.last).to match("post_count")
+    expect(fake_logger.warnings.last).to match(post_2.id.to_s)
   end
 
   it "should log the exception when a negative count will be inserted but 0 is used instead" do
     UserStatCountUpdater.set!(user_stat: user_stat, count: -10, count_column: :post_count)
 
-    expect(@fake_logger.warnings.last).to match("post_count")
-    expect(@fake_logger.warnings.last).to match("using 0")
-    expect(@fake_logger.warnings.last).to match("user #{user_stat.user_id}")
+    expect(fake_logger.warnings.last).to match("post_count")
+    expect(fake_logger.warnings.last).to match("using 0")
+    expect(fake_logger.warnings.last).to match("user #{user_stat.user_id}")
     expect(user_stat.reload.post_count).to eq(0)
   end
 end