Added option to disable all monkey patching #1465

Merged
merged 1 commit into from Apr 3, 2014

Conversation

Projects
None yet
4 participants
@waterlink
Contributor

waterlink commented Mar 30, 2014

Related to #1433

  • Add config option to disable all monkey patching
  • Fix for 1.8.7
  • Add doc-comments for introduced methods
  • Add cucumber scenarios
  • Move specs from disable_monkey_patching_spec.rb to configuration_spec.rb
  • Fix feature to support 1.8.7 syntax
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 30, 2014

Member

Thanks for getting this rolling, @waterlink. I don't have time to review at the moment but plan to do a review later tonight (or tomorrow if I don't get around to i tonight).

Member

myronmarston commented Mar 30, 2014

Thanks for getting this rolling, @waterlink. I don't have time to review at the moment but plan to do a review later tonight (or tomorrow if I don't get around to i tonight).

@waterlink

This comment has been minimized.

Show comment
Hide comment
@waterlink

waterlink Mar 30, 2014

Contributor

@myronmarston It is OK. Thank you too.

Contributor

waterlink commented Mar 30, 2014

@myronmarston It is OK. Thank you too.

lib/rspec/core/configuration.rb
+ end
+ end
+
+ def disable_monkey_patching_for_mocks_call

This comment has been minimized.

@xaviershay

xaviershay Mar 30, 2014

Member

inline this

@xaviershay

xaviershay Mar 30, 2014

Member

inline this

lib/rspec/core/configuration.rb
+ end
+ end
+
+ def disable_monkey_patching_for_expectations_call

This comment has been minimized.

@xaviershay

xaviershay Mar 30, 2014

Member

inline this

@xaviershay

xaviershay Mar 30, 2014

Member

inline this

lib/rspec/core/example_group.rb
@@ -400,8 +400,20 @@ def self.top_level?
# @private
def self.ensure_example_groups_are_configured
unless defined?(@@example_groups_configured)
- RSpec.configuration.configure_mock_framework
- RSpec.configuration.configure_expectation_framework
+ RSpec.configuration.tap do |rspec|

This comment has been minimized.

@xaviershay

xaviershay Mar 30, 2014

Member

s/rspec/config/, rspec is a confusing name for a variable.

@xaviershay

xaviershay Mar 30, 2014

Member

s/rspec/config/, rspec is a confusing name for a variable.

+ before { config.mock_with :rspec }
+
+ it 'calls disable_monkey_patching_for_mocks' do
+ expect(config).to receive(:disable_monkey_patching_for_mocks_call)

This comment has been minimized.

@xaviershay

xaviershay Mar 30, 2014

Member

oh I'm guessing that's why you didn't inline the method above :)

@xaviershay

xaviershay Mar 30, 2014

Member

oh I'm guessing that's why you didn't inline the method above :)

lib/rspec/core/example_group.rb
+ rspec.configure_expectation_framework
+
+ if rspec.mock_framework == RSpec::Core::MockingAdapters::RSpec &&
+ rspec.instance_variable_defined?(:@configure_mock_framework_block)

This comment has been minimized.

@xaviershay

xaviershay Mar 30, 2014

Member
  1. Put an accessor in front of this, so we don't need to reach into instance variables. Mark it @private in the method comment.
  2. With an accessor, you can return a default empty proc if it hasn't been sent, meaning you don't need any of these conditionals, you just always call configure_mock_framework_block.
@xaviershay

xaviershay Mar 30, 2014

Member
  1. Put an accessor in front of this, so we don't need to reach into instance variables. Mark it @private in the method comment.
  2. With an accessor, you can return a default empty proc if it hasn't been sent, meaning you don't need any of these conditionals, you just always call configure_mock_framework_block.
lib/rspec/core/configuration.rb
+ self.expose_dsl_globally = false
+
+ if @mock_framework == RSpec::Core::MockingAdapters::RSpec
+ disable_monkey_patching_for_mocks_call

This comment has been minimized.

@xaviershay

xaviershay Mar 30, 2014

Member

why do we have to call this explicitly here? Can't we just wait for ensure_example_groups_are_configured to do it below?

@xaviershay

xaviershay Mar 30, 2014

Member

why do we have to call this explicitly here? Can't we just wait for ensure_example_groups_are_configured to do it below?

+ end
+ end
+
+ describe 'disabling monkey patching for expectations' do

This comment has been minimized.

@xaviershay

xaviershay Mar 30, 2014

Member

these specs don't give me enough context to figure out why the behaviour is the way it is. I'd either change the wording, or add so comments to the code.

@xaviershay

xaviershay Mar 30, 2014

Member

these specs don't give me enough context to figure out why the behaviour is the way it is. I'd either change the wording, or add so comments to the code.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Mar 30, 2014

Member

This is a good start! I left some comments, I can't quite see the shape of this yet but I think your answers will help me :)

Member

xaviershay commented Mar 30, 2014

This is a good start! I left some comments, I can't quite see the shape of this yet but I think your answers will help me :)

@xaviershay xaviershay changed the title from 1433 Added option to disable all monkey patching to Added option to disable all monkey patching Mar 30, 2014

lib/rspec/core/configuration.rb
+ elsif @expectation_frameworks.empty?
+ self.configure_expectation_framework_block = disable_monkey_patching_for_expectations
+ end
+ end

This comment has been minimized.

@myronmarston

myronmarston Mar 31, 2014

Member

I think there's a simpler implementation that can do away with the extra methods necessitated by your solution here (and also with the slightly non-standard use of procs here).

def disable_monkey_patching!
  self.expose_dsl_globally = false
  @monkey_patching_disabled = true
  conditionally_disable_mocks_monkey_patching
  conditionally_disable_expectations_monkey_patching
end

def configure_mock_framework
  # ...whatever code was in this method before
  conditionally_disable_mocks_monkey_patching
end

