Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a warning when the should syntax is used. #339

Merged
merged 1 commit into from
Sep 10, 2013
Merged

Add a warning when the should syntax is used. #339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 6 additions & 2 deletions lib/rspec/mocks/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,17 @@ def transfer_nested_constants?
def transfer_nested_constants=(val)
@transfer_nested_constants = val
end

def reset_syntaxes_to_default
self.syntax = [:should, :expect]
RSpec::Mocks::Syntax.warn_about_should!
end
Copy link
Member

Choose a reason for hiding this comment

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

You know, I'm thinking that this method doesn't really belong here: Configuration is for public configuration methods, not for private RSpec implementation details. I'd consider this to be a private method, not intended to be called by end users. Also, instructing a config option to emit a warning is a bit odd. (Why should the config object have that responsibility?)

Maybe it belongs on the Syntax module instead? Thoughts?

end

def self.configuration
@configuration ||= Configuration.new
end

configuration.syntax = [:should, :expect]
configuration.reset_syntaxes_to_default
end
end

24 changes: 24 additions & 0 deletions lib/rspec/mocks/syntax.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,42 @@ module Mocks
# Provides methods for enabling and disabling the available syntaxes
# provided by rspec-mocks.
module Syntax
def self.warn_about_should!
@warn_about_should = true
end

def self.warn_unless_should_configured(method_name)
if @warn_about_should
RSpec.deprecate(
"Using #{method_name} from the old `:should` syntax without explicitly enabling the syntax.",
:replacement => "the new `:expect` syntax or explicitly enable `:should`"
)

@warn_about_should = false
end
end

# @api private
# Enables the should syntax (`dbl.stub`, `dbl.should_receive`, etc).
def self.enable_should(syntax_host = default_should_syntax_host)
@warn_about_should = false
return if should_enabled?(syntax_host)

syntax_host.class_exec do
def should_receive(message, opts={}, &block)
::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
opts[:expected_from] ||= CallerFilter.first_non_rspec_line
::RSpec::Mocks.expect_message(self, message.to_sym, opts, &block)
end

def should_not_receive(message, &block)
::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
opts = {:expected_from => CallerFilter.first_non_rspec_line}
::RSpec::Mocks.expect_message(self, message.to_sym, opts, &block).never
end

def stub(message_or_hash, opts={}, &block)
::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
if ::Hash === message_or_hash
message_or_hash.each {|message, value| stub(message).and_return value }
else
Expand All @@ -31,29 +49,35 @@ def stub(message_or_hash, opts={}, &block)
end

def unstub(message)
::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
::RSpec::Mocks.space.proxy_for(self).remove_stub(message)
end

def stub_chain(*chain, &blk)
::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
::RSpec::Mocks::StubChain.stub_chain_on(self, *chain, &blk)
end

def as_null_object
::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
@_null_object = true
::RSpec::Mocks.proxy_for(self).as_null_object
end

def null_object?
::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
defined?(@_null_object)
end

def received_message?(message, *args, &block)
::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
::RSpec::Mocks.proxy_for(self).received_message?(message, *args, &block)
end

unless Class.respond_to? :any_instance
Class.class_exec do
def any_instance
::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
::RSpec::Mocks.any_instance_recorder_for(self)
end
end
Expand Down
79 changes: 79 additions & 0 deletions spec/rspec/mocks/configuration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ def sandboxed
expect(configured_syntax).to eq([:should])
end

it "does not warn about the should syntax" do
RSpec.should_not_receive(:deprecate)
Object.new.should_not_receive(:bees)
end

it 'is a no-op when configured a second time' do
Syntax.default_should_syntax_host.should_not_receive(:method_added)
::RSpec::Mocks::ExampleMethods.should_not_receive(:method_undefined)
Expand All @@ -110,6 +115,68 @@ def sandboxed
it 'reports that both syntaxes are enabled' do
expect(configured_syntax).to eq([:should, :expect])
end

it "does not warn about the should syntax" do
RSpec.should_not_receive(:deprecate)
Object.new.should_not_receive(:bees)
end
end

context "by default" do
before do
configure_default_syntax
end

let(:expected_arguments) {
[
/Using.*without explicitly enabling/,
{:replacement=>"the new `:expect` syntax or explicitly enable `:should`"}
]
}

it "it warns about should once, regardless of how many times it is called" do
expect(RSpec).to receive(:deprecate).with(*expected_arguments)
o = Object.new
o2 = Object.new
o.should_receive(:bees)
o2.should_receive(:bees)

