Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call super in Formatter.close methods to ensure sync is reset #3094

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
12 changes: 10 additions & 2 deletions lib/rspec/core/formatters/base_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class BaseFormatter
def initialize(output)
@output = output || StringIO.new
@example_group = nil
@sync_started = false
end

# @api public
Expand Down Expand Up @@ -54,16 +55,23 @@ def close(_notification)
private

def start_sync_output
@old_sync, output.sync = output.sync, true if output_supports_sync
if output_supports_sync
@sync_started = true
@old_sync, output.sync = output.sync, true
end
end

def restore_sync_output
output.sync = @old_sync if output_supports_sync && !output.closed?
output.sync = @old_sync if sync_started? && !output.closed?
rahearn marked this conversation as resolved.
Show resolved Hide resolved
end

def output_supports_sync
output.respond_to?(:sync=)
end

def sync_started?
@sync_started
end
end
end
end
Expand Down
10 changes: 5 additions & 5 deletions lib/rspec/core/formatters/base_text_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ def seed(notification)
#
# @param _notification [NullNotification] (Ignored)
def close(_notification)
return if output.closed?

output.puts

output.flush
unless output.closed?
output.puts
output.flush
end
super
rahearn marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
Expand Down
1 change: 1 addition & 0 deletions lib/rspec/core/formatters/json_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ def seed(notification)

def close(_notification)
output.write @output_hash.to_json
super
end

def dump_profile(profile)
Expand Down
14 changes: 14 additions & 0 deletions spec/rspec/core/formatters/base_text_formatter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@
let(:output_to_close) { File.new("./output_to_close", "w") }
let(:formatter) { described_class.new(output_to_close) }

before do
# Call `formatter.start` in order to initialize `@old_sync` before using in `BaseFormatter#close`
formatter.start(RSpec::Core::Notifications::StartNotification.new({:count => 1}))
end

after do
# Windows appears to not let the `:isolated_directory` shared group
# cleanup if the file isn't closed.
Expand All @@ -30,6 +35,15 @@
formatter.close(RSpec::Core::Notifications::NullNotification)
expect(output_to_close.closed?).to be(false)
end

it "restores the output's sync setting" do
if !output_to_close.respond_to?(:sync=)
skip "output_to_close does not support sync, so closing the formatter will not reset it"
rahearn marked this conversation as resolved.
Show resolved Hide resolved
end
expect {
formatter.close(RSpec::Core::Notifications::NullNotification)
}.to change(output_to_close, :sync).from(true).to(false)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad this passes as is, but is it in the right place? This is actually implemented in the base formatter, and if this is testing the super call then it doesn't cover the base formatter and the json formatter, prehaps some of these should be a shared example we call across all the formatters that need it?

end

describe "#dump_summary" do
Expand Down
Loading