def configure_expectation_framework
  # ...whatever code was in this method before
  conditionally_disable_expectations_monkey_patching
end

private

# These methods are idempotent and are always safe to call as they will do "the right thing"
def conditionally_disable_mocks_monkey_patching
  return unless @monkey_patching_disabled && defined?(RSpec::Mocks)

  RSpec::Mocks.configure do |config|
    config.syntax = :expect
    config.patch_marshal_to_support_partial_doubles = false
  end
end

def conditionally_disable_expectations_monkey_patching
  return unless @monkey_patching_disabled && defined?(RSpec::Expectations)

  RSpec::Expectations.configuration.syntax = :expect
end

Thoughts?

BTW, notice that my version conditionally does it based on if RSpec::Mocks or RSpec::Expectations are defined rather than based on the config settings. I think this is important, because if the user requires one of those gems manually but then doesn't configure them (or even configures rspec to use a different mocking library or expectation library) we should still disable the monkey patching they do since they're part of RSpec.

@myronmarston

myronmarston Mar 31, 2014

Member

I think there's a simpler implementation that can do away with the extra methods necessitated by your solution here (and also with the slightly non-standard use of procs here).

def disable_monkey_patching!
  self.expose_dsl_globally = false
  @monkey_patching_disabled = true
  conditionally_disable_mocks_monkey_patching
  conditionally_disable_expectations_monkey_patching
end

def configure_mock_framework
  # ...whatever code was in this method before
  conditionally_disable_mocks_monkey_patching
end

def configure_expectation_framework
  # ...whatever code was in this method before
  conditionally_disable_expectations_monkey_patching
end

private

# These methods are idempotent and are always safe to call as they will do "the right thing"
def conditionally_disable_mocks_monkey_patching
  return unless @monkey_patching_disabled && defined?(RSpec::Mocks)

  RSpec::Mocks.configure do |config|
    config.syntax = :expect
    config.patch_marshal_to_support_partial_doubles = false
  end
end

def conditionally_disable_expectations_monkey_patching
  return unless @monkey_patching_disabled && defined?(RSpec::Expectations)

  RSpec::Expectations.configuration.syntax = :expect
end

Thoughts?

BTW, notice that my version conditionally does it based on if RSpec::Mocks or RSpec::Expectations are defined rather than based on the config settings. I think this is important, because if the user requires one of those gems manually but then doesn't configure them (or even configures rspec to use a different mocking library or expectation library) we should still disable the monkey patching they do since they're part of RSpec.

This comment has been minimized.

@waterlink

waterlink Mar 31, 2014

Contributor

That is quite good. In this case there is no need to modify example_group.rb at all.

This line here:

config.patch_marshal_to_support_partial_doubles = false

against this line here #1433

mocks.patch_marshal_to_support_partial_doubles = true

I understand that the one with false is the right one (because it disables patching of Marshal).

@waterlink

waterlink Mar 31, 2014

Contributor

That is quite good. In this case there is no need to modify example_group.rb at all.

This line here:

config.patch_marshal_to_support_partial_doubles = false

against this line here #1433

mocks.patch_marshal_to_support_partial_doubles = true

I understand that the one with false is the right one (because it disables patching of Marshal).

This comment has been minimized.

@myronmarston

myronmarston Mar 31, 2014

Member

In this case there is no need to modify example_group.rb at all.

Yep, that's a nice side benefit :).

I understand that the one with false is the right one (because it disables patching of Marshal).

Yep. It was a typo in #1433.

@myronmarston

myronmarston Mar 31, 2014

Member

In this case there is no need to modify example_group.rb at all.

Yep, that's a nice side benefit :).

I understand that the one with false is the right one (because it disables patching of Marshal).

Yep. It was a typo in #1433.

This comment has been minimized.

@JonRowe

JonRowe Mar 31, 2014

Member

My 2¢ is I'd like to see less methods here, and also removal of the proc stuff that @xaviershay was commenting on.

@JonRowe

JonRowe Mar 31, 2014

Member

My 2¢ is I'd like to see less methods here, and also removal of the proc stuff that @xaviershay was commenting on.

@waterlink waterlink closed this Mar 31, 2014

@waterlink waterlink reopened this Mar 31, 2014

@waterlink

This comment has been minimized.

Show comment
Hide comment
@waterlink

waterlink Mar 31, 2014

Contributor

I hate this button Close within comment, that feels like just Cancel.
Travis build failed due to timeout of rubygems..

Contributor

waterlink commented Mar 31, 2014

I hate this button Close within comment, that feels like just Cancel.
Travis build failed due to timeout of rubygems..

@@ -0,0 +1,177 @@
+Feature: Zero monkey patching mode
+
+ Use the ```disable_monkey_patching!``` configuration option to disable all monkey patching done by RSpec:

This comment has been minimized.

@myronmarston

myronmarston Mar 31, 2014

Member

Why triple backticks? We generally use single backticks for code identifiers in prose sentences.

@myronmarston

myronmarston Mar 31, 2014

Member

Why triple backticks? We generally use single backticks for code identifiers in prose sentences.

This comment has been minimized.

@waterlink

waterlink Mar 31, 2014

Contributor

Fixed.

@waterlink

waterlink Mar 31, 2014

Contributor

Fixed.

