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

WIP: Removes bisect from SPEC_OPTS to prevent infinite recursion #2259

Closed
wants to merge 2 commits into from
Closed

WIP: Removes bisect from SPEC_OPTS to prevent infinite recursion #2259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc3cff2
Test permanent removal of bisect from SPEC_OPTS
bf4 Jun 3, 2016
a28aaf1
wip
bf4 Jun 3, 2016
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
16 changes: 14 additions & 2 deletions lib/rspec/core/bisect/runner.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,18 @@ module Bisect
# the given bisect server to capture the results.
# @private
class Runner
attr_reader :original_cli_args
attr_reader :original_cli_args, :original_spec_opts, :spec_opts

def initialize(server, original_cli_args)
@server = server
@original_cli_args = original_cli_args.reject { |arg| arg.start_with?("--bisect") }
@original_spec_opts = ENV['SPEC_OPTS']
if original_spec_opts =~ /--bisect/
bisect_arg = Shellwords.split(ENV["SPEC_OPTS"]).flatten.grep(/--bisect/).shift
Copy link
Member

Choose a reason for hiding this comment

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

  • IMO, using shift here is very odd. shift is like first (a query) combined with drop(1) (a command) -- but you are only using it for the query part (since you keep no reference to the array being mutated), so I'd prefer to see first be used.
  • Is flatten necessary? I don't see how Shellwords.split can result in an array that is not already flattened.
  • Using grep(...).shift (or grep(...).first) causes the entire list to be traversed, even if --bisect is one of the first arguments. Instead, I'd use find { ... }.
  • You are basically looking for --bisect in the spec opts multiple times: once in the conditional on the line above, and again on this line. That's wasted work.
  • Actually, I'm not sure a conditional is even needed...

Putting all that together, I think this section can be simplified to:

ENV["SPEC_OPTS"] = Shellwords.split(ENV.fetch("SPEC_OPTS", "")).reject { |opt| opt =~ /--bisect/ }.shelljoin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flatten came from from ConfigurationOptions iirc, where I couldn't figure out the right way to reuse that code.

Agree with your comments

B mobile phone

On Jun 4, 2016, at 12:45 PM, Myron Marston [email protected] wrote:

In lib/rspec/core/bisect/runner.rb:

     def initialize(server, original_cli_args)
       @server            = server
       @original_cli_args = original_cli_args.reject { |arg| arg.start_with?("--bisect") }

  •      @original_spec_opts = ENV['SPEC_OPTS']

  •      if original_spec_opts =~ /--bisect/

  •        bisect_arg = Shellwords.split(ENV["SPEC_OPTS"]).flatten.grep(/--bisect/).shift
    IMO, using shift here is very odd. shift is like first (a query) combined with drop(1) (a command) -- but you are only using it for the query part (since you keep no reference to the array being mutated), so I'd prefer to see first be used.
    Is flatten necessary? I don't see how Shellwords.split can result in an array that is not already flattened.
    Using grep(...).shift (or grep(...).first) causes the entire list to be traversed, even if --bisect is one of the first arguments. Instead, I'd use find { ... }.
    You are basically looking for --bisect in the spec opts multiple times: once in the conditional on the line above, and again on this line. That's wasted work.
    Actually, I'm not sure a conditional is even needed...
    Putting all that together, I think this section can be simplified to:

ENV["SPEC_OPTS"] = Shellwords.split(ENV.fetch("SPEC_OPTS", "")).reject { |opt| opt =~ /--bisect/ }.shelljoin
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

# mutate spec_opts
ENV['SPEC_OPTS'] = ENV['SPEC_OPTS'].sub(bisect_arg, '')
end
@spec_opts = ENV['SPEC_OPTS']
end

def run(locations)
Expand Down Expand Up @@ -67,17 +74,22 @@ def run_locations(*capture_args)
end
end

# Consider just using Process.spawn since Open3 uses that.
Copy link
Member

Choose a reason for hiding this comment

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

Did you try using Process.spawn? Is there a reason you can't use that instead of mutating ENV['SPEC_OPTS']?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, was my first attempt but a lot broke, so I revised strategy to look for how I could test it... In the end, decided to make a pr to let you see what I am thinking and trying, and if you have any ideas which is right place to test....

Thanks for looking

Because I'm testing bisect, I can't run the results of a bisect on any failures :)

B mobile phone

On Jun 4, 2016, at 12:37 PM, Myron Marston [email protected] wrote:

In lib/rspec/core/bisect/runner.rb:

