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

Remove monkey patching #1036

Merged
merged 16 commits into from
Dec 3, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion features/command_line/exit_status.feature
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ Feature: exit status
require 'rspec/autorun'
at_exit { exit(0) }

describe "exit 0 at_exit" do
RSpec.describe "exit 0 at_exit" do
it "does not interfere with rspec's exit code" do
fail
end
Expand Down
2 changes: 1 addition & 1 deletion features/command_line/ruby.feature
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Feature: run with ruby command
"""ruby
require 'rspec/autorun'

describe 1 do
RSpec.describe 1 do
it "is < 2" do
expect(1).to be < 2
end
Expand Down
54 changes: 54 additions & 0 deletions features/configuration/enable_global_dsl.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
Feature: Global namespace DSL

RSpec has a few top-level constructs that allow you to begin describing
behaviour:

* `RSpec.describe`: Define a named context for a group of examples.
* `RSpec.shared_examples_for`: Define a set of shared examples that can later be included in an example group.
* `RSpec.shared_context`: define some common context (using `before`, `let`, helper methods, etc) that can later be included in an example group.

Historically, these constructs have been available directly off of the main
object, so that you could use these at the start of a file without the
`RSpec.` prefix. They have also been available off of any class or module so
that you can scope your examples within a particular constant namespace.

RSpec 3 now provides an option to disable this global monkey patching:

`config.expose_dsl_globally = false`.

For backwards compatibility it defaults to true.

Scenario: by default RSpec allows the DSL to be used globally
Given a file named "spec/example_spec.rb" with:
"""ruby
describe "specs here" do
it "passes" do
end
end
"""
When I run `rspec`
Then the output should contain "1 example, 0 failures"

Scenario: when exposing globally is disabled the top level DSL no longer works
Given a file named "spec/example_spec.rb" with:
"""ruby
RSpec.configure { |c| c.expose_dsl_globally = false }
describe "specs here" do
it "passes" do
end
end
"""
When I run `rspec`
Then the output should contain "undefined method `describe'"

Scenario: regardless of setting
Given a file named "spec/example_spec.rb" with:
"""ruby
RSpec.configure { |c| c.expose_dsl_globally = true }
RSpec.describe "specs here" do
it "passes" do
end
end
"""
When I run `rspec`
Then the output should contain "1 example, 0 failures"
7 changes: 6 additions & 1 deletion lib/rspec/core.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,12 @@ def self.configuration

WARNING
end
@configuration ||= RSpec::Core::Configuration.new
@configuration ||= begin
config = RSpec::Core::Configuration.new
config.expose_dsl_globally = true
config
end

end

# @private
Expand Down
17 changes: 17 additions & 0 deletions lib/rspec/core/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,23 @@ def self.add_setting(name, opts={})
# Default: `$stderr`.
add_setting :error_stream

# @macro add_setting
# Default: true
# Use this to expose the core RSpec DSL via `Module` and the `main`
# object. It will be set automatically but you can override it to
# remove the DSL.
add_setting :expose_dsl_globally

def expose_dsl_globally=(value)
if value
Core::DSL.expose_globally!
Core::SharedExampleGroup::TopLevelDSL.expose_globally!
else
Core::DSL.remove_globally!
Core::SharedExampleGroup::TopLevelDSL.remove_globally!
end
end

# @macro add_setting
# Default: `$stderr`.
add_setting :deprecation_stream
Expand Down
76 changes: 58 additions & 18 deletions lib/rspec/core/dsl.rb
Original file line number Diff line number Diff line change
@@ -1,26 +1,66 @@
module RSpec

# Defines a named context for one or more examples. The given block
# is evaluated in the context of a generated subclass of
# {RSpec::Core::ExampleGroup}
#
# ## Examples:
#
# describe "something" do
# it "does something" do
# # example code goes here
# end
# end
#
# @see ExampleGroup
# @see ExampleGroup.describe
def self.describe(*args, &example_group_block)
RSpec::Core::ExampleGroup.describe(*args, &example_group_block).register
end

module Core
# Adds the `describe` method to the top-level namespace.
module DSL
# Generates a subclass of {ExampleGroup}
#
# ## Examples:
#
# describe "something" do
# it "does something" do
# # example code goes here
# end
# end
#
# @see ExampleGroup
# @see ExampleGroup.describe
def describe(*args, &example_group_block)
RSpec::Core::ExampleGroup.describe(*args, &example_group_block).register

class << self
# @private
attr_accessor :top_level
end

# @private
def self.exposed_globally?
@exposed_globally ||= false
Copy link
Member

Choose a reason for hiding this comment

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

||= has funny semantics with false. It doesn't memoize like truthy values do. Should this just be @exposed_globally?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just preventing a warning being issued.

end

# Add's the describe method to Module and the top level binding
def self.expose_globally!
return if exposed_globally?

to_define = proc do
def describe(*args, &block)
::RSpec.describe(*args, &block)
end
end

top_level.instance_eval(&to_define)
Module.class_exec(&to_define)
@exposed_globally = true
Copy link
Member

Choose a reason for hiding this comment

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

One suggestion here...I think this can be simplified with an alternate approch:

  • Have a module like RSpec::Core::TopLevelDSL that is always extended onto main and included in Module.
  • When the expose_globally config changes, it can define or undefine the appropriate methods in that module, which will automatically make it available (or not) from main and from modules.

That way, you don't have to apply all of this to two different contexts.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point is not to monkey patch! By doing that we'd always monkey patch and then have to control that...

end

def self.remove_globally!
return unless exposed_globally?

to_undefine = proc do
undef describe
end

top_level.instance_eval(&to_undefine)
Module.class_exec(&to_undefine)
@exposed_globally = false
end

end
end
end

extend RSpec::Core::DSL
Module.send(:include, RSpec::Core::DSL)

# cature main without an eval
::RSpec::Core::DSL.top_level = self
51 changes: 41 additions & 10 deletions lib/rspec/core/shared_example_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,49 @@ def shared_example_groups
end

module TopLevelDSL
def shared_examples(*args, &block)
SharedExampleGroup.registry.add_group('main', *args, &block)
def self.definitions
proc do
def shared_examples(*args, &block)
SharedExampleGroup.registry.add_group('main', *args, &block)
end
alias :shared_context :shared_examples
alias :share_examples_for :shared_examples
alias :shared_examples_for :shared_examples
Copy link
Member

Choose a reason for hiding this comment

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

Huh, I never knew you could pass symbols to alias. I've always used bare words with alias (e.g. alias shared_examples_for shared_examples), and symbols with alias_method (e.g. alias_method :shared_examples_for, :shared_examples).

Nothing needs to be changed here...I just found it interesting :).

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI the difference is the bare name will error if the method doesn't exist. I just got used to using symbols...

def shared_example_groups
SharedExampleGroup.registry.shared_example_groups_for('main')
end
end
end

alias_method :shared_context, :shared_examples
alias_method :share_examples_for, :shared_examples
alias_method :shared_examples_for, :shared_examples
# @private
def self.exposed_globally?
@exposed_globally ||= false
end

def shared_example_groups
SharedExampleGroup.registry.shared_example_groups_for('main')
def self.expose_globally!
return if exposed_globally?

Core::DSL.top_level.instance_eval(&definitions)
Module.class_exec(&definitions)
@exposed_globally = true
end

def self.remove_globally!
return unless exposed_globally?

to_undefine = proc do
undef shared_examples
undef shared_context
undef share_examples_for
undef shared_examples_for
undef shared_example_groups
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize we had so many methods here. In particular, I didn't realize shared_example_groups is exposed at the top level. Why would it need to be? Also, do we really need share_examples_for and shared_examples_for and shared_examples?

I feel like 3.0 is a good time to consider cleaning up having so many aliases for the same thing. Honestly, I'm not sure on the history of how they all came about...

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

BTW, I definitely don't want to hold up this PR for this -- mostly I was just thinking aloud. If you like, we can open an issue for this and come back to it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats the current public api, which also all do the same thing incidentally.

Copy link
Member

Choose a reason for hiding this comment

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

I know that's the current public API. My point is that it seems excessive to have so many methods that do the same thing.

However, shared_example_groups doesn't do the same thing, and I'm not sure why it's exposed at the top level at all.

Regardless, as you said, it's what we currently have so I'm fine punting on this for another day as long as we open an issue so we don't forget about it.

end

Core::DSL.top_level.instance_eval(&to_undefine)
Module.class_exec(&to_undefine)
@exposed_globally = false
end

end

def self.registry
Expand Down Expand Up @@ -140,7 +172,6 @@ def ensure_block_has_source_location(block, caller_line)
end
end
end
end

extend RSpec::Core::SharedExampleGroup::TopLevelDSL
Module.send(:include, RSpec::Core::SharedExampleGroup::TopLevelDSL)
instance_eval &Core::SharedExampleGroup::TopLevelDSL.definitions
end
13 changes: 13 additions & 0 deletions script/ignores
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,16 @@ lib/rspec/core/example_group.rb: subclass = Class.new(parent)
# This happens at file load time.
lib/rspec/core/formatters/deprecation_formatter.rb: DeprecationError = Class.new(StandardError)

# This enables / disable monkey patching and only happens on demand
lib/rspec/core/dsl.rb: to_undefine = proc do
lib/rspec/core/dsl.rb: undef describe
lib/rspec/core/dsl.rb: top_level.instance_eval(&to_undefine)
lib/rspec/core/dsl.rb: Module.class_exec(&to_undefine)
lib/rspec/core/shared_example_group.rb: to_undefine = proc do
lib/rspec/core/shared_example_group.rb: undef shared_examples
lib/rspec/core/shared_example_group.rb: undef shared_context
lib/rspec/core/shared_example_group.rb: undef share_examples_for
lib/rspec/core/shared_example_group.rb: undef shared_examples_for
lib/rspec/core/shared_example_group.rb: undef shared_example_groups
lib/rspec/core/shared_example_group.rb: Core::DSL.top_level.instance_eval(&to_undefine)
lib/rspec/core/shared_example_group.rb: Module.class_exec(&to_undefine)
30 changes: 15 additions & 15 deletions spec/command_line/order_spec.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
require 'spec_helper'

describe 'command line', :ui do
RSpec.describe 'command line', :ui do
let(:stderr) { StringIO.new }
let(:stdout) { StringIO.new }

before :all do
write_file 'spec/simple_spec.rb', """
describe 'group 1' do
RSpec.describe 'group 1' do
specify('group 1 example 1') {}
specify('group 1 example 2') {}
specify('group 1 example 3') {}
Expand All @@ -19,7 +19,7 @@
"""

write_file 'spec/simple_spec2.rb', """
describe 'group 2' do
RSpec.describe 'group 2' do
specify('group 2 example 1') {}
specify('group 2 example 2') {}
specify('group 2 example 3') {}
Expand All @@ -32,7 +32,7 @@
"""

write_file 'spec/order_spec.rb', """
describe 'group 1' do
RSpec.describe 'group 1' do
specify('group 1 example 1') {}
specify('group 1 example 2') {}
specify('group 1 example 3') {}
Expand Down Expand Up @@ -68,15 +68,15 @@
describe('group 1-10') { specify('example') {} }
end