o.bees
o2.bees
end

it "warns about should not once, regardless of how many times it is called" do
expect(RSpec).to receive(:deprecate).with(*expected_arguments)
o = Object.new
o2 = Object.new
o.should_not_receive(:bees)
o2.should_not_receive(:bees)
end

it "warns about stubbing once, regardless of how many times it is called" do
expect(RSpec).to receive(:deprecate).with(*expected_arguments)
o = Object.new
o2 = Object.new

o.stub(:faces)
o2.stub(:faces)
end

it "doesn't warn about stubbing after a reset and setting should" do
expect(RSpec).not_to receive(:deprecate)
RSpec::Mocks.configuration.reset_syntaxes_to_default
RSpec::Mocks.configuration.syntax = :should
o = Object.new
o2 = Object.new
o.stub(:faces)
o2.stub(:faces)
end
Copy link
Member

Choose a reason for hiding this comment

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

Given that we've had some trouble with this, I'd like a spec that asserts on the correct call site being included in the deprecation warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean add the call site to the expected_arguments list, slightly confused because you've put this next to the negative case that doesn't filter by arguments. I assume you only want this to happen on the ones that do args matching?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just had another look at this, it'd change the expect to kernel#warn instead of rspec#deprecate. Based on looking at this I'd assume we have tests elsewhere for call sites from CallerFilter? if not would you go for something like expect(CallerFilter).to receive(:first_non_rspec_line).and_return(the correct things here)?

Copy link
Member

Choose a reason for hiding this comment

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

you mean add the call site to the expected_arguments list, slightly confused because you've put this next to the negative case that doesn't filter by arguments. I assume you only want this to happen on the ones that do args matching?

No, I would like an additional spec added specifically for this. Having a spec for it calls it out as important (which it is).

Based on looking at this I'd assume we have tests elsewhere for call sites from CallerFilter

No, setting a mock expectation on CallerFilter doesn't really help us here; consider that a few weeks back, we had two different deprecation warnings that were printing the wrong call site, and both were using the CallerFilter (or the equivalent, before that got added):

  • in rspec-expectations, the be_true deprecation was printing a line from rspec/matchers.rb, because the caller-searching logic considered rspec/matchers/*.rb to be an rspec file but not rspec/matchers.rb. Luckily, CallerFilter as added to rspec-mocks already handled this correctly, so I ported it to rspec-core to solve the bug.
  • in rspec-mocks, the deprecation for the any_instance receiver yielding was printing a line from the user's code but the wrong line, because we wanted it to print the line where the implementation block was defined, but the warning was triggered when the mocked/stubbed method was called, and thus CallerFilter was being given the wrong backtrace to begin with.

To really have confidence that the right line is being printed, we need a spec that actually asserts on the file and line number (typically with __FILE__ and __LINE__ so that it won't fail as new code is added to the file or if the file is renamed). We have a helper method in rspec-core that assists with testing this. I recommend porting that to here (eventually, I suspect it'll go into the rspec-support gem we've discussed) and then you can test this like so:

it "includes the call site in the deprecation warning" do
  obj = Object.new
  expect_deprecation_with_call_site(__FILE__, __LINE__ + 1)
  obj.stub(:faces)
end

Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, I'll have an experiment and work it out.


it "includes the call site in the deprecation warning" do
obj = Object.new
expect_deprecation_with_call_site(__FILE__, __LINE__ + 1)
obj.stub(:faces)
end
end
end

Expand All @@ -122,6 +189,10 @@ def configure_syntax(syntax)
def configured_syntax
RSpec::Mocks.configuration.syntax
end

def configure_default_syntax
RSpec::Mocks.configuration.reset_syntaxes_to_default
end
end
end

Expand All @@ -142,6 +213,14 @@ def configured_syntax
end
end
end

def configure_default_syntax
RSpec.configure do |rspec|
rspec.mock_with :rspec do |c|
c.reset_syntaxes_to_default
end
end
end
end
end
end
Expand Down
6 changes: 6 additions & 0 deletions spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,9 @@ def reset(object)
RSpec::Mocks.instance_variable_set(:@configuration, orig_configuration)
end
end

def expect_deprecation_with_call_site(file, line)
expect(RSpec.configuration.reporter).to receive(:deprecation) do |options|
expect(options[:call_site]).to include([file, line].join(':'))
end
end