WIP: Removes bisect from SPEC_OPTS to prevent infinite recursion

[bf4]

Fixes #2231

• Test permanent removal of bisect from SPEC_OPTS

[bf4]

@myronmarston any thoughts on this appreciated. Basically, I'm not sure how to test this in the test suite because of how bisect shells out (I can't set it in the SPEC_OPTS :) and I'm not sure bundling from github and experience-testing is sufficient

[bf4]

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

[myronmarston]

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

[bf4]

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.

[myronmarston]

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.

[JonRowe]

Surely a single instance per process...

[myronmarston]

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) }

[myronmarston]

This looks like a good start. Have you tried it to confirm this actually fixes the problem? From what @JonRowe and @urbanautomaton said in #2231 it sounds like they attempted a solution like this and it didn't work even though it seems like it should.

BTW, as for writing an integration test for this: we have two places for integration tests:

• The cuke -- this is our main end-to-end test and also serves as user-facing documentation.
• An integration spec -- we use this to integration test things that can't be covered with a simple unit test and that don't serve a useful documentation purpose (and therefore should not be in the cuke).

For this, an integration test would best belong in the latter.

I'm not sure bundling from github and experience-testing is sufficient

What does "bundling from github" and "experience-testing" mean?

[JonRowe]

What does "bundling from github" and "experience-testing" mean?

I'm assuming that this means using RSpec as a git dependency and running yourself on the command line.

BTW my pet theory for why it doesn't work, is that when Ruby spawns / forks new processes it somehow just uses the parent ENV rather than the "in ruby" environment.

[bf4]

I'll investigate further in the near future. Specifically what it means to pass in an empty hash.

Any thoughts on what to name this 'env spec opts without bisect' value?

I think we've agreed the code for processing spec opts is in the bisect runner.

I think I'll write tests that pass a non-bisect command in to confirm env behavior

Eg

with_env 'RSPEC_TEMP' => 'original' do
actual = execute "ruby -e 'puts ENV['RSPEC_TEMP']"
expect(actual).to eq('original')
end

Etc

Thanks for your interest in this.

[urbanautomaton]

I've had another quick go at this too, and now have a branch that successfully bisects with SPEC_OPTS="--bisect" - you can see my diff here.

It turns out fixing the ENV is only part of the problem. The second issue is that bisection is currently invoked during options parsing, here:

# lib/rspec/core/option_parser.rb
parser.on('--bisect[=verbose]', 'Repeatedly runs the suite in order to isolate the failures to the ',
          '  smallest reproducible case.') do |argument|
  bisect_and_exit(argument)
end

Because the option parser is invoked twice (once for CLI args, once for ENV args), if --bisect is set in the ENV then the bisection is started with only the ENV args - any CLI args are lost. I've hacked around this to get my branch working as a proof of concept, but I don't know whether this is a reasonable direction to go in for a proper fix.

[myronmarston]

Because the option parser is invoked twice (once for CLI args, once for ENV args), if --bisect is set in the ENV then the bisection is started with only the ENV args - any CLI args are lost.

Interesting, I didn't realize the problems it was causing to perform alternate actions in the CLI parsing. The alternate action logic for --init was put there long ago and I just built off that without realizing the problems it could cause.

I think we can refactor the options parsing so that it returns a callable for alternate actions like bisection, --init setup, etc -- and then the caller will be responsible for invoking the callable if it's appropriate to do so in that context.

Thoughts?

[soulcutter]

@urbanautomaton I suspect you're on to something. Would it make sense to open a new PR for your branch to continue discussion of that implementation? Or were you considering having @bf4 take cues from your approach? It's not clear to me if your branch is solely for illustrative purposes.

https://github.com/urbanautomaton/rspec-core/blob/b7a49cab343e621bd6dccbf39e438d53fd4a43e6/lib/rspec/core/bisect/runner.rb#L92-L94 We shouldn't completely empty out SPEC_OPTS - we just need to remove the bisect option from it.

[soulcutter]

@myronmarston said:

I think we can refactor the options parsing so that it returns a callable for alternate actions like bisection, --init setup, etc -- and then the caller will be responsible for invoking the callable if it's appropriate to do so in that context.

It does make sense to defer bisect - putting it into options as a callable seems like a reasonable way to do that.

It looks like --init is a special case that doesn't care about any other CLI/env args. Same with --version. Nothing else stands out to me as an action that needs to be deferred.

[urbanautomaton]

@myronmarston: I'm not sure I follow, sorry. Would the OptionParser return the callable itself, or would it be the responsibility of ConfigurationOptions?

@soulcutter: I'm easy - I'm happy to open a PR for my branch if @bf4 doesn't mind.

https://github.com/urbanautomaton/rspec-core/blob/b7a49cab343e621bd6dccbf39e438d53fd4a43e6/lib/rspec/core/bisect/runner.rb#L92-L94 We shouldn't completely empty out SPEC_OPTS - we just need to remove the bisect option from it.

With the approach I've taken, any args originally specified in SPEC_OPTS will have been transferred to CLI args for the child process (the merge happens here), so to avoid repeated options I did need to empty SPEC_OPTS completely.

[myronmarston]

It looks like --init is a special case that doesn't care about any other CLI/env args. Same with --version. Nothing else stands out to me as an action that needs to be deferred.

While it has worked pretty OK to put the --version and --init actions in option parsing, I think it makes the option parsing harder to reason about if it is responsible for both parsing options and, in some cases (but not all) performing an action based on what was parsed. IMO, it would be easier to reason about if the option parser was side-effect free and just parsed options, leaving it up to the caller to invoke the callable if desired.

@myronmarston: I'm not sure I follow, sorry. Would the OptionParser return the callable itself, or would it be the responsibility of ConfigurationOptions?

I think the option parser would return the callable.

[bf4]

If you're about to write code to fix this, by all means, go with my blessing! Y'all are obviously paying more attention to it at present than I am :)

[bf4]

Can use result of option parser to lookup proper action for each option. The rest is design: hash of callables? Send a method?

[myronmarston]

Closing in favor of #2271.