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.

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

Verify the ordering of spies. #439

Merged
merged 7 commits into from
Oct 23, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Enhancements:
* `instance_double` and `class_double` raise `ArgumentError` if the underlying
module is loaded and the arity of the method being invoked does not match the
arity of the method as it is actually implemented. (Andy Lindeman)
* Spies can now check their invocation ordering is correct. (Jon Rowe)

Deprecations:

Expand Down
12 changes: 6 additions & 6 deletions lib/rspec/mocks/matchers/have_received.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module Matchers
class HaveReceived
COUNT_CONSTRAINTS = %w(exactly at_least at_most times once twice)
ARGS_CONSTRAINTS = %w(with)
CONSTRAINTS = COUNT_CONSTRAINTS + ARGS_CONSTRAINTS
CONSTRAINTS = COUNT_CONSTRAINTS + ARGS_CONSTRAINTS + %w(ordered)

def initialize(method_name, &block)
@method_name = method_name
Expand All @@ -21,14 +21,14 @@ def matches?(subject, &block)
@block ||= block
@subject = subject
@expectation = expect
expected_messages_received?
expected_messages_received_in_order?
end

def does_not_match?(subject)
@subject = subject
ensure_count_unconstrained
@expectation = expect.never
expected_messages_received?
expected_messages_received_in_order?
end

def failure_message
Expand All @@ -50,7 +50,7 @@ def description
end
end

private
private

def expect
expectation = mock_proxy.build_expectation(@method_name)
Expand Down Expand Up @@ -84,9 +84,9 @@ def generate_failure_message
error.message
end

def expected_messages_received?
def expected_messages_received_in_order?
mock_proxy.replay_received_message_on @expectation, &@block
@expectation.expected_messages_received?
@expectation.expected_messages_received? && @expectation.ensure_expected_ordering_received!
end
Copy link
Member

Choose a reason for hiding this comment

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

I think it's a little confusing for this method to be called expected_messages_received? when it does more than that now. Maybe it should be renamed to something like expected_messages_received_in_specified_order? or something?


def mock_proxy
Expand Down
13 changes: 12 additions & 1 deletion lib/rspec/mocks/message_expectation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ def initialize(error_generator, expectation_ordering, expected_from, method_doub
@expected_received_count = expected_received_count
@argument_list_matcher = ArgumentListMatcher::MATCH_ALL
@order_group = expectation_ordering
@order_group.register(self)
@ordered = false
@at_least = @at_most = @exactly = nil
@args_to_yield = []
@failed_fast = nil
Expand Down Expand Up @@ -277,6 +279,11 @@ def expected_messages_received?
ignoring_args? || matches_exact_count? || matches_at_least_count? || matches_at_most_count?
end

def ensure_expected_ordering_received!
@order_group.verify_invocation_order(self) if @ordered
true
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully this is clearer @myronmarston?

Copy link
Member

Choose a reason for hiding this comment

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

Much clearer :).