describe('group 2') { specify('example') {} }
describe('group 3') { specify('example') {} }
describe('group 4') { specify('example') {} }
describe('group 5') { specify('example') {} }
describe('group 6') { specify('example') {} }
describe('group 7') { specify('example') {} }
describe('group 8') { specify('example') {} }
describe('group 9') { specify('example') {} }
describe('group 10') { specify('example') {} }
RSpec.describe('group 2') { specify('example') {} }
RSpec.describe('group 3') { specify('example') {} }
RSpec.describe('group 4') { specify('example') {} }
RSpec.describe('group 5') { specify('example') {} }
RSpec.describe('group 6') { specify('example') {} }
RSpec.describe('group 7') { specify('example') {} }
RSpec.describe('group 8') { specify('example') {} }
RSpec.describe('group 9') { specify('example') {} }
RSpec.describe('group 10') { specify('example') {} }
"""
end

Expand Down Expand Up @@ -158,14 +158,14 @@
end
end

describe 'group B' do
RSpec.describe 'group B' do
specify('group B example D') {}
specify('group B example B') {}
specify('group B example A') {}
specify('group B example C') {}
end

describe 'group A' do
RSpec.describe 'group A' do
specify('group A example 1') {}
end
"""
Expand Down
2 changes: 1 addition & 1 deletion spec/rspec/core/backtrace_formatter_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require "spec_helper"