+ end
+ """
+ When I run `rspec`
+ Then the output should contain "1 example, 0 failures"

This comment has been minimized.

@myronmarston

myronmarston Mar 31, 2014

Member

This file is super thorough, so well done :).

However, I think the thoroughness gets in the way of its purpose as documentation (which is one of the main purposes of these files). I don't think users want to read so many scenarios. Also, each scenario adds to the build time of rspec-core, so it's beneficial to have fewer scenarios instead of more.

Here's how I'd recommend we do this:

  • Have 3 scenarios:
    • One with no explicit config (monkey patching will be enabled by default) that shows that the monkey patched methods work. The scenario can define a spec file with multiple examples. It'll use raw describe and can have one example using rspec-expectations' should and one using rspec-mocks' stub or should_receive.
    • One with disable_monkey_patching! config that demonstrates that using the monkey patched APIs fails with NoMethodErrors.
    • One with disable_monkey_patching! that does not use any monkey patched APIs.

Actually, you could probably define a Background step that defines all the spec files for each of these cases, and then in the individual scenarios it can define an appropriate spec_helper that sets the config.

Thinking about this some more...you may also have to put the use of raw describe in a separate file from the user of stub, should and should_receive, since when monkey patching is disabled it'll abort on describe and not even get around to calling those other methods. If you define these in separate files, then in the scenarios you can run rspec path/to/spec to selectively run whatever files you want to to demonstrate the goal of the scenario.

How does that sound?

@myronmarston

myronmarston Mar 31, 2014

Member

This file is super thorough, so well done :).

However, I think the thoroughness gets in the way of its purpose as documentation (which is one of the main purposes of these files). I don't think users want to read so many scenarios. Also, each scenario adds to the build time of rspec-core, so it's beneficial to have fewer scenarios instead of more.

Here's how I'd recommend we do this:

  • Have 3 scenarios:
    • One with no explicit config (monkey patching will be enabled by default) that shows that the monkey patched methods work. The scenario can define a spec file with multiple examples. It'll use raw describe and can have one example using rspec-expectations' should and one using rspec-mocks' stub or should_receive.
    • One with disable_monkey_patching! config that demonstrates that using the monkey patched APIs fails with NoMethodErrors.
    • One with disable_monkey_patching! that does not use any monkey patched APIs.

Actually, you could probably define a Background step that defines all the spec files for each of these cases, and then in the individual scenarios it can define an appropriate spec_helper that sets the config.

Thinking about this some more...you may also have to put the use of raw describe in a separate file from the user of stub, should and should_receive, since when monkey patching is disabled it'll abort on describe and not even get around to calling those other methods. If you define these in separate files, then in the scenarios you can run rspec path/to/spec to selectively run whatever files you want to to demonstrate the goal of the scenario.

How does that sound?

This comment has been minimized.

@waterlink

waterlink Mar 31, 2014

Contributor

Good, I'll do it.

@waterlink

waterlink Mar 31, 2014

Contributor

Good, I'll do it.

lib/rspec/core/configuration.rb
+ #
+ # @note
+ # When rspec-mocks or/and rspec-expectations is not used,
+ # it does nothing for them.

This comment has been minimized.

@myronmarston

myronmarston Mar 31, 2014

Member

Elsewhere we format this like so:

# @note When rspec-mocks or/and rspec-expectations is not used,
#   it does nothing for them.

I'm sure that that will render correctly in the API docs but I'm not sure about your form. Do you mind changing this (here and below)?

@myronmarston

myronmarston Mar 31, 2014

Member

Elsewhere we format this like so:

# @note When rspec-mocks or/and rspec-expectations is not used,
#   it does nothing for them.

I'm sure that that will render correctly in the API docs but I'm not sure about your form. Do you mind changing this (here and below)?

This comment has been minimized.

@waterlink

waterlink Mar 31, 2014

Contributor

I'll fix it.

@waterlink

waterlink Mar 31, 2014

Contributor

I'll fix it.

lib/rspec/core/configuration.rb
+
+ def rspec_mocks_loaded?
+ defined? RSpec::Mocks
+ end

This comment has been minimized.

@myronmarston

myronmarston Mar 31, 2014

Member

I think I'd rather see this logic inlined at the call site (both for this and rspec_expectations_loaded?). I don't see an advantage to making it a separate method.

@myronmarston

myronmarston Mar 31, 2014

Member

I think I'd rather see this logic inlined at the call site (both for this and rspec_expectations_loaded?). I don't see an advantage to making it a separate method.

This comment has been minimized.

@waterlink

waterlink Mar 31, 2014

Contributor

I'm using this method to stub state (mocks loaded or not for example).

I thought about just hiding hide_const(RSpec::Mocks) or hide_const(RSpec::Expectations), but it breaks example itself and renders to be an exception.

@waterlink

waterlink Mar 31, 2014

Contributor

I'm using this method to stub state (mocks loaded or not for example).

I thought about just hiding hide_const(RSpec::Mocks) or hide_const(RSpec::Expectations), but it breaks example itself and renders to be an exception.

spec/rspec/core/configuration_spec.rb
+ end
+ end
+ end
+ end

This comment has been minimized.

@myronmarston

myronmarston Mar 31, 2014

Member

Your specs here rely extensively on mocking. I'd prefer it do minimal (if any) mocking, for a couple reasons:

  • Mocking the object-under-test is a code smell.
  • These specs are tightly coupled to the implementation (e.g. the private helper methods that it calls), which will make future refactoring more difficult.
  • These specs give me confidence that particular methods are being called based on the config, but it doesn't give me confidence that it's actually achieving the desired behavior. The cuke helps provide that level of coverage but we don't often run the cukes locally (as they are so much slower) so we like to keep the specs integrating enough to give us confidence things are working.

Here's an example of how you could test this w/o mocking:

it "stops exposing the DSL methods globally" do
  mod = Module.new
  config.expose_dsl_globally = true
  expect {
    config.disable_monkey_patching!
  }.to change { mod.respond_to?(:describe) }.from(true).to(false)
end

For the specs that show that it works properly when rspec-expectations or rspec-mocks are not loaded, you can use hide_const("RSpec::Mocks") (or hide_const("RSpec::Expectations")) to make it simulate those libraries not being loaded.

@myronmarston

myronmarston Mar 31, 2014

Member

Your specs here rely extensively on mocking. I'd prefer it do minimal (if any) mocking, for a couple reasons:

  • Mocking the object-under-test is a code smell.
  • These specs are tightly coupled to the implementation (e.g. the private helper methods that it calls), which will make future refactoring more difficult.
  • These specs give me confidence that particular methods are being called based on the config, but it doesn't give me confidence that it's actually achieving the desired behavior. The cuke helps provide that level of coverage but we don't often run the cukes locally (as they are so much slower) so we like to keep the specs integrating enough to give us confidence things are working.

Here's an example of how you could test this w/o mocking:

it "stops exposing the DSL methods globally" do
  mod = Module.new
  config.expose_dsl_globally = true
  expect {
    config.disable_monkey_patching!
  }.to change { mod.respond_to?(:describe) }.from(true).to(false)
end

For the specs that show that it works properly when rspec-expectations or rspec-mocks are not loaded, you can use hide_const("RSpec::Mocks") (or hide_const("RSpec::Expectations")) to make it simulate those libraries not being loaded.

This comment has been minimized.

@waterlink

waterlink Mar 31, 2014

Contributor

I feel some problems from hide_const("RSpec::Expectations"), but maybe I am wrong.

I'll do it in this style. I think, my current cucumber feature may be a base to make this specs better. I'll just borrow ideas from there and put them in spec.

@waterlink

waterlink Mar 31, 2014

Contributor

I feel some problems from hide_const("RSpec::Expectations"), but maybe I am wrong.

I'll do it in this style. I think, my current cucumber feature may be a base to make this specs better. I'll just borrow ideas from there and put them in spec.

This comment has been minimized.

@myronmarston

myronmarston Mar 31, 2014

Member

Hmm, yeah I can see that causing problems. They may not be easily solvable.

One alternate approach: use defined?(RSpec::Mocks.configuration) and defined?(RSpec::Expectations.configuration) and then you can can use stub_const("RSpec::Mocks", Module.new, :transfer_nested_constants => true) and then the defined? checks will return false but code that needs to access a nested constant will still work.

@myronmarston

myronmarston Mar 31, 2014

Member

Hmm, yeah I can see that causing problems. They may not be easily solvable.

One alternate approach: use defined?(RSpec::Mocks.configuration) and defined?(RSpec::Expectations.configuration) and then you can can use stub_const("RSpec::Mocks", Module.new, :transfer_nested_constants => true) and then the defined? checks will return false but code that needs to access a nested constant will still work.

This comment has been minimized.

@waterlink

waterlink Mar 31, 2014

Contributor

It is something unbelievable:

      context 'when rspec-mocks is not loaded' do
        let!(:original_mocks) { RSpec::Mocks }

        # trick to emulate mock framework adapter initial loading
        # shortly, to restore #configuration method
        before do
          allow(RSpec.configuration).to receive(:mock_with).with(:rspec) do |framework|
            RSpec::Mocks.instance_eval do
              def configuration
                original_mocks.configuration
              end
            end
            RSpec.configuration.instance_eval { @mock_framework = RSpec::Core::MockingAdapters::RSpec }
          end
        end

        # trick RSpec to think that RSpec::Mocks is not configured
        before do
          mocks = stub_const('RSpec::Mocks', RSpec::Mocks.dup)
          mocks.instance_eval { undef :configuration }
          config.instance_eval { @mock_framework = nil }
        end

        # trick example group to think it is not yet configured
        before { ExampleGroup.remove_class_variable :@@example_groups_configured }

        it 'loads rspec-mocks' do
          expect {
            ExampleGroup.ensure_example_groups_are_configured
          }.to change { defined?(RSpec::Mocks.configuration) }.from(nil).to('method')
        end
      end

trying to make it easier.

@waterlink

waterlink Mar 31, 2014

Contributor

It is something unbelievable:

      context 'when rspec-mocks is not loaded' do
        let!(:original_mocks) { RSpec::Mocks }

        # trick to emulate mock framework adapter initial loading
        # shortly, to restore #configuration method
        before do
          allow(RSpec.configuration).to receive(:mock_with).with(:rspec) do |framework|
            RSpec::Mocks.instance_eval do
              def configuration
                original_mocks.configuration
              end
            end
            RSpec.configuration.instance_eval { @mock_framework = RSpec::Core::MockingAdapters::RSpec }
          end
        end

        # trick RSpec to think that RSpec::Mocks is not configured
        before do
          mocks = stub_const('RSpec::Mocks', RSpec::Mocks.dup)
          mocks.instance_eval { undef :configuration }
          config.instance_eval { @mock_framework = nil }
        end

        # trick example group to think it is not yet configured
        before { ExampleGroup.remove_class_variable :@@example_groups_configured }

        it 'loads rspec-mocks' do
          expect {
            ExampleGroup.ensure_example_groups_are_configured
          }.to change { defined?(RSpec::Mocks.configuration) }.from(nil).to('method')
        end
      end

trying to make it easier.

This comment has been minimized.

@waterlink

waterlink Mar 31, 2014

Contributor

Seems it will be a lot easier and cleaner to leave rspec_mocks_loaded? and rspec_expectations_loaded? methods and stub them in tests.

@waterlink

waterlink Mar 31, 2014

Contributor

Seems it will be a lot easier and cleaner to leave rspec_mocks_loaded? and rspec_expectations_loaded? methods and stub them in tests.

This comment has been minimized.

@waterlink

waterlink Mar 31, 2014

Contributor

It became something like that:

      context 'when user did not configure mock framework' do
        def emulate_not_configured_mock_framework
          original_mocks = RSpec::Mocks
          allow(RSpec.configuration).to receive(:mock_with).with(:rspec) do |arg|
            @mocks.send(:define_singleton_method, :configuration) { original_mocks.configuration }
            RSpec.configuration.instance_variable_set :@mock_framework, RSpec::Core::MockingAdapters::RSpec
          end
          @mocks = stub_const('RSpec::Mocks', original_mocks.dup)
          @mocks.instance_eval { undef :configuration }
          RSpec.configuration.instance_variable_set :@mock_framework, nil
          ExampleGroup.remove_class_variable :@@example_groups_configured
        end

        it 'disables monkey patching after example groups being configured' do
          emulate_not_configured_mock_framework

          RSpec.configuration.disable_monkey_patching!
          expect(defined? @mocks.configuration).to eq(nil)

          expect {
            ExampleGroup.ensure_example_groups_are_configured
          }.to change { @mocks.configuration.syntax rescue [:expect, :should] }.
            from([:expect, :should]).to([:expect])
        end
      end

Practically the same thing is for expectations

@waterlink

waterlink Mar 31, 2014

Contributor

It became something like that:

      context 'when user did not configure mock framework' do
        def emulate_not_configured_mock_framework
          original_mocks = RSpec::Mocks
          allow(RSpec.configuration).to receive(:mock_with).with(:rspec) do |arg|
            @mocks.send(:define_singleton_method, :configuration) { original_mocks.configuration }
            RSpec.configuration.instance_variable_set :@mock_framework, RSpec::Core::MockingAdapters::RSpec
          end
          @mocks = stub_const('RSpec::Mocks', original_mocks.dup)
          @mocks.instance_eval { undef :configuration }
          RSpec.configuration.instance_variable_set :@mock_framework, nil
          ExampleGroup.remove_class_variable :@@example_groups_configured
        end

        it 'disables monkey patching after example groups being configured' do
          emulate_not_configured_mock_framework

          RSpec.configuration.disable_monkey_patching!
          expect(defined? @mocks.configuration).to eq(nil)

          expect {
            ExampleGroup.ensure_example_groups_are_configured
          }.to change { @mocks.configuration.syntax rescue [:expect, :should] }.
            from([:expect, :should]).to([:expect])
        end
      end

Practically the same thing is for expectations

This comment has been minimized.

@myronmarston

myronmarston Mar 31, 2014

Member

Yeah, I think I prefer the rspec_<lib>_loaded? approach you had before over that. That said, I don't fully understand why all that extra setup is needed with the approach I suggested, and it's complex enough that explaining why probably won't be simple....so for now go with the simpler approach you originally had and I'll try it out locally and see if I can find a better solution.

@myronmarston

myronmarston Mar 31, 2014

Member

Yeah, I think I prefer the rspec_<lib>_loaded? approach you had before over that. That said, I don't fully understand why all that extra setup is needed with the approach I suggested, and it's complex enough that explaining why probably won't be simple....so for now go with the simpler approach you originally had and I'll try it out locally and see if I can find a better solution.

@waterlink

This comment has been minimized.

Show comment
Hide comment
@waterlink

waterlink Apr 1, 2014

Contributor

According to this article http://blog.travis-ci.com/2013-05-20-network-timeouts-build-retries/ I think it would be good to put travis_retry before bundle install ones, when build is running on travis.

Contributor

waterlink commented Apr 1, 2014

According to this article http://blog.travis-ci.com/2013-05-20-network-timeouts-build-retries/ I think it would be good to put travis_retry before bundle install ones, when build is running on travis.

lib/rspec/core/configuration.rb
+ #
+ # @note If the user uses this options with `mock_with :mocha`
+ # (or similiar) that they will still have monkey patching active
+ # in their test environment from mocha.

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

Does it look good ?

@waterlink

waterlink Apr 1, 2014

Contributor

Does it look good ?

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

Yes, this is well written. Please indent the 2nd/3rd lines of your notes, though.

@myronmarston

myronmarston Apr 1, 2014

Member

Yes, this is well written. Please indent the 2nd/3rd lines of your notes, though.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 1, 2014

Member

According to this article http://blog.travis-ci.com/2013-05-20-network-timeouts-build-retries/ I think it would be good to put travis_retry before bundle install ones, when build is running on travis.

We already do this:

travis_retry bundle install $bundle_install_flags

Member

myronmarston commented Apr 1, 2014

According to this article http://blog.travis-ci.com/2013-05-20-network-timeouts-build-retries/ I think it would be good to put travis_retry before bundle install ones, when build is running on travis.

We already do this:

travis_retry bundle install $bundle_install_flags

@waterlink

This comment has been minimized.

Show comment
Hide comment
@waterlink

waterlink Apr 1, 2014

Contributor

Hmm, strange, then why it did not retried a lot in previous commits ? Just failed from first connection timeout error..

Contributor

waterlink commented Apr 1, 2014

Hmm, strange, then why it did not retried a lot in previous commits ? Just failed from first connection timeout error..

+ expectations.syntax = [:expect, :should]
+ end
+ end
+ """

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