It's unclear to me why you are returning true, though; this method is a command, not a query, and the caller shouldn't be treating the return value as meaningful (since it's not).

Copy link
Member Author

Choose a reason for hiding this comment

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

Because:

@expectation.expected_messages_received? && @expectation.ensure_expected_ordering_received!

is much nicer than:

if @expectation.expected_messages_received?
  @expectation.ensure_expected_ordering_received!
end
true

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.e. at some point this has to pretend it fits into the predicate pattern, because of how the matcher is implemented.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd rather the matcher be implemented like so:

return false if @expectation.expected_messages_received?
@expectation.ensure_expected_ordering_received!
true

I'm big on command/query separation, though, so I like pushing it through. YMMV :).

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'm big on command/query separation, but it's just pretending here (because its starting as a query) so I prefer the elimination of conditionals here.


# @private
def ignoring_args?
@expected_received_count == :any
Expand Down Expand Up @@ -462,11 +469,15 @@ def twice(&block)
# api.should_receive(:finish).ordered
def ordered(&block)
self.inner_implementation_action = block
@order_group.register(self)
@ordered = true
self
end

# @private
def ordered?
@ordered
end

# @private
def negative_expectation_for?(message)
@message == message && negative?
Expand Down
56 changes: 49 additions & 7 deletions lib/rspec/mocks/order_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,80 @@ module Mocks
# @private
class OrderGroup
def initialize
@ordering = Array.new
@expectations = []
@invocation_order = []
@index = 0
end

# @private
def register(expectation)
@ordering << expectation
@expectations << expectation
end

def invoked(message)
@invocation_order << message
end

# @private
def ready_for?(expectation)
@ordering.first == expectation
remaining_expectations.find(&:ordered?) == expectation
end

# @private
def consume
@ordering.shift
remaining_expectations.each_with_index do |expectation, index|
if expectation.ordered?
@index += index + 1
return expectation
end
end
nil
end

# @private
def handle_order_constraint(expectation)
return unless @ordering.include?(expectation)
return unless expectation.ordered? && @expectations.include?(expectation)
return consume if ready_for?(expectation)
expectation.raise_out_of_order_error
end

def verify_invocation_order(expectation)
expectation.raise_out_of_order_error unless expectations_invoked_in_order?
true
end

def clear
@ordering.clear
@index = 0
@invocation_order.clear
@expectations.clear
end

def empty?
@ordering.empty?
@expectations.empty?
end

private

def remaining_expectations
@expectations[@index..-1] || []
end

def expectations_invoked_in_order?
invoked_expectations == expected_invocations
end

def invoked_expectations
@expectations.select { |e| e.ordered? && @invocation_order.include?(e) }
end

def expected_invocations
@invocation_order.map { |invocation| expectation_for(invocation) }.compact
end

def expectation_for(message)
@expectations.find { |e| message == e }
end

end
end
end
9 changes: 8 additions & 1 deletion lib/rspec/mocks/proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,16 @@ module RSpec
module Mocks
# @private
class Proxy
SpecificMessage = Struct.new(:object,:message) do
def ==(expectation)
expectation.orig_object == object && expectation.message == message
end
end

# @private
def initialize(object, name=nil, options={})
def initialize(object, order_group, name=nil, options={})
@object = object
@order_group = order_group
@name = name
@error_generator = ErrorGenerator.new(object, name)
@expectation_ordering = RSpec::Mocks::space.expectation_ordering
Expand Down Expand Up @@ -139,6 +145,7 @@ def has_negative_expectation?(message)

# @private
def record_message_received(message, *args, &block)
@order_group.invoked SpecificMessage.new(object, message)
@messages_received << [message, args, block]
end

Expand Down
4 changes: 2 additions & 2 deletions lib/rspec/mocks/proxy_for_nil.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ module Mocks
# @private
class ProxyForNil < Proxy

def initialize
def initialize(order_group)
@warn_about_expectations = true
super nil
super nil, order_group
end
attr_accessor :warn_about_expectations
alias warn_about_expectations? warn_about_expectations
Expand Down
6 changes: 3 additions & 3 deletions lib/rspec/mocks/space.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ def proxy_for(object)
id = id_for(object)
proxies.fetch(id) do
proxies[id] = case object
when NilClass then ProxyForNil.new
when TestDouble then object.__build_mock_proxy
when NilClass then ProxyForNil.new(expectation_ordering)
when TestDouble then object.__build_mock_proxy(expectation_ordering)
else
PartialMockProxy.new(object)
PartialMockProxy.new(object, expectation_ordering)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/rspec/mocks/test_double.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ def respond_to?(message, incl_private=false)
end

# @private
def __build_mock_proxy
Proxy.new(self, @name)
def __build_mock_proxy(order_group)
Proxy.new(self, order_group, @name)
end

private
Expand Down
8 changes: 4 additions & 4 deletions lib/rspec/mocks/verifying_double.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ def initialize(doubled_module, *args)
__initialize_as_test_double(doubled_module, *args)
end

def __build_mock_proxy
VerifyingProxy.new(self,
def __build_mock_proxy(order_group)
VerifyingProxy.new(self, order_group,
@doubled_module,
InstanceMethodReference
)
Expand All @@ -47,8 +47,8 @@ def initialize(doubled_module, *args)
__initialize_as_test_double(doubled_module, *args)
end

def __build_mock_proxy
VerifyingProxy.new(self,
def __build_mock_proxy(order_group)
VerifyingProxy.new(self, order_group,
@doubled_module,
ObjectMethodReference
)
Expand Down
4 changes: 2 additions & 2 deletions lib/rspec/mocks/verifying_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ module Mocks
#
# @api private
class VerifyingProxy < Proxy
def initialize(object, name, method_reference_class)
super(object)
def initialize(object, order_group, name, method_reference_class)
super(object, order_group)
@object = object
@doubled_module = name
@method_reference_class = method_reference_class
Expand Down
22 changes: 22 additions & 0 deletions spec/rspec/mocks/matchers/have_received_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,28 @@ module Mocks
end
end
end

context 'ordered' do
let(:dbl) { double :one => 1, :two => 2 }

it 'passes when the messages were received in order' do
dbl.one
dbl.two

expect(dbl).to have_received(:one).ordered
expect(dbl).to have_received(:two).ordered
end

it 'fails when the messages are received out of order' do
dbl.two
dbl.one

expect {
expect(dbl).to have_received(:one).ordered
expect(dbl).to have_received(:two).ordered
}.to raise_error(/received :two out of order/m)
end
end
end

describe "expect(...).not_to have_received" do
Expand Down
27 changes: 27 additions & 0 deletions spec/rspec/mocks/order_group_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
require 'spec_helper'
describe 'OrderGroup' do
let(:order_group) { ::RSpec::Mocks::OrderGroup.new }

describe '#consume' do
let(:ordered_1) { double :ordered? => true }
let(:ordered_2) { double :ordered? => true }
let(:unordered) { double :ordered? => false }

before do
order_group.register unordered
order_group.register ordered_1
order_group.register unordered
order_group.register ordered_2
order_group.register unordered
order_group.register unordered
end

it 'returns the first ordered? expectation' do
expect(order_group.consume).to eq ordered_1
end
it 'keeps returning ordered? expectation until all are returned' do
expectations = 3.times.map { order_group.consume }
expect(expectations).to eq [ordered_1, ordered_2, nil]
end
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified with 3.times.map :).

Copy link
Member Author

Choose a reason for hiding this comment

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

So it could, hang over from initially building an until empty loop, but it broke when it failed ;)

end
end