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.

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

Don't silently ignore arbitrary method expectations when combining them with 'and_call_original' #382

Merged
merged 3 commits into from Oct 5, 2013

Conversation

Projects
None yet
5 participants
@JonRowe
Member

JonRowe commented Jul 28, 2013

I had a discussion with @myronmarston about a problem I ran into during the conversion of our RSpec suite to the new expect/allow syntax. The gist of it can be found here.

Funnily what I thought was a bug, never quite worked the way I thought it was and I refactored the spec after reading his suggestions. Myron asked me to also add an issue for it, though. So here we go :-)

The spec we talked about had used the following expectation.

CachedUser.should_receive(:where) do |args|
    expect(args[:id]).to have(3).items
    expect(contact_ids).to include(*args[:id])
end.and_call_original

The values passed to 'where' are sort of random so you can't really setup an argument matcher with 'with'. That's why we tried to verify them in the block. The whole code under test there was also part of an AREL call comparable to this one. That's where the 'and_call_original' came into the game

CachedUser.where(id: random_related_user_ids).all

The spec passed. Though, as Myron pointed out, 'and_call_original' completely replaces the block expectation, resulting in the inner expectations never to be executed.

That was a bit surprising to find out, but to be honest also a fault on my side, since I probably never saw the expectation fail in the first place :-/

To make that behavior more obvious I think it would be good to either raise an exception or to output a warning in that case.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 28, 2013

Coverage Status

Coverage decreased (-0%) when pulling c05a378 on warn_when_overriding_implementation into 9202615 on master.

coveralls commented Jul 28, 2013

Coverage Status

Coverage decreased (-0%) when pulling c05a378 on warn_when_overriding_implementation into 9202615 on master.

@BjRo

This comment has been minimized.

Show comment
Hide comment
@BjRo

BjRo Jul 28, 2013

Awesome, thx!

BjRo commented Jul 28, 2013

Awesome, thx!

Show outdated Hide outdated lib/rspec/mocks/warning.rb Outdated
@@ -464,6 +467,7 @@ def initial_implementation_action=(action)
end
def inner_implementation_action=(action)
RSpec.warning("You're overriding a previous implementation for this stub") if implementation.inner_action

This comment has been minimized.

@myronmarston

myronmarston Jul 31, 2013

Member

I think RSpec.warn reads better (as its a verb) than RSpec.warning (as warning is a noun). Thoughts?

@myronmarston

myronmarston Jul 31, 2013

Member

I think RSpec.warn reads better (as its a verb) than RSpec.warning (as warning is a noun). Thoughts?

This comment has been minimized.

@JonRowe

JonRowe Oct 2, 2013

Member

This matches up with the stuff in rspec/rspec-core#1024, happy to have that discussion there :)

@JonRowe

JonRowe Oct 2, 2013

Member

This matches up with the stuff in rspec/rspec-core#1024, happy to have that discussion there :)

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 31, 2013

Member

In @BjRo's gist, it also came up that an expression like this:

      expect(CachedUser).to receive(:where) do |args|
        expect(args[:id]).to have(3).items
        expect(contact_ids).to include(*args[:id])
      end.and_call_original

...raises a confusing error (NoMethodError: undefined method 'and_call_original' for []:Array). It would be good to fix that as well.

Member

myronmarston commented Jul 31, 2013

In @BjRo's gist, it also came up that an expression like this:

      expect(CachedUser).to receive(:where) do |args|
        expect(args[:id]).to have(3).items
        expect(contact_ids).to include(*args[:id])
      end.and_call_original

...raises a confusing error (NoMethodError: undefined method 'and_call_original' for []:Array). It would be good to fix that as well.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 2, 2013

Coverage Status

Changes Unknown when pulling d8bca94 on warn_when_overriding_implementation into * on master*.

coveralls commented Oct 2, 2013

Coverage Status

Changes Unknown when pulling d8bca94 on warn_when_overriding_implementation into * on master*.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 2, 2013

Coverage Status

Changes Unknown when pulling 650bee8 on warn_when_overriding_implementation into * on master*.

coveralls commented Oct 2, 2013

Coverage Status

Changes Unknown when pulling 650bee8 on warn_when_overriding_implementation into * on master*.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 2, 2013

Coverage Status

Changes Unknown when pulling 650bee8 on warn_when_overriding_implementation into * on master*.

coveralls commented Oct 2, 2013

Coverage Status

Changes Unknown when pulling 650bee8 on warn_when_overriding_implementation into * on master*.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Oct 2, 2013

Member

Ready for a review I guess.

Member

JonRowe commented Oct 2, 2013

Ready for a review I guess.