module RSpec::Core
describe BacktraceFormatter do
RSpec.describe BacktraceFormatter do
def make_backtrace_formatter(exclusion_patterns=nil, inclusion_patterns=nil)
BacktraceFormatter.new.tap do |bc|
bc.exclusion_patterns = exclusion_patterns if exclusion_patterns
Expand Down
12 changes: 8 additions & 4 deletions spec/rspec/core/command_line_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,23 @@
require 'tmpdir'

module RSpec::Core
describe CommandLine do
RSpec.describe CommandLine do

let(:out) { StringIO.new }
let(:err) { StringIO.new }
let(:config) { RSpec::configuration }
let(:world) { RSpec::world }

before { allow(config.hooks).to receive(:run) }
before do
allow(config.hooks).to receive(:run)
allow(config).to receive(:expose_dsl_globally?).and_return(false)
end

it "configures streams before command line options" do
allow(RSpec).to receive(:deprecate) # remove this and should_receive when ordered
stdout = StringIO.new
allow(config).to receive :load_spec_files
allow(config).to receive_messages(:reporter => double.as_null_object)
allow(config).to receive(:load_spec_files)
allow(config).to receive(:reporter).and_return(double.as_null_object)
config.output_stream = $stdout

# this is necessary to ensure that color works correctly on windows
Expand Down
Loading