Support bisect in spec opts #2271

urbanautomaton · 2016-06-10T22:38:54Z

I'm breaking out a new PR to fix #2231, based on discussion in #2259.

To summarise progress: the infinite looping problem is solved by modifying the environment given to the child processes, so that only the parent process starts a bisection.

However, the existing invocation of Bisect::Runner from within OptionParser is problematic, because OptionParser is invoked twice: once for CLI args, and once for args set in the environment. We need to invoke the bisection only once all args have been parsed, so that the correct commands can be issued to the child processes.

@myronmarston has suggested we refactor the OptionParser so it no longer invokes any actions directly, but instead returns a callable that the main runner can invoke. The current diff contains a sketch implementation of what I understand this to mean.

@myronmarston - is this sort of thing what you envisaged? (Obviously there'd be further refactoring to do, I just wanted to check I've got the right general idea...)

myronmarston · 2016-06-10T23:23:16Z

Hey @urbanautomaton, thanks for taking a stab at this. It definitely looks like a good start.

I think the refactoring of option parsing can be a self-contained PR. What do you think about opening a PR for just that so we can get it merged quick? Then you can deal with the bisect issues in a PR on top of that.

I'll go leave a few comments about the option parsing stuff on this PR now as well.

myronmarston · 2016-06-10T23:24:48Z

lib/rspec/core/configuration_options.rb

@@ -13,6 +13,10 @@ def initialize(args)
        organize_options
      end

+      def command_line_and_env_args


It's not clear to me why we need a method containing both command line and env args but not args from the dot files. Can you explain?

It's because I was transferring all the env args to the CLI args of the child processes and removing SPEC_OPTS completely for the children, but there's actually no need to do that, so this can be removed.

urbanautomaton · 2016-06-11T13:01:36Z

@myronmarston Thanks for the feedback. Responding inline here so the convo doesn't get lost when the diff changes:

options[:runner] = -> (opts, err, out) do
  initialize_project_and_exit
end
Could this be options[:runner] = method(:initialize_project_and_exit)? The method would have to take the args, but I think that's ok.

I had a stab at this, but ran into difficulties when porting #print_help_and_exit, because it requires extra information that's local to the parser, and not accessible via the options passed to the callable.

I've now tried something similar to what you suggest, but with factory methods returning lambdas that can capture any information necessary for the particular callable. What do you think?

myronmarston · 2016-06-11T16:03:16Z

lib/rspec/core/bisect/runner.rb

@@ -89,6 +89,10 @@ def run_command(cmd)
          # :nocov:
        end

+        def spec_opts_without_bisect
+          { 'SPEC_OPTS' => ENV.fetch('SPEC_OPTS', '').gsub(/--bisect/, '') }


What if the user has put --bisect=verbose in SPEC_OPTS? This gsub will leave =verbose.

myronmarston · 2016-06-11T16:08:26Z

I've now tried something similar to what you suggest, but with factory methods returning lambdas that can capture any information necessary for the particular callable. What do you think?

👍

urbanautomaton · 2016-06-13T20:29:44Z

I've addressed the feedback so far, and replaced the cuke with assertions at the Bisect::Runner unit test level. I think I'm happy with that, and don't feel like integration-level testing is necessary.

For JRuby and 1.8.7, Open3.popen3 doesn't support a passed environment. I've fallen back to modifying-and-restoring the env of the parent process in this case; I felt this was less error-prone than trying to build a command string with environment variables in it. I'd appreciate feedback on whether the assertions this necessitates are too complex as a result.

At the moment I'm testing the runner callables as part of the OptionParser tests, and have consolidated these tests there (moving some tests of the DRbRunner invocation as appropriate). Does this seem reasonable? I'm half-tempted to extract the callables to their own file and test them directly - would renaming the module something like RSpec::Core::Invocations make sense? (I would suggest Runners, but this name is pretty taken.)