RSpec enables both syntaxes by default. No need to explicitly configure it like this. It makes this file noisier then it needs to be and suggests to the user they have to enable it, so I'd prefer to see this removed.

@myronmarston

myronmarston Apr 1, 2014

Member

RSpec enables both syntaxes by default. No need to explicitly configure it like this. It makes this file noisier then it needs to be and suggests to the user they have to enable it, so I'd prefer to see this removed.

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

It seems to be not truthy. When I tried to run it without setting up syntax I ended up with errors. There were no should, stub and should_not methods. I have 3.0.0.beta2 version of all rspec-* related gems (cloned right from github). Could it be that my environment somehow is weird ? And it defaults me to only expect syntax ?

@waterlink

waterlink Apr 1, 2014

Contributor

It seems to be not truthy. When I tried to run it without setting up syntax I ended up with errors. There were no should, stub and should_not methods. I have 3.0.0.beta2 version of all rspec-* related gems (cloned right from github). Could it be that my environment somehow is weird ? And it defaults me to only expect syntax ?

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

Oh right...I forgot about this, but we have something in our cucumber environment that enforces the use of the expect syntax in our cukes because we want it to be shown in docs as we consider it the primary syntax moving forward:

https://github.com/rspec/rspec-core/blob/master/features/support/require_expect_syntax_in_aruba_specs.rb

