Skip to content

Mock proxies are reset twice at the end of each example #165

Closed
myronmarston opened this Issue Jul 22, 2012 · 5 comments

4 participants

@myronmarston
RSpec member

I was just looking around rspec-mocks a bit and noticed that the mock proxies are reset twice at the end of each example.

As a result, the mock proxies are reset twice for each example.

@dchelimsky, do you know why this is? Is it intentional? If we change things so it only happens once per example it probably won't make a noticeable perf difference, but it obviously won't be slower, and every little bit counts.

@dchelimsky
RSpec member

As long as there is a guarantee that it happens once, that should be fine.

@JonRowe
RSpec member
JonRowe commented Mar 16, 2013

I only did some quick experimentation, but it seems removing either call seemed to have undesired side effects (judging by the number of test failures), it's possible that we could introduce some check to exclude a double reset scenario (when core calls reset after a mock has been verified) but unless there is a significant performance gain this seems like a cleaner solution.

@dchelimsky
RSpec member

IIRC the reason is that even though things get automatically reset at the end, it is feasible verify or reset mid-example e.g.

it "does two things" do
  log = double
  thing = Thing.new(log)
  log.should_receive(:info)
  thing.do_something_that_logs_info
  log.rspec_verify
  # now log has been reset
  log.should_receive(:warn)
  thing.do_something_that_logs_warn
  log.rspec_verify
end

To remove the reset from verify, some of rspec's own examples would likely need to change. Then there's the risk that an app (unlikely) or an extension lib (less unlikely but still unlikely) calls verify early relying on the reset behavior. They are both marked private (so don't show up in the API docs on http://rubydoc.info), but that is fairly recent.

@xaviershay
RSpec member

We should add @dchelimsky's comment to the code base then close this out. If there are any perf improvements to be made here, we'll pick them up in profiling.

@xaviershay xaviershay was assigned Oct 1, 2013
@myronmarston
RSpec member

👍

@myronmarston myronmarston was assigned Jan 4, 2014
@myronmarston myronmarston added a commit that referenced this issue Jan 4, 2014
@myronmarston myronmarston Stop double-resetting mock proxies.
We don't need to reset the mock proxy while verifying it.
That'll happen later during the teardown phase.

It looks like the main reason we were doing this was
to assist with rspec-mocks testing itself; we would
verify the middle of an example to assert a mock expectation
failure, and a `reset` is needed so it doesn't fail again
when rspec-core calls `verify` as well.

The solution is to use spec helper methods that perform
the reset when we verify in the middle of an example.

I don't have any evidence this improves perf, but it
seems reasonable to assume that removing an extra method
call per example can only make it faster (however slight
the improvement may be).

Fixes #165.
6e51804
@myronmarston myronmarston added a commit that referenced this issue Jan 4, 2014
@myronmarston myronmarston Stop double-resetting mock proxies.
We don't need to reset the mock proxy while verifying it.
That'll happen later during the teardown phase.

It looks like the main reason we were doing this was
to assist with rspec-mocks testing itself; we would
verify the middle of an example to assert a mock expectation
failure, and a `reset` is needed so it doesn't fail again
when rspec-core calls `verify` as well.

The solution is to use spec helper methods that perform
the reset when we verify in the middle of an example.

I don't have any evidence this improves perf, but it
seems reasonable to assume that removing an extra method
call per example can only make it faster (however slight
the improvement may be).

Fixes #165.
c439349
@myronmarston myronmarston closed this in #519 Jan 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.