The suite is almost totally green, but for a single warning generated by the 2.2 job. This job is running 2.2.0-p0, and the warning seems to be in error, as it doesn't appear in later versions of the 2.2.x branch. Is testing against 2.2.0-p0 deliberate, or could this job be updated to 2.2.5? If the former, I'd love a pointer as to how to address this warning, because I can't see anything wrong with the code that's triggering it.

myronmarston · 2016-06-14T00:00:02Z

The suite is almost totally green, but for a single warning generated by the 2.2 job. This job is running 2.2.0-p0, and the warning seems to be in error, as it doesn't appear in later versions of the 2.2.x branch. Is testing against 2.2.0-p0 deliberate, or could this job be updated to 2.2.5? If the former, I'd love a pointer as to how to address this warning, because I can't see anything wrong with the code that's triggering it.

This was a warning added to 2.2.0:

https://bugs.ruby-lang.org/issues/10661

Ruby-core removed the warning when I raised that issue, and it got removed from 2.2.1. (Unfortunately, it was added at the last minute to 2.2.0 and so it was never part of any of the RCs so I could notice and comment then).

To fix the warning on 2.2.0, you'll need to apply one of these fixes:

Rename the args local variable from earlier in the file
Rename the let(:args)
Change the calls to the args method (defined by the let) so that they are obviously method calls and not variables -- e.g. args() or self.args.

