mirror of
https://github.com/discourse/discourse.git
synced 2026-08-04 10:23:17 -05:00
DEV: Disallow the use of Rails.logger= in RSpec tests (#31920)
Setting Rails.logger after the application has been initialized does not seem to be safe anymore and can lead to flaky tests. This commit disallows the reassignment of `Rails.logger` going forward and updates the affected test. ### Reviewer notes Reassigning `Rails.logger` from within RSpec tests is causing tests which uses `Rails.logger.broadcast_to(FakeLogger.new)` to flake. Example: https://github.com/discourse/discourse/actions/runs/13951116847/job/39050616967 ``` 1) invalid requests handles NotFound with invalid json body Failure/Error: expect(fake_logger.errors).to have_attributes(size: 1) expected [] to have attributes {:size => 1} but had attributes {:size => 0} Diff: @@ -1 +1 @@ -:size => 1, +:size => 0, # ./spec/integration/invalid_request_spec.rb:18:in `block (2 levels) in <main>' # ./spec/rails_helper.rb:619:in `block (3 levels) in <top (required)>' # /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/benchmark-0.4.0/lib/benchmark.rb:304:in `measure' # ./spec/rails_helper.rb:619:in `block (2 levels) in <top (required)>' # ./spec/rails_helper.rb:580:in `block (3 levels) in <top (required)>' # /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/timeout-0.4.3/lib/timeout.rb:185:in `block in timeout' # /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/timeout-0.4.3/lib/timeout.rb:192:in `timeout' # ./spec/rails_helper.rb:570:in `block (2 levels) in <top (required)>' # ./spec/rails_helper.rb:527:in `block (2 levels) in <top (required)>' # /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/webmock-3.25.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>' ```
This commit is contained in:
@@ -1000,6 +1000,10 @@ ensure
|
||||
STDOUT.unstub(:write)
|
||||
end
|
||||
|
||||
def Rails.logger=(logger)
|
||||
raise "Setting Rails.logger is not allowed as it can lead to unexpected behavior in tests. Use `fake_logger = track_log_messages { ... }` instead."
|
||||
end
|
||||
|
||||
def track_log_messages
|
||||
logger = FakeLogger.new
|
||||
Rails.logger.broadcast_to(logger)
|
||||
|
||||
@@ -305,13 +305,11 @@ RSpec.describe ApplicationController do
|
||||
end
|
||||
|
||||
describe "invalid request params" do
|
||||
before do
|
||||
@old_logger = Rails.logger
|
||||
@logs = StringIO.new
|
||||
Rails.logger = Logger.new(@logs)
|
||||
end
|
||||
let(:fake_logger) { FakeLogger.new }
|
||||
|
||||
after { Rails.logger = @old_logger }
|
||||
before { Rails.logger.broadcast_to(fake_logger) }
|
||||
|
||||
after { Rails.logger.stop_broadcasting_to(fake_logger) }
|
||||
|
||||
it "should not raise a 500 (nor should it log a warning) for bad params" do
|
||||
bad_str = (+"d\xDE").force_encoding("utf-8")
|
||||
@@ -321,20 +319,7 @@ RSpec.describe ApplicationController do
|
||||
|
||||
expect(response.status).to eq(400)
|
||||
|
||||
log = @logs.string
|
||||
|
||||
if (log.include? "exception app middleware")
|
||||
# heisentest diagnostics
|
||||
puts
|
||||
puts "EXTRA DIAGNOSTICS FOR INTERMITTENT TEST FAIL"
|
||||
puts log
|
||||
puts ">> action_dispatch.exception"
|
||||
ex = request.env["action_dispatch.exception"]
|
||||
puts ">> exception class: #{ex.class} : #{ex}"
|
||||
end
|
||||
|
||||
expect(log).not_to include("exception app middleware")
|
||||
|
||||
expect(fake_logger.warnings.length).to eq(0)
|
||||
expect(response.status).to eq(400)
|
||||
end
|
||||
end
|
||||
|
||||
@@ -52,13 +52,11 @@ RSpec.describe Middleware::DefaultHeaders do
|
||||
end
|
||||
|
||||
context "when a rescued exception is raised" do
|
||||
before do
|
||||
@old_logger = Rails.logger
|
||||
@logs = StringIO.new
|
||||
Rails.logger = Logger.new(@logs)
|
||||
end
|
||||
let(:fake_logger) { FakeLogger.new }
|
||||
|
||||
after { Rails.logger = @old_logger }
|
||||
before { Rails.logger.broadcast_to(fake_logger) }
|
||||
|
||||
after { Rails.logger.stop_broadcasting_to(fake_logger) }
|
||||
|
||||
it "adds default headers to the response" do
|
||||
bad_str = (+"d\xDE").force_encoding("utf-8")
|
||||
@@ -66,9 +64,7 @@ RSpec.describe Middleware::DefaultHeaders do
|
||||
|
||||
get "/latest", params: { test: bad_str }
|
||||
|
||||
log = @logs.string
|
||||
expect(log).not_to include("exception app middleware")
|
||||
|
||||
expect(fake_logger.warnings.length).to eq(0)
|
||||
expect(response.status).to eq(400)
|
||||
expect(response.headers).to have_key("Cross-Origin-Opener-Policy")
|
||||
expect(response.headers["Cross-Origin-Opener-Policy"]).to eq("same-origin-allow-popups")
|
||||
|
||||
Reference in New Issue
Block a user