Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Avoid using RSpec.configuration from the bisect coordinator. #2507

Merged
merged 1 commit into from
Jan 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 15 additions & 22 deletions lib/rspec/core/bisect/coordinator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,47 +15,40 @@ module Bisect
# - Formatters::BisectProgressFormatter: provides progress updates
# to the user.
class Coordinator
def self.bisect_with(original_cli_args, configuration, formatter)
new(original_cli_args, configuration, formatter).bisect
def self.bisect_with(original_cli_args, formatter)
new(original_cli_args, formatter).bisect
end

def initialize(original_cli_args, configuration, formatter)
def initialize(original_cli_args, formatter)
@shell_command = ShellCommand.new(original_cli_args)
@configuration = configuration
@formatter = formatter
@notifier = Bisect::Notifier.new(formatter)
end

def bisect
@configuration.add_formatter @formatter
Copy link
Member

Choose a reason for hiding this comment

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

so we only used the config to add a formatter. Instead now we just call the formatter directly, right?

So that probably explains why we don't need to register all the events below. This also implies that previously I would have been able to hook in my own formatter to bisect events, where as now I ... can't? Guess it depends where the formatter comes from that's passed into the initializer.

All LGTM, I figure you have this in your head, just trying to see if I'm following along :)

Copy link
Member Author

Choose a reason for hiding this comment

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

so we only used the config to add a formatter. Instead now we just call the formatter directly, right?

We used the config for 2 things:

  • To add the formatter
  • To access the reporter, which we used to publish events to the formatter

Now we just publish events to the formatter directly, without config and the reporter mediating access.

So that probably explains why we don't need to register all the events below.

Yep!

This also implies that previously I would have been able to hook in my own formatter to bisect events, where as now I ... can't?

In theory, yes, you could have, but we did not provide any way for users to specify a custom bisect formatter, so in practice, a user would have had to monkey patch RSpec to make it use their custom formatter for bisect events. So we aren't breaking any public APIs here.

Copy link
Member

Choose a reason for hiding this comment

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

thank you for extra explanation!

repro = ShellRunner.start(@shell_command) do |runner|
minimizer = ExampleMinimizer.new(@shell_command, runner, @notifier)

reporter.close_after do
repro = ShellRunner.start(@shell_command) do |runner|
minimizer = ExampleMinimizer.new(@shell_command, runner, reporter)

gracefully_abort_on_sigint(minimizer)
minimizer.find_minimal_repro
minimizer.repro_command_for_currently_needed_ids
end

reporter.publish(:bisect_repro_command, :repro => repro)
gracefully_abort_on_sigint(minimizer)
minimizer.find_minimal_repro
minimizer.repro_command_for_currently_needed_ids
end

@notifier.publish(:bisect_repro_command, :repro => repro)

true
rescue BisectFailedError => e
reporter.publish(:bisect_failed, :failure_explanation => e.message)
@notifier.publish(:bisect_failed, :failure_explanation => e.message)
false
ensure
@notifier.publish(:close)
end

private

def reporter
@configuration.reporter
end

def gracefully_abort_on_sigint(minimizer)
trap('INT') do
repro = minimizer.repro_command_for_currently_needed_ids
reporter.publish(:bisect_aborted, :repro => repro)
@notifier.publish(:bisect_aborted, :repro => repro)
exit(1)
end
end
Expand Down
8 changes: 4 additions & 4 deletions lib/rspec/core/bisect/example_minimizer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ module Bisect
# Contains the core bisect logic. Searches for examples we can ignore by
# repeatedly running different subsets of the suite.
class ExampleMinimizer
attr_reader :shell_command, :runner, :reporter, :all_example_ids, :failed_example_ids
attr_reader :shell_command, :runner, :all_example_ids, :failed_example_ids
attr_accessor :remaining_ids

def initialize(shell_command, runner, reporter)
def initialize(shell_command, runner, notifier)
@shell_command = shell_command
@runner = runner
@reporter = reporter
@notifier = notifier
end

def find_minimal_repro
Expand Down Expand Up @@ -164,7 +164,7 @@ def abort_if_ordering_inconsistent(results)
end

def notify(*args)
reporter.publish(*args)
@notifier.publish(*args)
end
end
end
Expand Down
16 changes: 16 additions & 0 deletions lib/rspec/core/bisect/utilities.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,22 @@ def self.for_failed_spec_run(spec_output)
spec_output)
end
end

# Wraps a `formatter` providing a simple means to notify it in place
# of an `RSpec::Core::Reporter`, without involving configuration in
# any way.
# @private
class Notifier
def initialize(formatter)
@formatter = formatter
end