All the fixes for this are sub-optimal and the warning is annoying (it's why I raised the issue on ruby-core when I first noticed).

Is testing against 2.2.0-p0 deliberate, or could this job be updated to 2.2.5?

Testing against 2.2.0-p0 is not deliberate. I thought that are use of 2.2 in .travis.yml would cause travis to pick the latest available version of 2.2 (something like 2.2.5) but that does not work. So I think it's worth changing it to 2.2.5 in .travis.yml so we don't have to deal with the annoying warning anymore (it has come up multiple times, unfortunately).

myronmarston · 2016-06-14T00:04:39Z

spec/rspec/core/bisect/runner_spec.rb

+            expect(Open3).to have_received(open3_method).with(command)
+            expect(called_environment).to include(environment)
+          end
+        end


If open3_method is set to something besides capture2e, the match logic does nothing, which could lead to a silent failure. I think I'd prefer to see this as:

if open3_method == :capture2e RSpec::Matchers.define :invoke_command_with_env do |command, environment| # ... end elsif open3_method == :popen3 RSpec::Matchers.define :invoke_command_with_env do |command, environment| # ... end end

Then, if a different method is used, the matcher won't even be defined and we'll definitely get an error in that case.

myronmarston · 2016-06-14T00:17:26Z

👍 Looking good, @urbanautomaton! We are expecting to release 3.5 in the next few days (we plan to release as soon as rails 5 final ships, and we hear it should be this week), so I'm hoping we can get this merged before 3.5 ships :).

myronmarston · 2016-06-14T02:00:10Z

At the moment I'm testing the runner callables as part of the OptionParser tests, and have consolidated these tests there (moving some tests of the DRbRunner invocation as appropriate). Does this seem reasonable? I'm half-tempted to extract the callables to their own file and test them directly - would renaming the module something like RSpec::Core::Invocations make sense? (I would suggest Runners, but this name is pretty taken.)

What you've done is fine. Your idea to take it a step further with RSpec::Core::Invocations sounds like a nice further improvement. I don't consider it a pre-requisite to merge this PR but if you want to do that, please take a stab at it!

bf4 · 2016-06-14T04:04:46Z

lib/rspec/core/bisect/runner.rb

          end
        else # for 1.8.7
          # :nocov:
          def run_command(cmd)
            out = err = nil

+            original_spec_opts = ENV['SPEC_OPTS']
+            ENV['SPEC_OPTS'] = spec_opts_without_bisect
+


👍 did you try using the open3 feature of passing in the env opts?

The parameters env, cmd, and opts are passed to Process.spawn

env: hash
name => val : set the environment variable
name => nil : unset the environment variable
options: hash
clearing environment variables:
:unsetenv_others => true : clear environment variables except specified by env
:unsetenv_others => false : don't clear (default)
If a hash is given as env, the environment is updated by env before exec(2) in the child process. If a pair in env has nil as the value, the variable is deleted.

set FOO as BAR and unset BAZ.
pid = spawn({"FOO"=>"BAR", "BAZ"=>nil}, command)

The :unsetenv_others key in options specifies to clear environment variables, other than specified by env.

pid = spawn(command, :unsetenv_others=>true) # no environment variable
pid = spawn({"FOO"=>"BAR"}, command, :unsetenv_others=>true) # FOO only

The upside of supporting ruby 1.8.7 is that I get to use the old hash syntax without feeling guilt. The downside is that some of stdlib wasn't as capable back then, and Open3/Process.spawn is one of those bits, unfortunately. I've used the new passed environment where possible, but for 1.8.7 it's either put the new env in the command string or mutate ENV, so I plumped for the latter.

urbanautomaton · 2016-06-14T12:40:00Z

Your idea to take it a step further with RSpec::Core::Invocations sounds like a nice further improvement. I don't consider it a pre-requisite to merge this PR but if you want to do that, please take a stab at it!

Okay - given the imminent release, then, I'd like to make that change but will tackle it in a separate PR.

I think the build should be passing again now (it's being very slow kicking off for some reason). I'm still not entirely happy with the Open3 matchers (the way I've got it accessing the called_environment seems wrong somehow), but apart from that I think I'm about done. If there's no further feedback I'll rebase the commits into a better story and then we'll hopefully be ready to go.

myronmarston · 2016-06-14T15:34:09Z

spec/rspec/core/bisect/runner_spec.rb

+            a_string_including("--seed 1234"),
+            { 'SPEC_OPTS' => '--fail-fast' }
+          )
+        end


I see what you mean about it being tricky to figure out what to treat as the subject for these expectations. Maybe the block approach does make the most sense. I'm happy to merge with this has it is but if you want to switch back to the block form that would be fine, too.

myronmarston · 2016-06-14T15:38:53Z

I think the build should be passing again now (it's being very slow kicking off for some reason).

I think Travis is starting it now. Must have just gotten backed up for some reason.

If there's no further feedback I'll rebase the commits into a better story and then we'll hopefully be ready to go.

I left the one comment above about switching back to a block perhaps making more sense and being OK, but whichever approach you decide to take there is fine. Regardless, this LGTM. I'll be happy to merge after you've rebased into a clearer story as you said :).

myronmarston · 2016-06-14T16:05:25Z

The build is green now, FWIW :).

urbanautomaton · 2016-06-14T19:55:04Z

Okay - I went back to the block form for the Open3 assertions and have rebased the branch into something more coherent.

The Invocations extraction turned out pretty easy, so I've done that too (but will happily revert and separately PR it if that commit contains anything controversial at all). Last question: I've marked that whole module @private for documentation purposes - is that okay? I wondered whether I should be documenting the module and marking the invocation methods @private instead. I don't think it's something any user is likely to access directly, though.

myronmarston · 2016-06-14T19:57:33Z

spec/rspec/core/bisect/runner_spec.rb

+
+          supports_block_expectations
+        end
+      end


Not a merge blocker, but I think maybe the matcher should be named invoke_command_with_env instead of the past-tense have_invoked_command_with_env. Consider that all our other block matchers are named with the present tense form (e.g. change over have_changed, raise_error over have_raised_error, etc).

Oh - yeah, that's an artifact of switching to the non-block style and back again, will change it now.

Currently if RSpec is invoked with SPEC_OPTS="--bisect", an infinite number of child processes is spawned. This is because the bisect feature works by spawning child processes for each bisection run. In the normal cli usage case, the --bisect argument is removed before the child processes are invoked. It is not removed from the SPEC_OPTS variable, however, meaning each child process believes it's starting a new bisection. This commit allows Bisect::Runner to invoke its child processes with a modified environment, removing --bisect if present. In modern rubies we can use Open3.capture2e's optional environment argument - in older rubies and JRuby we have to fall back on modifying the parent process's environment, then restoring it after the child process has run. Previously the Bisect::Runner was invoked during options parsing. However, because SPEC_OPTS and the cli args are parsed by separate instances of the parser, invoking the bisection during SPEC_OPTS parsing meant the cli args were lost. To address this I've made the OptionParser set a :bisect option, which is then picked up in the main Runner to invoke the actual bisection. This has increased the complexity of Runner.run, and there are other invocations remaining in the OptionParser (e.g. --init and --help). These will be refactored in a follow-up commit, so that the OptionParser sets a callable in each case that can be called in the main Runner.

@param

RSpec can be invoked with a number of CLI flags that result in substantially different code paths. As well as the default runner, there is: * `--init` - initializes the project directory and exits * `--help` - prints CLI usage and exits * `--version` - prints the version and exits * `--drb` - runs examples via a DRb connection * `--bisect` - starts a bisection process Previously some of these paths were invoked directly in the OptionParser (--init, --help and --version), while others were invoked by the main Runner.run call. This necessitated a lot of complexity in Runner.run, and a reasonable amount of stubbing of exit calls (which can be error-prone, and lead to a prematurely-terminated test run). This commit refactors the different invocations into callables, which the OptionParser sets as the `:runner` option. If an alternative invocation is found in Runner.run, it is used, falling back to the default of instantiating a Runner and starting it. The invocation callables do not handle exiting themselves, but are required to return an exit code instead. They must all accept the following arguments: @param options [RSpec::Core::ConfigurationOptions] @param err [IO] @param out [IO] At the moment the invocations are tested via the OptionParser specs - in future work we intend to extract these to their own module and test them independently.

This method is now occasionally (but not reliably) failing the PerceivedComplexity cop. Since the complexity of this method is hard to control and the CyclomaticComplexity rule is already disabled for it, I'm turning off the cop for this method only.

Previously, this was set to `2.2` in the expectation that travis would use the latest of the 2.2.x branch as a result. However, builds appear to be running with 2.2.0-p0, which contains erroneous warnings that can force variables and lets to be needlessly renamed. While this may be a travis bug, I'm explicitly setting the version in the configuration to ensure that an up-to-date version is picked up in the meanwhile.

myronmarston · 2016-06-14T20:04:47Z

spec/rspec/core/option_parser_spec.rb

+
+      it "sets the `:runner` option with the `bisect` invocation" do
+        bisect = double(:bisect)
+        allow(RSpec::Core::Invocations).to receive(:bisect).and_return(bisect)


Stubbing invocations to return a double just so we can assert on equality below seems pretty sub-optimal. As I see it, the RSpec::Core::Invocations methods are essentially just exposing constants -- given that they take no arguments. So WDYT about turning them into constants, and then you don't have to stub here -- instead, down below it can do:

expect(options[:runner]).to eq(RSpec::Core::Invocations::BISECT)

Thoughts?

The downside there is it doesn't let you capture extra info in the callable (as required by .print_help). I do agree though, I don't particularly like the stubbing. Maybe we could turn them into callable objects?

module Invocations PrintHelp = Struct.new(:parser, :invalid_options) do def call(options, err, out) #... end end end

which would let us assert:

expect(options[:runner]).to be_instance_of(RSpec::Core::Invocations::PrintHelp)

The downside there is it doesn't let you capture extra info in the callable (as required by .print_help).

I missed that little detail. Thanks for pointing it out!

Maybe we could turn them into callable objects?

👍

I like this approach a lot.

myronmarston · 2016-06-14T20:44:18Z

👍 LGTM. I plan to merge when the build goes green.

Previous work introduced a set of invocation callables that could be set in the OptionParser to run instead of the default RSpec::Core::Runner. Testing them via the the OptionParser seemed out-of-place, however, and made the test stimulus needlessly complex. This change extracts the invocations to a set of callable objects defined in the RSpec::Core::Invocations namespace, allowing them to be tested separately from the OptionParser.

urbanautomaton · 2016-06-14T21:55:05Z

Missed out the @private declarations on the callable classes - patched and now it's green! 🎉

myronmarston · 2016-06-14T21:58:07Z

Thanks, @urbanautomaton!

[ci skip]

myronmarston · 2016-06-14T22:00:14Z

As always, thanks for the great work, @urbanautomaton :).