Show outdated Hide outdated lib/rspec/mocks/warnings.rb Outdated
@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Oct 5, 2013

Member

Could final commit be squashed into another?

Member

xaviershay commented Oct 5, 2013

Could final commit be squashed into another?

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Oct 5, 2013

Member

Looks good otherwise when build is green.

Member

xaviershay commented Oct 5, 2013

Looks good otherwise when build is green.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Oct 5, 2013

Member

It's awaiting rspec/rspec-core#1024 (to sync the warning stuff)

Member

JonRowe commented Oct 5, 2013

It's awaiting rspec/rspec-core#1024 (to sync the warning stuff)

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 5, 2013

Coverage Status

Changes Unknown when pulling 894dd8b on warn_when_overriding_implementation into * on master*.

coveralls commented Oct 5, 2013

Coverage Status

Changes Unknown when pulling 894dd8b on warn_when_overriding_implementation into * on master*.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Oct 5, 2013

Member

Actually I think we can merge this now and sync up the warnings stuff later if need be

Member

JonRowe commented Oct 5, 2013

Actually I think we can merge this now and sync up the warnings stuff later if need be

module RSpec
# We don't redefine the deprecation helpers
# when they already exist (defined by rspec-core etc)

This comment has been minimized.

@xaviershay

xaviershay Oct 5, 2013

Member

good

@xaviershay

xaviershay Oct 5, 2013

Member

good

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Oct 5, 2013

Member

Yeah I don't see a reason to gate this on the core PR. Worst case scenario we just need to come back here and change it again if core PR changes, that's not terrible.

Member

xaviershay commented Oct 5, 2013

Yeah I don't see a reason to gate this on the core PR. Worst case scenario we just need to come back here and change it again if core PR changes, that's not terrible.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 5, 2013

Coverage Status

Changes Unknown when pulling 659b039 on warn_when_overriding_implementation into * on master*.

coveralls commented Oct 5, 2013

Coverage Status

Changes Unknown when pulling 659b039 on warn_when_overriding_implementation into * on master*.

JonRowe added a commit that referenced this pull request Oct 5, 2013

Merge pull request #382 from rspec/warn_when_overriding_implementation
Don't silently ignore arbitrary method expectations when combining them with 'and_call_original'

@JonRowe JonRowe merged commit f434af5 into master Oct 5, 2013

1 check passed

default The Travis CI build passed
Details

@JonRowe JonRowe deleted the warn_when_overriding_implementation branch Oct 5, 2013

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Oct 5, 2013

Member

This looks good, but did we address the other odd error (NoMethodError: undefined method 'and_call_original' for []:Array) I mentioned above ?

Member

myronmarston commented Oct 5, 2013

This looks good, but did we address the other odd error (NoMethodError: undefined method 'and_call_original' for []:Array) I mentioned above ?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Oct 5, 2013

Member

Hmm, no, but thats a block capture issue... e.g.

expect(user).to receive(:where) do |args|
  expect(args[:id].size).to eq 3
end.and_call_original

produces the weird error

expect(user).to( receive(:where) do |args|
  expect(args[:id].size).to eq 3
end.and_call_original )

Works fine.

Member

JonRowe commented Oct 5, 2013

Hmm, no, but thats a block capture issue... e.g.

expect(user).to receive(:where) do |args|
  expect(args[:id].size).to eq 3
end.and_call_original

produces the weird error

expect(user).to( receive(:where) do |args|
  expect(args[:id].size).to eq 3
end.and_call_original )

Works fine.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Oct 5, 2013

Member

Ah...I know what's going on: that expression passes the block to to due to ruby's precedence rules. So and_call_original is being called from the return value of to. which is the return value of handle_matcher, which returns whatever matcher.matches? returns, which, in this case, is returning @recorded_customizations, which is an array:

@recorded_customizations.each do |customization|
customization.playback_onto(expectation)
end

So, I think if we changed RSpec::Mocks::Matchers::Recive#setup_method_substitute to return expectation, it would allow further chaining. Want to take a stab at writing a failing spec, adding that, and seeing if it passes?

Member

myronmarston commented Oct 5, 2013

Ah...I know what's going on: that expression passes the block to to due to ruby's precedence rules. So and_call_original is being called from the return value of to. which is the return value of handle_matcher, which returns whatever matcher.matches? returns, which, in this case, is returning @recorded_customizations, which is an array:

@recorded_customizations.each do |customization|
customization.playback_onto(expectation)
end

So, I think if we changed RSpec::Mocks::Matchers::Recive#setup_method_substitute to return expectation, it would allow further chaining. Want to take a stab at writing a failing spec, adding that, and seeing if it passes?

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