@@ -67,17 +74,22 @@ def run_locations(*capture_args)
end
end

  •    # Consider just using Process.spawn since Open3 uses that.
    Did you try using Process.spawn? Is there a reason you can't use that instead of mutating ENV['SPEC_OPTS']?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

# `Open3.capture2e` does not work on JRuby:
# https://github.com/jruby/jruby/issues/2766
if Open3.respond_to?(:capture2e) && !RSpec::Support::Ruby.jruby?
def run_command(cmd)
# consider passing in { 'SPEC_OPTS' => spec_opts } when not spec_opts.empty?
# instead of mutating SPEC_OPTS
Copy link
Member

Choose a reason for hiding this comment

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

I could do neither, have you got this working?

Open3.capture2e(cmd).first
end
else # for 1.8.7
else # for 1.8.7 and JRuby
# :nocov:
def run_command(cmd)
out = err = nil

# consider passing in { 'SPEC_OPTS' => spec_opts } when not spec_opts.empty?
# instead of mutating SPEC_OPTS
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this can't be done now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't work first time and didn't think till right now that I can test effect of setting any env vars for arbitrary command, not just adding the hash and watching tests fail....

B mobile phone

On Jun 4, 2016, at 12:46 PM, Myron Marston [email protected] wrote:

In lib/rspec/core/bisect/runner.rb:

       # :nocov:
       def run_command(cmd)
         out = err = nil

  •        # consider passing in { 'SPEC_OPTS' => spec_opts } when not spec_opts.empty?

  •        # instead of mutating SPEC_OPTS
    Any reason this can't be done now?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Open3.popen3(cmd) do |_, stdout, stderr|
# Reading the streams blocks until the process is complete
out = stdout.read
Expand Down
2 changes: 2 additions & 0 deletions lib/rspec/core/option_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,8 @@ def initialize_project_and_exit
def bisect_and_exit(argument)
RSpec::Support.require_rspec_core "bisect/coordinator"

# This might be the best spot to mutate ENV['SPEC_OPTS']
# if we do that.
Copy link
Contributor Author

@bf4 bf4 Jun 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't find an obvious spot in the ConfigurationOptions nor Configuration where this would go and then be available to the bisect runner, but if I check the env here at cli parsing and just pass that through the coordinator, that might be ok

Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious to me what's better about this spot then the spot you already have it. Can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm reading code correctly, I can handle the env vars here, or multiple times in each runner.

B mobile phone

On Jun 4, 2016, at 12:47 PM, Myron Marston [email protected] wrote:

In lib/rspec/core/option_parser.rb:

@@ -291,6 +291,8 @@ def initialize_project_and_exit
def bisect_and_exit(argument)
RSpec::Support.require_rspec_core "bisect/coordinator"

  •  # This might be the best spot to mutate ENV['SPEC_OPTS']

  •  # if we do that.
    It's not obvious to me what's better about this spot then the spot you already have it. Can you explain?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Copy link
Member

Choose a reason for hiding this comment

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

Only a single Runner instance is created for the entirety of the bisect run, so it's only going to happen once, regardless. IMO, the logic (if we take this approach) belongs better in Runner than here.

Copy link
Member

Choose a reason for hiding this comment

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

Surely a single instance per process...

Copy link
Member

Choose a reason for hiding this comment

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

Surely a single instance per process...

A Bisect::Runner instance is only created in the main bisect process, not in the processes created to run subsets of the suite. So yes, a single instance per main bisect process, but there's only one main bisect process for an entire --bisect run.

It's made here in the coordinator:

rspec-core/lib/rspec/core/bisect/coordinator.rb

Line 33 in cc869c8

runner = Runner.new(server, @original_cli_args)

...and is then used repeatedly in the example minimizer to run specific example subsets:

rspec-core/lib/rspec/core/bisect/example_minimizer.rb

Line 142 in cc869c8

results, duration = track_duration { runner.run(ids_to_run) }

success = Bisect::Coordinator.bisect_with(
original_args,
RSpec.configuration,
Expand Down
9 changes: 9 additions & 0 deletions spec/rspec/core/bisect/runner_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,15 @@ def command_for(locations, options={})
end
end

%w[ --bisect --bisect=verbose --bisect=blah ].each do |value|
it "ignores SPEC_OPTS='#{value}' option since that would infinitely recurse" do
with_env_vars 'SPEC_OPTS' => value do
expect(runner.original_spec_opts).to eq(value)
expect(runner.spec_opts).to eq('')
end
end
end

it 'uses the bisect formatter' do
cmd = command_for([])
expect(cmd).to include("--format bisect")
Expand Down