urbanautomaton · 2016-06-14T22:05:02Z

Thanks, and a pleasure as always. 😄

…pec-opts Support bisect in spec opts

[ci skip]

urbanautomaton force-pushed the support-bisect-in-spec-opts branch 2 times, most recently from 6ee83a5 to c2ca932 Compare June 10, 2016 22:46

myronmarston reviewed Jun 10, 2016
View reviewed changes

urbanautomaton force-pushed the support-bisect-in-spec-opts branch from c2ca932 to 68398d6 Compare June 11, 2016 12:59

urbanautomaton force-pushed the support-bisect-in-spec-opts branch from 68398d6 to 1c5818d Compare June 11, 2016 13:06

myronmarston reviewed Jun 11, 2016
View reviewed changes

urbanautomaton force-pushed the support-bisect-in-spec-opts branch 3 times, most recently from de0e370 to 21ed282 Compare June 13, 2016 19:48

myronmarston reviewed Jun 14, 2016
View reviewed changes

myronmarston mentioned this pull request Jun 14, 2016

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

Closed

bf4 reviewed Jun 14, 2016
View reviewed changes

urbanautomaton force-pushed the support-bisect-in-spec-opts branch from f2db0e3 to 6baac52 Compare June 14, 2016 11:43

myronmarston reviewed Jun 14, 2016
View reviewed changes