The solution is to tweak that file like we have it in rspec-expecations:

https://github.com/rspec/rspec-expectations/blob/9febc53557ccec50bd2549b3bedc77e94b603a2b/features/support/disallow_certain_apis.rb#L6-L8

...so that it allows scenarios tagged with a particular tag to use should, stub, etc.

@myronmarston

myronmarston Apr 1, 2014

Member

Oh right...I forgot about this, but we have something in our cucumber environment that enforces the use of the expect syntax in our cukes because we want it to be shown in docs as we consider it the primary syntax moving forward:

https://github.com/rspec/rspec-core/blob/master/features/support/require_expect_syntax_in_aruba_specs.rb

The solution is to tweak that file like we have it in rspec-expecations:

https://github.com/rspec/rspec-expectations/blob/9febc53557ccec50bd2549b3bedc77e94b603a2b/features/support/disallow_certain_apis.rb#L6-L8

...so that it allows scenarios tagged with a particular tag to use should, stub, etc.

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

But when I run it in script/console it shows that both :should and :expect syntax used.

@waterlink

waterlink Apr 1, 2014

Contributor

But when I run it in script/console it shows that both :should and :expect syntax used.

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

But when I run it in script/console it shows that both :should and :expect syntax used.

Huh, I forgot we had script/console. Anyhow, it doesn't have the setup the cucumber environment has, so it's not going to be the same.