def publish(event, *args)
return unless @formatter.respond_to?(event)
notification = Notifications::CustomNotification.for(*args)
@formatter.__send__(event, notification)
end
end
end
end
end
18 changes: 3 additions & 15 deletions lib/rspec/core/formatters/bisect_progress_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,6 @@ module Formatters
# @private
# Produces progress output while bisecting.
class BisectProgressFormatter < BaseTextFormatter
# We've named all events with a `bisect_` prefix to prevent naming collisions.
Formatters.register self, :bisect_starting, :bisect_original_run_complete,
:bisect_round_started, :bisect_individual_run_complete,
:bisect_complete, :bisect_repro_command,
:bisect_failed, :bisect_aborted,
:bisect_round_ignoring_ids, :bisect_round_detected_multiple_culprits,
:bisect_dependency_check_started, :bisect_dependency_check_passed,
:bisect_dependency_check_failed
Copy link
Member

Choose a reason for hiding this comment

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

not sure I follow why all this goes away?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you answered your own question above, but for posterity: previously, this was a "real" formatter that used with the RSpec::Core::Reporter, which required we register the formatter with all the events it subscribes to.

Now that we are no longer using the reporter to publish events to the formatter during bisection runs, we don't have to register all these events. Instead, the new Notifier I wrote just checks respond_to? as a way to see if it can safely publish an event.

In general, we've learned that respond_to? isn't a good approach with 3rd-party formatters (particularly when paired with common event names like start or close that could be meant for some entirely different purpose, or monkey patched onto all objects by a 3rd-party gem, etc...), but when we entirely control the formatter, as we do here, respond_to? is simple and efficient.


def bisect_starting(notification)
@round_count = 0
options = notification.original_cli_args.join(' ')
Expand Down Expand Up @@ -88,13 +79,10 @@ def bisect_aborted(notification)
end

# @private
# Produces detailed debug output while bisecting. Used when
# bisect is performed while the `DEBUG_RSPEC_BISECT` ENV var is used.
# Designed to provide details for us when we need to troubleshoot bisect bugs.
# Produces detailed debug output while bisecting. Used when bisect is
# performed with `--bisect=verbose`. Designed to provide details for
# us when we need to troubleshoot bisect bugs.
class BisectDebugFormatter < BisectProgressFormatter
Formatters.register self, :bisect_original_run_complete, :bisect_individual_run_start,
:bisect_individual_run_complete, :bisect_round_ignoring_ids

def bisect_original_run_complete(notification)
output.puts " (#{Helpers.format_duration(notification.duration)})"

Expand Down
10 changes: 7 additions & 3 deletions lib/rspec/core/invocations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ def call(options, _err, _out)

success = RSpec::Core::Bisect::Coordinator.bisect_with(
options.args,
RSpec.configuration,
bisect_formatter_for(options.options[:bisect])
)

Expand All @@ -41,8 +40,13 @@ def call(options, _err, _out)
private

def bisect_formatter_for(argument)
return Formatters::BisectDebugFormatter if argument == "verbose"
Formatters::BisectProgressFormatter
klass = if argument == "verbose"
Formatters::BisectDebugFormatter
else
Formatters::BisectProgressFormatter
end

klass.new(RSpec.configuration.output_stream)
end
end

Expand Down
3 changes: 1 addition & 2 deletions spec/rspec/core/bisect/coordinator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ def find_minimal_repro(output, formatter=Formatters::BisectProgressFormatter)
allow(Bisect::Server).to receive(:run).and_yield(instance_double(Bisect::Server))
allow(Bisect::ShellRunner).to receive(:new).and_return(fake_runner)

RSpec.configuration.output_stream = output
Bisect::Coordinator.bisect_with([], RSpec.configuration, formatter)
Bisect::Coordinator.bisect_with([], formatter.new(output))
ensure
RSpec.reset # so that RSpec.configuration.output_stream isn't closed
end
Expand Down
26 changes: 26 additions & 0 deletions spec/rspec/core/bisect/utilities_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'rspec/core/bisect/utilities'

module RSpec::Core
RSpec.describe Bisect::Notifier do
class ExampleFormatterClass
def foo(notification); end
end

let(:formatter) { instance_spy(ExampleFormatterClass) }
let(:notifier) { Bisect::Notifier.new(formatter) }

it 'publishes events to the wrapped formatter' do
notifier.publish :foo, :length => 15, :width => 12

expect(formatter).to have_received(:foo).with(an_object_having_attributes(
:length => 15, :width => 12
))
end

it 'does not publish events the formatter does not recognize' do
expect {
notifier.publish :unrecognized_event, :length => 15, :width => 12
}.not_to raise_error
end
end
end
6 changes: 2 additions & 4 deletions spec/rspec/core/invocations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,7 @@ def run_invocation

expect(RSpec::Core::Bisect::Coordinator).to have_received(:bisect_with).with(
args,
RSpec.configuration,
Formatters::BisectProgressFormatter
an_instance_of(Formatters::BisectProgressFormatter)
)
end

Expand Down Expand Up @@ -123,8 +122,7 @@ def run_invocation

expect(RSpec::Core::Bisect::Coordinator).to have_received(:bisect_with).with(
args,
RSpec.configuration,
Formatters::BisectDebugFormatter
an_instance_of(Formatters::BisectDebugFormatter)
)
end
end
Expand Down