urbanautomaton force-pushed the support-bisect-in-spec-opts branch 2 times, most recently from 095aaf5 to 40154ab Compare June 14, 2016 19:39

myronmarston reviewed Jun 14, 2016
View reviewed changes

urbanautomaton added 4 commits June 14, 2016 20:59

urbanautomaton force-pushed the support-bisect-in-spec-opts branch from 40154ab to ff0e080 Compare June 14, 2016 20:01

myronmarston reviewed Jun 14, 2016
View reviewed changes

urbanautomaton force-pushed the support-bisect-in-spec-opts branch 2 times, most recently from 4efce80 to f0e5391 Compare June 14, 2016 20:39

urbanautomaton force-pushed the support-bisect-in-spec-opts branch from f0e5391 to d52c969 Compare June 14, 2016 21:29

myronmarston merged commit 97a0cdb into rspec:master Jun 14, 2016

myronmarston added a commit that referenced this pull request Jun 14, 2016

Add changelog for #2271.

2edb982

[ci skip]

JonRowe mentioned this pull request Jun 15, 2016

Add Cucumber features for new RespondTo matcher methods. rspec/rspec-expectations#921

Merged

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Merge pull request rspec#2271 from urbanautomaton/support-bisect-in-s…

a4e22bf

…pec-opts Support bisect in spec opts

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Add changelog for rspec#2271.

8bda7e8

[ci skip]

Uh oh!

Support bisect in spec opts #2271

Support bisect in spec opts #2271

Uh oh!

Conversation

urbanautomaton commented Jun 10, 2016

Uh oh!

myronmarston commented Jun 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urbanautomaton commented Jun 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Jun 11, 2016

Uh oh!

urbanautomaton commented Jun 13, 2016

Uh oh!

myronmarston commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Jun 14, 2016

Uh oh!

myronmarston commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urbanautomaton commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Jun 14, 2016

Uh oh!

myronmarston commented Jun 14, 2016

Uh oh!

urbanautomaton commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Jun 14, 2016

Uh oh!

urbanautomaton commented Jun 14, 2016

Uh oh!

myronmarston commented Jun 14, 2016

Uh oh!

myronmarston commented Jun 14, 2016

Uh oh!

urbanautomaton commented Jun 14, 2016

Uh oh!

Uh oh!