Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Using `any_instance` mocking propagates accross contexts. #52

Closed
nathanvda opened this Issue · 20 comments

4 participants

@nathanvda

Hi If i have two tests like this:

it "should call the method" do
   SomeClass.any_instance.expects(:the_method)
   @my_test_object.a_method_that_calls_the_method
end

it "should reset the authenticator application data" do
   @my_test_object.a_method_that_calls_the_method
   @my_test_object.some_property.should == "something else"
 end

So in short: two test-blocks, where in the first i check if the method is actually called, and in the second i check for the effects of the complete method --including the previously stubbed method.

What happens then: in the second test the actual method is not called, by I get the error that the expectation was received again. So somehow, the any_instance sticks between tests, and I would expect it to last for only the current scope (test/context/ ...).

Is this the intended behaviour?

@dchelimsky
Owner

Are you using mocha or rspec for mocking? The example uses expects, which suggests you are using mocha.

@nathanvda

Good remark! We were using mocha, and i am now converting to rspec-mocks, I am using the version from git, and encountered that bug. The code I showed was trying to make it clearer (oops: and made it worse).

To make sure, here is the actual sample from code:

context "reset-failing" do
  before(:each) do
    @authenticator_application = Factory(:authenticator).authenticator_applications[0]
    @authenticator_application.blob = fixture_file('used_blob')
    @authenticator_application.save
  end

  it "should reset the authenticator application" do
    Vacman::DpBlob.any_instance.should_receive(:reset_token_info)
    @authenticator_application.reset_token_info
  end

  it "should reset the authenticator application data" do
    @authenticator_application.reset_token_info
    @authenticator_application.reload.blob_parameters[:error_count].should == 0
  end

end  

To make this context pass, i just need to reverse the two tests. I find that smelly :)

@nathanvda

Ok, i forked rspec-mocks, and was able to add failing tests. Now i hope to be able to fix those ;)
In the tests all tests are executed against a fresh klass.

@alindeman
Collaborator

Yikes, nice find. Do you need any help fixing it?

@kaiwren

I'll take a look at this over the weekend and try to get it sorted.

@dchelimsky
Owner

FYI - this is clearly a blocker for the 2.6.0 release.

@kaiwren kaiwren referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@kaiwren

I've got a failing test replicating this at c42engineering/rspec-mocks@805c2f8 - I've been unable to replicate what @nathanvda describes (I've taken the liberty of depending on spec ordering in an attempt to replicate this issue). I did however find something that might be causing this and I have a failing test for it. Please take a look - it's all under the when resetting after an example context. In the meanwhile I'll try to get that broken spec to pass.

@dchelimsky
Owner

I merged c42engineering/rspec-mocks@805c2f8 to a local branch and see the failure. I started looking into the problem and it looks like an even ordering problem, but I haven't gotten to the root of it yet. Essentially, @__recorder is set to nil before stop_observing_currently_observed_methods! is invoked, so a new instance of Recorder is generated that doesn't have the previously recorded messages. I'm going to keep poking because I'd like to clear this up so I can do this release, but feel free to comment here if you have any insights or are working on it as well.

@dchelimsky
Owner

The above diagnosis is partially wrong: @__recorder is not getting set to nil before stop_observing_currently_observed_methods!, but at the point that it is called, observed_method_names is empty.

@dchelimsky
Owner

Got it - restore_method was calling observed_method_names.delete(method_name) prematurely - fix coming shortly

@kaiwren

@dchelimsky Awesome. I was about to get started on fixing this when I saw your comments (I've been afk for a bit). Let me know if you need any input from me. Also, since you've been looking at my code in some detail, I would greatly value any feedback you may have if you can spare the time.

@dchelimsky
Owner

I'm actually heading out for the rest of the day, but will pick this up tomorrow.

@dchelimsky dchelimsky was assigned
@nathanvda

Just pushed my test that replicates my issue. I presume it is not up to standard, the style is quite different then the others. But i replicate the exact behaviour that i failed to work in my project. Thanks to the hint from @dchelimsky the fix is indeed very simple. Thanks guys, you all are amazing.

@dchelimsky: not sure if the code/test is good enough for a pull request?

@dchelimsky
Owner

I'm actually not satisfied w/ that solution yet - there is something deeper going on that I'm close to putting my finger on, but not there yet - planning to continue on this tomorrow. Out for real now.

@nathanvda

Great. I am looking forward to that. Thanks for your effort.

@kaiwren kaiwren referenced this issue from a commit
@kaiwren kaiwren Fixing issue #52 2840a76
@kaiwren

So it looks like I'd done a couple of dumb things while trying to get the never and any_number_of_times constraints working. I'll list what I realised and the changes I've made - let me know if I've missed anything:

  • rspec_reset is called on the class before it's called on any instances - consequently, by the time rspec_reset is called on an instance, @recorder is nil and is consequently created afresh
  • My original implementation dealt with expectations assuming any_instance behaves as every_instance; once we changed the semantics to exactly_one_instance a fair number of things became unnecessary and I'd missed removing all of these
  • I'd wrapped @observed_methods with a method named observed_method_names as an aid for refactoring at some point when working on never/any_number_of_times, then forgotten about it completely. This obscured the fact that stop_observing_currently_observed_methods! wound up having @observed_methods mutate itself while being iterated over.
  • I've now deleted all the code that dealt with cleaning up @observed_methods and playing unplayed expectations to every instance in scope as these are now unnecessary and the build is now green
  • I've verified that @nathanvda's specs duplicating his issue pass with my fixes but I haven't included them in my code

Question: I've left in place the specs that depend on ordering to verify that there are no dependencies (see first spec and second_spec) - should I leave them in place? Is there a good way to test this without our own specs having to be ordered?

There are a couple of other commits I've made on the issue52 branch on c42engineering/rspec-mocks that you may or may not wish to include:

  • I've added specs to this branch detailing the behaviour of any_instance as every_instance for stubs and renamed an existing context to better express the fact that any_instance behaves as exactly_one_instance for expectations
  • I've added specs ensuring that expectations set on a class using any_instance do not interfere with normal expectations set on the class

Let me know if this is satisfactory and I'll raise a pull request.

@dchelimsky
Owner

No pull necessary - I squashed your commits and merged them with your name listed as author.

I did follow up w/ a couple of refactoring/cleanup commits: ef65d3 and 189fda. Please comment on those commits if you have any questions.

@dchelimsky
Owner

ps - HUGE THANKS!

@dchelimsky dchelimsky reopened this
@dchelimsky dchelimsky closed this
@dchelimsky
Owner

Sorry about the open/close - when I pushed the aforementioned commits our new CI server told me that the build was failing against 1.9.1. Turned out to be a scoping issue in the spec, so it was a quick fix.

@kaiwren

Not at all - it was my mess to begin with. Thanks for taking the time to give me feedback - I do have questions (several of them once I started looking at all the clean-up you've done) - I'll probably catch you on IRC later today or tomorrow rather than spam you with notification mails from Github.

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.