@myronmarston

myronmarston Apr 1, 2014

Member

But when I run it in script/console it shows that both :should and :expect syntax used.

Huh, I forgot we had script/console. Anyhow, it doesn't have the setup the cucumber environment has, so it's not going to be the same.

+ end
+ """
+
+ Scenario: by default RSpec allows monkey patching

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

...especially because this says "by default" but then explicitly configures it, so that it's not actually demonstrating the defaults.

@myronmarston

myronmarston Apr 1, 2014

Member

...especially because this says "by default" but then explicitly configures it, so that it's not actually demonstrating the defaults.

+ When I run `rspec`
+ Then the output should contain "2 examples, 0 failures"
+
+ Scenario: in zero monkey patching mode monkey patching not works

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

I'd rewrite this to: "in zero monkey patching mode, the monkey patched methods are undefined"

Also, I think we're standardizing on capitalizing the first word in the scenario name. @cupakromer, can you confirm?

@myronmarston

myronmarston Apr 1, 2014

Member

I'd rewrite this to: "in zero monkey patching mode, the monkey patched methods are undefined"

Also, I think we're standardizing on capitalizing the first word in the scenario name. @cupakromer, can you confirm?

@@ -1008,6 +1009,7 @@ def configure_expectation_framework
expectation_frameworks.each do |framework|
RSpec::Core::ExampleGroup.__send__(:include, framework)
end
+ conditionally_disable_expectations_monkey_patching

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

I don't see any specs that demonstrate that this is done when the mock and expectation frameworks are configured.

@myronmarston

myronmarston Apr 1, 2014

Member

I don't see any specs that demonstrate that this is done when the mock and expectation frameworks are configured.

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

Nevermind, I see that you do have specs for this :).

@myronmarston

myronmarston Apr 1, 2014

Member

Nevermind, I see that you do have specs for this :).

lib/rspec/core/configuration.rb
@@ -1155,6 +1157,52 @@ def raise_errors_for_deprecations!
self.deprecation_stream = Formatters::DeprecationFormatter::RaiseErrorStream.new
end
+ # Enables zero monkey patching mode for RSpec. It removes monkey
+ # patching of the top-level DSL methods (`describe`,
+ # `shared_examples_for`, etc) onto `main` and `Module` instead

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

This needs a comma between Module and "instead".

@myronmarston

myronmarston Apr 1, 2014

Member

This needs a comma between Module and "instead".

lib/rspec/core/configuration.rb
+ # requiring you to prefix these methods with `RSpec.`. It enables
+ # expect-only syntax for rspec-mocks and rspec-expectations. It
+ # simply disables monkey patching on whatever pieces of rspec
+ # the user is using

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

This needs a period.

@myronmarston

myronmarston Apr 1, 2014

Member

This needs a period.

spec/rspec/core/configuration_spec.rb
+ context 'when user did not configure mock framework' do
+ def emulate_not_configured_mock_framework
+ allow(config).to receive(:rspec_mocks_loaded?).and_return(false)
+ RSpec.configuration.instance_variable_set :@mock_framework, nil

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

There are two configuration instances here -- config and RSpec.configuration. It's confusing that you're using both. Can it just use config? If not, then it should just use RSpec.configuration, IMO.

@myronmarston

myronmarston Apr 1, 2014

Member

There are two configuration instances here -- config and RSpec.configuration. It's confusing that you're using both. Can it just use config? If not, then it should just use RSpec.configuration, IMO.

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

It works with config, I'll fix it in next commit.

@waterlink

waterlink Apr 1, 2014

Contributor

It works with config, I'll fix it in next commit.

spec/rspec/core/configuration_spec.rb
+ obj = Object.new
+ RSpec.configuration.disable_monkey_patching!
+
+ expect(RSpec::Mocks.configuration).to receive(:syntax=).with(:expect)

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

I'd prefer to see this not use mocking here...is it necessary? Seems like that the expect(obj).not_to respond_to(:should_receive) line would be sufficient...

@myronmarston

myronmarston Apr 1, 2014

Member

I'd prefer to see this not use mocking here...is it necessary? Seems like that the expect(obj).not_to respond_to(:should_receive) line would be sufficient...

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

The main problem is that current spec_helper specifies only expect syntax for both frameworks. So expect(obj).not_to respond_to(:should_receive) will be always a success, because should_receive never gets its hands on any object.
So I wanted to check if it really configures expect syntax.

@waterlink

waterlink Apr 1, 2014

Contributor

The main problem is that current spec_helper specifies only expect syntax for both frameworks. So expect(obj).not_to respond_to(:should_receive) will be always a success, because should_receive never gets its hands on any object.
So I wanted to check if it really configures expect syntax.

This comment has been minimized.

@JonRowe

JonRowe Apr 1, 2014

Member

So enable the should syntax for this spec, and then call disable_monkey_patching!

@JonRowe

JonRowe Apr 1, 2014

Member

So enable the should syntax for this spec, and then call disable_monkey_patching!

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

It is possible to do something outrageous:
Move this two contexts into another spec file and require there different spec_helper, so inside this suite both syntax will be available by default.
And so we can check that object responds to should when we didn't run disable_monkey_patching! and in another example check that object does not responds to should when we did run disable_monkey_patching!

We can go even further (without need to create different spec_helper), we can take advantage of environment variables for example:

# spec/rspec/core/our_brand_new_spec.rb
ENV['ALLOW_SHOULD_SYNTAX'] = 'true'
require 'spec_helper'

# here our specs
# spec/spec_helper.rb
# ...
    c.expect_with :rspec do |expectations|
      expectations.syntax = ENV['ALLOW_SHOULD_SYNTAX'] == 'true' ? [:expect, :should] : :expect
    end

    c.mock_with :rspec do |mocks|
      mocks.syntax = ENV['ALLOW_SHOULD_SYNTAX'] == 'true' ? [:expect, :should] : :expect
    end
# ...

That might work. But is that a good approach to problem ?

@waterlink

waterlink Apr 1, 2014

Contributor

It is possible to do something outrageous:
Move this two contexts into another spec file and require there different spec_helper, so inside this suite both syntax will be available by default.
And so we can check that object responds to should when we didn't run disable_monkey_patching! and in another example check that object does not responds to should when we did run disable_monkey_patching!

We can go even further (without need to create different spec_helper), we can take advantage of environment variables for example:

# spec/rspec/core/our_brand_new_spec.rb
ENV['ALLOW_SHOULD_SYNTAX'] = 'true'
require 'spec_helper'

# here our specs
# spec/spec_helper.rb
# ...
    c.expect_with :rspec do |expectations|
      expectations.syntax = ENV['ALLOW_SHOULD_SYNTAX'] == 'true' ? [:expect, :should] : :expect
    end

    c.mock_with :rspec do |mocks|
      mocks.syntax = ENV['ALLOW_SHOULD_SYNTAX'] == 'true' ? [:expect, :should] : :expect
    end
# ...

That might work. But is that a good approach to problem ?

This comment has been minimized.

@JonRowe

JonRowe Apr 1, 2014

Member

Take a look at how we configure the specs for the syntax enabling disabling, we've already solved this problem to our satisfaction.

Move this two contexts into another spec file and require there different spec_helper, so inside this suite both syntax will be available by default

Won't work if in the same process as our current test suite (which it would be by default).

... snip...
That might work. But is that a good approach to problem ?

As above, it would just conflict between specs and isn't a good approach to the problem, we shouldn't rely on the outer RSpec environment in our tests.

@JonRowe

JonRowe Apr 1, 2014

Member

Take a look at how we configure the specs for the syntax enabling disabling, we've already solved this problem to our satisfaction.

Move this two contexts into another spec file and require there different spec_helper, so inside this suite both syntax will be available by default

Won't work if in the same process as our current test suite (which it would be by default).

... snip...
That might work. But is that a good approach to problem ?

As above, it would just conflict between specs and isn't a good approach to the problem, we shouldn't rely on the outer RSpec environment in our tests.

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

@JonRowe Got it. Thank you.

@waterlink

waterlink Apr 1, 2014

Contributor

@JonRowe Got it. Thank you.

This comment has been minimized.

@JonRowe

JonRowe Apr 1, 2014

Member

See: https://github.com/rspec/rspec-expectations/blob/9febc53557ccec50bd2549b3bedc77e94b603a2b/spec/rspec/expectations/configuration_spec.rb

Here we disable and enable syntaxes for testing, but make sure to reset them to their original values afterwards. For additional bonus points you could use the in_sub_process do ... end helpers to isolate this from the main RSpec process entirely, and then you have less cleanup to worry about.

@JonRowe

JonRowe Apr 1, 2014

Member

See: https://github.com/rspec/rspec-expectations/blob/9febc53557ccec50bd2549b3bedc77e94b603a2b/spec/rspec/expectations/configuration_spec.rb

Here we disable and enable syntaxes for testing, but make sure to reset them to their original values afterwards. For additional bonus points you could use the in_sub_process do ... end helpers to isolate this from the main RSpec process entirely, and then you have less cleanup to worry about.

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

Seems to be required to use in_sub_process for these specs because it mangles with expose_dsl_globally and everything else.

@waterlink

waterlink Apr 1, 2014

Contributor

Seems to be required to use in_sub_process for these specs because it mangles with expose_dsl_globally and everything else.

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

This specs should look better now.
@JonRowe Thanks for the tip!

@waterlink

waterlink Apr 1, 2014

Contributor

This specs should look better now.
@JonRowe Thanks for the tip!

spec/rspec/core/configuration_spec.rb
+
+ context 'when user did not configure mock framework' do
+ def emulate_not_configured_mock_framework
+ allow(config).to receive(:rspec_mocks_loaded?).and_return(false)

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

This line confuses me...if rspec_mocks_loaded? always returns false, then it wouldn't get configured...but then you show below that it does get configured.

I suspect it has to do with the fact that this is a different config instance and this stub probably never gets used?

@myronmarston

myronmarston Apr 1, 2014

Member

This line confuses me...if rspec_mocks_loaded? always returns false, then it wouldn't get configured...but then you show below that it does get configured.

I suspect it has to do with the fact that this is a different config instance and this stub probably never gets used?

This comment has been minimized.

@waterlink

waterlink Apr 1, 2014

Contributor

What if we do something like that:

allow(config).to receive(:rspec_mocks_loaded?).and_return(false, true)

So it will return false once and afterwards will return true.

We can do this in more honest way:

  1. stub rspec_mocks_loaded? to return false
  2. stub config#mock_with with custom block which will unstub rspec_mocks_loaded? or stub it with true
@waterlink

waterlink Apr 1, 2014

Contributor

What if we do something like that:

allow(config).to receive(:rspec_mocks_loaded?).and_return(false, true)

So it will return false once and afterwards will return true.

We can do this in more honest way:

  1. stub rspec_mocks_loaded? to return false
  2. stub config#mock_with with custom block which will unstub rspec_mocks_loaded? or stub it with true
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 1, 2014

Member

This is looking really good, @waterlink!

Member

myronmarston commented Apr 1, 2014

This is looking really good, @waterlink!

@waterlink

This comment has been minimized.

Show comment
Hide comment
@waterlink

waterlink Apr 1, 2014

Contributor

@myronmarston Re-written feature. How does it look now ? Is it better than it was before ?

Contributor

waterlink commented Apr 1, 2014

@myronmarston Re-written feature. How does it look now ? Is it better than it was before ?

spec/rspec/core/configuration_spec.rb
+ in_sub_process do
+ config.expose_dsl_globally = true
+ mocks.configuration.syntax = [:expect, :should]
+ mocks.configuration.patch_marshal_to_support_partial_doubles = true

This comment has been minimized.

@myronmarston

myronmarston Apr 1, 2014

Member

This defaults to false, actually. I think it's useful to show how things work to have this method set it to true. I'd just rename this to in_fully_monkey_patched_rspec_environment.

@myronmarston

myronmarston Apr 1, 2014

Member

This defaults to false, actually. I think it's useful to show how things work to have this method set it to true. I'd just rename this to in_fully_monkey_patched_rspec_environment.

@waterlink

This comment has been minimized.

Show comment
Hide comment
@waterlink

waterlink Apr 3, 2014

Contributor

@myronmarston It seems to be ready. What do you think ?

Contributor

waterlink commented Apr 3, 2014

@myronmarston It seems to be ready. What do you think ?

spec/rspec/core/configuration_spec.rb
+ config.instance_variable_set :@expectation_frameworks, []
+ ExampleGroup.send :remove_class_variable, :@@example_groups_configured
+ end
+ end

This comment has been minimized.

@myronmarston

myronmarston Apr 3, 2014

Member

This method needs a yield...so the spec below isn't testing anything.

It's a good idea to "break" the implementation and see the test fail when you refactor it like this to confirm it still fails with a good failure message in the way you expect.

@myronmarston

myronmarston Apr 3, 2014

Member

This method needs a yield...so the spec below isn't testing anything.

It's a good idea to "break" the implementation and see the test fail when you refactor it like this to confirm it still fails with a good failure message in the way you expect.

This comment has been minimized.

@waterlink

waterlink Apr 3, 2014

Contributor

@myronmarston Fixed this. So it is good Idea to return to red phase in red-green-refactor cycle after refactor step.

@waterlink

waterlink Apr 3, 2014

Contributor

@myronmarston Fixed this. So it is good Idea to return to red phase in red-green-refactor cycle after refactor step.

[fix rspec/rspec-core#1433] Added option to disable all monkey patching
- Added option to disable all monkey patching
- Fixed syntax to support ruby-1.8.7
- Refactored disable_monkey_patching!. Added docs, feature.
- Moved monkey patching specs to proper place
- Fixed zero monkey patching feature to support ruby-1.8.7
- Re-written specs to be more BDD-ish.
- Re-written docs for disable_monkey_patching!
- Made non-configured frameworks specs easier
- Fixed cuke to be lighter
- Fixed specs to support 1.8.7
- Improved rspec_<lib>_loaded? a bit;
- Improved not configured frameworks specs.
- Fixed typo in comments-docs.
- Fixed docs; rewritten feature.
- Re-written to use `in_sub_process`.
- Added missing yield to
  emulate_not_configured_expectation_framework
+ Given a file named "spec/spec_helper.rb" with:
+ """ruby
+ """
+ When I run `rspec spec/example_should_spec.rb`

This comment has been minimized.

@myronmarston

myronmarston Apr 3, 2014

Member

This scenario should also run spec/example_describe_spec.rb so that it shows that monkey-patched describe works as well. Currently that spec file is only run when monkey patching is disabled (in the scenario below). Really this should probably just use rspec (with no spec files specified as that will run them all).

@myronmarston

myronmarston Apr 3, 2014

Member

This scenario should also run spec/example_describe_spec.rb so that it shows that monkey-patched describe works as well. Currently that spec file is only run when monkey patching is disabled (in the scenario below). Really this should probably just use rspec (with no spec files specified as that will run them all).

This comment has been minimized.

@waterlink

waterlink Apr 3, 2014

Contributor

Yes, my bad.

@waterlink

waterlink Apr 3, 2014

Contributor

Yes, my bad.

myronmarston added a commit that referenced this pull request Apr 3, 2014

Merge pull request #1465 from waterlink/issue-1433-disable-all-monkey…
…-patching-option

Added option to disable all monkey patching

@myronmarston myronmarston merged commit 914cf0a into rspec:master Apr 3, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 3, 2014

Member

Well done, @waterlink! I'm going to fixup the one final comment I made above post-merge.

Member

myronmarston commented Apr 3, 2014

Well done, @waterlink! I'm going to fixup the one final comment I made above post-merge.

myronmarston added a commit that referenced this pull request Apr 3, 2014

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 3, 2014

Member

BTW, the slight change to the cuke is in b897913, in case you were wondering.

Member

myronmarston commented Apr 3, 2014

BTW, the slight change to the cuke is in b897913, in case you were wondering.

@waterlink waterlink deleted the waterlink:issue-1433-disable-all-monkey-patching-option branch Apr 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment