Resolve ambiguity/underspecification of mocks and before(:all) #240

Closed
myronmarston opened this Issue Mar 15, 2013 · 23 comments

Comments

Projects
None yet
7 participants
@myronmarston
Member

myronmarston commented Mar 15, 2013

See #92 for some background on this.

We've never intended mocks/stubs to work when setup in before(:all). We should explicitly disallow this, or decide to what extent we support it (my vote is "none").

This is similar to the let/subject/before(:all) issue I recently solved in rspec-core.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Mar 15, 2013

Member

I agree it should be explicitly disallowed. The problem with supporting it is the inevitable confusion created by internal mock-system-related state changes in objects across examples. For example, a stub declared in before(:all) would return the same answer every time, but a message expectation would fail in the second example that runs unless the declaration said twice or at_least_once, etc. This would result in erroneous bug reports that are time consuming to manage and take time away from maintaining code and building new features.

Member

dchelimsky commented Mar 15, 2013

I agree it should be explicitly disallowed. The problem with supporting it is the inevitable confusion created by internal mock-system-related state changes in objects across examples. For example, a stub declared in before(:all) would return the same answer every time, but a message expectation would fail in the second example that runs unless the declaration said twice or at_least_once, etc. This would result in erroneous bug reports that are time consuming to manage and take time away from maintaining code and building new features.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Mar 18, 2013

Member

It's also possible to run into similar issues by setting consts to doubles e.g.

MyModel ||= double "my fat model", create!: my_instance

Is it possible to cleanup doubles after each test run so that they are specifically dereferenced? This would cause an error in this behaviour state and others where it's a similar 'pollution' problem...

Member

JonRowe commented Mar 18, 2013

It's also possible to run into similar issues by setting consts to doubles e.g.

MyModel ||= double "my fat model", create!: my_instance

Is it possible to cleanup doubles after each test run so that they are specifically dereferenced? This would cause an error in this behaviour state and others where it's a similar 'pollution' problem...

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 19, 2013

Member

We provide a nice API for setting a constant to a test double for a particular example now (in the form of stub_const), so I'd generally discourage doing things like that, and I'm not sure I'd want to expend significant effort in rspec-mocks to work around that case.

That said, I once ran into a problem where a test accidentally caused a class attribute to be set to a test double, which caused problems for the next test that accessed that class attribute, and at the time, I was thinking it would be nice to cause test doubles to "expire" after one example, where any method call on them causes them to raise an error saying "this test double was initially created at line X but has now expired". I think that's a different issue from the one hear but I'm very open to the idea.

Member

myronmarston commented Mar 19, 2013

We provide a nice API for setting a constant to a test double for a particular example now (in the form of stub_const), so I'd generally discourage doing things like that, and I'm not sure I'd want to expend significant effort in rspec-mocks to work around that case.

That said, I once ran into a problem where a test accidentally caused a class attribute to be set to a test double, which caused problems for the next test that accessed that class attribute, and at the time, I was thinking it would be nice to cause test doubles to "expire" after one example, where any method call on them causes them to raise an error saying "this test double was initially created at line X but has now expired". I think that's a different issue from the one hear but I'm very open to the idea.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Mar 19, 2013

Member

In general I was thinking about pollution between test cases, constants, class attributes, ENV, other singleton stuff, but also I wasn't aware of stub_const...

Member

JonRowe commented Mar 19, 2013

In general I was thinking about pollution between test cases, constants, class attributes, ENV, other singleton stuff, but also I wasn't aware of stub_const...

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 19, 2013

Member

See http://myronmars.to/n/dev-blog/2012/06/constant-stubbing-in-rspec-2-11 for more info on stub_const.

I think my idea of "expiring" constants will nicely for any way a double leaks outside the scope of one example. I'm not really sure what else we could do, anyway; ruby doesn't give you any sort of API for managing or querying object references.

Member

myronmarston commented Mar 19, 2013

See http://myronmars.to/n/dev-blog/2012/06/constant-stubbing-in-rspec-2-11 for more info on stub_const.

I think my idea of "expiring" constants will nicely for any way a double leaks outside the scope of one example. I'm not really sure what else we could do, anyway; ruby doesn't give you any sort of API for managing or querying object references.

@michihuber

This comment has been minimized.

Show comment
Hide comment
@michihuber

michihuber Mar 21, 2013

Contributor

I gave implementing a warning a try, but the way core and mocks (and potentially other libraries) play together would make it very awkward (@dchelimsky said as much in #92). before(:all) should be used sparingly (and there was some discussion about deprecating it), so I think a note in the docs should be enough.

Contributor

michihuber commented Mar 21, 2013

I gave implementing a warning a try, but the way core and mocks (and potentially other libraries) play together would make it very awkward (@dchelimsky said as much in #92). before(:all) should be used sparingly (and there was some discussion about deprecating it), so I think a note in the docs should be enough.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 21, 2013

Member

Thanks for giving that a shot, @michihuber. I'd still like to see a warning issued at runtime when users do this, but I think it'll require the expertise of someone who has worked in the rspec codebase for a while to figure out a clean way to do this. I'll try to give this a shot at some point.

Member

myronmarston commented Mar 21, 2013

Thanks for giving that a shot, @michihuber. I'd still like to see a warning issued at runtime when users do this, but I think it'll require the expertise of someone who has worked in the rspec codebase for a while to figure out a clean way to do this. I'll try to give this a shot at some point.

@michihuber

This comment has been minimized.

Show comment
Hide comment
@michihuber

michihuber Mar 25, 2013

Contributor

Uh, I got this issue mixed up with #92 and thought it was a year old, hence my odd comment. Sorry about that...

Contributor

michihuber commented Mar 25, 2013

Uh, I got this issue mixed up with #92 and thought it was a year old, hence my odd comment. Sorry about that...

@replaid

This comment has been minimized.

Show comment
Hide comment
@replaid

replaid Sep 23, 2013

We're using before(:all) more to help optimize test speed and had no idea about this. Lost time. 👍 on adding a warning message soon.

replaid commented Sep 23, 2013

We're using before(:all) more to help optimize test speed and had no idea about this. Lost time. 👍 on adding a warning message soon.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 23, 2013

Member

@replaid -- this is definitely slated for inclusion in RSpec 3.

Member

myronmarston commented Sep 23, 2013

@replaid -- this is definitely slated for inclusion in RSpec 3.

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Nov 2, 2013

Member

@myronmarston I did a little lightweight digging into this issue. RSpec::Mocks.space seems to be nil inside a before(:all) block. Is changing the behaviour of Syntax#stub (and related methods) to print a warning and then return if RSpec::Mocks.space is nil a valid approach to solving this?

Member

samphippen commented Nov 2, 2013

@myronmarston I did a little lightweight digging into this issue. RSpec::Mocks.space seems to be nil inside a before(:all) block. Is changing the behaviour of Syntax#stub (and related methods) to print a warning and then return if RSpec::Mocks.space is nil a valid approach to solving this?

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Nov 2, 2013

Member

Ok, so my investigation on RSpec::Mocks space only applies for the first spec. Oh well.

Member

samphippen commented Nov 2, 2013

Ok, so my investigation on RSpec::Mocks space only applies for the first spec. Oh well.

@skalee

This comment has been minimized.

Show comment
Hide comment
@skalee

skalee Dec 16, 2013

Although I agree on confusion danger with @dchelimsky's comment:

That would become very confusing if we ever decide to support stubs in before(:all) in the future. If anything we could add an implementation that raises an error explaining what is/is not supported.

I see the need for stubbing/mocking support in before :all. Consider following example utilizing FactoryGirl and Geocoder:

factory :place, traits: [:stub_geocoding_if_coordinates_already_present] do
  trait :stub_geocoding_if_coordinates_already_present do
    after(:build) do |object|
      if [object.latitude, object.longitude].all?(&:present?)
        object.stub(:geocode){ [object.latitude, object.longitude] }
      end
    end
  end
end

I found stubbing geocoder very useful when test is more related to coordinates than address. It speeds things up, it isolates geocoding logic so it can't affect tests where not related. That worked fine until some day, for performance reasons, I wanted to create an array of places in before(:all) block:

before :all do
  @places = [
    (create :place, latitude: 15.0, longitude: 15.0, other_param: 1),
    (create :place, latitude: 15.0, longitude: 15.1, something_else: 2),
    (create :place, latitude: 15.1, longitude: 15.1, whatever: 3),
  ]
end

It fails for aforementioned reasons.

Stubs are required only during object blueprinting, they may go away after that. What about introducing temporary RSpec::Mocks.space? What about clearing stubs and throwing that space away after before(:all)? And, if it is confusing, what about introducing yet another block-accepting method like with_temporary_stub_space or with_volatile_stubbing?

before :all do
  with_volatile_stubbing do
    @places = [
      (create :place, latitude: 15.0, longitude: 15.0, other_param: 1),
      (create :place, latitude: 15.0, longitude: 15.1, something_else: 2),
      (create :place, latitude: 15.1, longitude: 15.1, whatever: 3),
    ]
  end # stubs would be discarded at the end of #with_volatile_stubbing block
end

Such instantiation would work both in before :each and :all.

skalee commented Dec 16, 2013

Although I agree on confusion danger with @dchelimsky's comment:

That would become very confusing if we ever decide to support stubs in before(:all) in the future. If anything we could add an implementation that raises an error explaining what is/is not supported.

I see the need for stubbing/mocking support in before :all. Consider following example utilizing FactoryGirl and Geocoder:

factory :place, traits: [:stub_geocoding_if_coordinates_already_present] do
  trait :stub_geocoding_if_coordinates_already_present do
    after(:build) do |object|
      if [object.latitude, object.longitude].all?(&:present?)
        object.stub(:geocode){ [object.latitude, object.longitude] }
      end
    end
  end
end

I found stubbing geocoder very useful when test is more related to coordinates than address. It speeds things up, it isolates geocoding logic so it can't affect tests where not related. That worked fine until some day, for performance reasons, I wanted to create an array of places in before(:all) block:

before :all do
  @places = [
    (create :place, latitude: 15.0, longitude: 15.0, other_param: 1),
    (create :place, latitude: 15.0, longitude: 15.1, something_else: 2),
    (create :place, latitude: 15.1, longitude: 15.1, whatever: 3),
  ]
end

It fails for aforementioned reasons.

Stubs are required only during object blueprinting, they may go away after that. What about introducing temporary RSpec::Mocks.space? What about clearing stubs and throwing that space away after before(:all)? And, if it is confusing, what about introducing yet another block-accepting method like with_temporary_stub_space or with_volatile_stubbing?

before :all do
  with_volatile_stubbing do
    @places = [
      (create :place, latitude: 15.0, longitude: 15.0, other_param: 1),
      (create :place, latitude: 15.0, longitude: 15.1, something_else: 2),
      (create :place, latitude: 15.1, longitude: 15.1, whatever: 3),
    ]
  end # stubs would be discarded at the end of #with_volatile_stubbing block
end

Such instantiation would work both in before :each and :all.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 16, 2013

Member

Hmm, this is actually pretty reasonable request. The problem with using mocks or stubs in before(:all) has always been the weird ordering dependencies it can create in the individual examples if they rely on those mocks or stubs. This avoids that problem by limiting the lifetime of the mocks and stubs in before(:all) to just that block of code. I don't think I like the name suggests, though. Maybe something like RSpec::Mocks.with_temporary_scope { }?

We could also have this new method mentioned in the error message that will be issued when mocking or stubbing is used from before(:all) w/o wrapping it in this method.

Member

myronmarston commented Dec 16, 2013

Hmm, this is actually pretty reasonable request. The problem with using mocks or stubs in before(:all) has always been the weird ordering dependencies it can create in the individual examples if they rely on those mocks or stubs. This avoids that problem by limiting the lifetime of the mocks and stubs in before(:all) to just that block of code. I don't think I like the name suggests, though. Maybe something like RSpec::Mocks.with_temporary_scope { }?

We could also have this new method mentioned in the error message that will be issued when mocking or stubbing is used from before(:all) w/o wrapping it in this method.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Dec 17, 2013

Member

👍 for the temporary scope and then remove support for them at all in all blocks. (So the replacement is to use the new temporary stub scope, making it obvious)

Member

JonRowe commented Dec 17, 2013

👍 for the temporary scope and then remove support for them at all in all blocks. (So the replacement is to use the new temporary stub scope, making it obvious)

@skalee

This comment has been minimized.

Show comment
Hide comment
@skalee

skalee Dec 17, 2013

Yes, with_temporary_scope sounds much better, but I'm not fully convinced to this name. IMO all stubs are something temporary due to their nature, those in this new scope are only bit more temporary. But I can't invent anything better.

skalee commented Dec 17, 2013

Yes, with_temporary_scope sounds much better, but I'm not fully convinced to this name. IMO all stubs are something temporary due to their nature, those in this new scope are only bit more temporary. But I can't invent anything better.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Dec 17, 2013

Member

I'm skeptical of this for exactly the same reason I'm skeptical of supporting message expectations in before(:all): it increases complexity by offering a different life cycle for them. Personally, I'd sooner support a separate Mock::Space in the before/after :all scope. At least that expands the capability of existing scopes rather than adding a new scope to support what I see as an edge case.

Member

dchelimsky commented Dec 17, 2013

I'm skeptical of this for exactly the same reason I'm skeptical of supporting message expectations in before(:all): it increases complexity by offering a different life cycle for them. Personally, I'd sooner support a separate Mock::Space in the before/after :all scope. At least that expands the capability of existing scopes rather than adding a new scope to support what I see as an edge case.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 17, 2013

Member

I don't understand your alternate suggestion, @dchelimsky. Can you explain more?

Member

myronmarston commented Dec 17, 2013

I don't understand your alternate suggestion, @dchelimsky. Can you explain more?

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Dec 17, 2013

Member

Back when the idea of supporting stubs/mocks in before(:all) first came up, the solution I pondered was to attach a separate Mock::Space to the group scope, which would reset (i.e. clean up) implicitly after the group the same way the current single Mock::Space cleans up implicitly after each example. Not a slam dunk as it opens the door for a world of new problems e.g. what happens when you allow(@foo).to receive(:bar) in before(:all) and then expect(@foo).to receive(:bar) in an example.

Member

dchelimsky commented Dec 17, 2013

Back when the idea of supporting stubs/mocks in before(:all) first came up, the solution I pondered was to attach a separate Mock::Space to the group scope, which would reset (i.e. clean up) implicitly after the group the same way the current single Mock::Space cleans up implicitly after each example. Not a slam dunk as it opens the door for a world of new problems e.g. what happens when you allow(@foo).to receive(:bar) in before(:all) and then expect(@foo).to receive(:bar) in an example.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 17, 2013

Member

Right. I specifically don't want to allow any mocking or stubbing in before(:all) to affect any examples because it creates ambiguities and ordering dependencies. As a simple example (beyond yours), consider allow(MyClass).to receive(:foo).and_return(1, 2, 3, 4): if you have 4 examples in that group, and each calls MyClass.foo once, then what value is returned in a particular example depends entirely on the order the examples run in.

The reason I like @skalee's suggestion is because it avoids these problems. It doesn't allow you to setup a stub or method expectation in before(:all) that can affect any of the examples in the group.

Personally, I can't think of a case where mocking or stubbing something in before(:all) and having it affect the examples would be useful: if you are trying to reduce duplication, put the mock/stub setup in before(:each), and that has none of the problems before(:all) has. OTOH, I think the use case for @skalee's suggestion makes sense: you have some expensive operation you want to do in before(:all) but you need something stubbed while that runs.

Member

myronmarston commented Dec 17, 2013

Right. I specifically don't want to allow any mocking or stubbing in before(:all) to affect any examples because it creates ambiguities and ordering dependencies. As a simple example (beyond yours), consider allow(MyClass).to receive(:foo).and_return(1, 2, 3, 4): if you have 4 examples in that group, and each calls MyClass.foo once, then what value is returned in a particular example depends entirely on the order the examples run in.

The reason I like @skalee's suggestion is because it avoids these problems. It doesn't allow you to setup a stub or method expectation in before(:all) that can affect any of the examples in the group.

Personally, I can't think of a case where mocking or stubbing something in before(:all) and having it affect the examples would be useful: if you are trying to reduce duplication, put the mock/stub setup in before(:each), and that has none of the problems before(:all) has. OTOH, I think the use case for @skalee's suggestion makes sense: you have some expensive operation you want to do in before(:all) but you need something stubbed while that runs.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Dec 17, 2013

Member

I think the argument for supporting it in before(:all) is the same argument for having before(:all) at all: one-time set up for a series of expectations, each in its own example. Not saying it's without its problems, just explaining the motivation.

I agree this request makes sense. I just don't know if it belongs directly in rspec or as part of an extension library. It's an edge case, IMO, and will not be problem free e.g. somebody will likely expect the stubs to be there in spite of all best intentions of documentation. If the stub returns a canned value for an otherwise calculated value that ends up getting accessed later, either directly or indirectly through another method, the resulting examples will be error prone (i.e. false positives/negatives) and difficult to reason about.

Member

dchelimsky commented Dec 17, 2013

I think the argument for supporting it in before(:all) is the same argument for having before(:all) at all: one-time set up for a series of expectations, each in its own example. Not saying it's without its problems, just explaining the motivation.

I agree this request makes sense. I just don't know if it belongs directly in rspec or as part of an extension library. It's an edge case, IMO, and will not be problem free e.g. somebody will likely expect the stubs to be there in spite of all best intentions of documentation. If the stub returns a canned value for an otherwise calculated value that ends up getting accessed later, either directly or indirectly through another method, the resulting examples will be error prone (i.e. false positives/negatives) and difficult to reason about.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 17, 2013

Member

I just don't know if it belongs directly in rspec or as part of an extension library.

I think it makes sense to include it in rspec-mocks. Among other things, it could be useful for the sandboxing we do in rspec-core's specs to allow us to define example groups and examples and run them from within an example.

It's an edge case, IMO, and will not be problem free e.g. somebody will likely expect the stubs to be there in spite of all best intentions of documentation.

One thing I really like about this solution is that the user has to explicitly opt-in to this feature by wrapping their before(:all) code in RSpec::Mocks.with_temporary_scope { } (or someone similarly-named method). If we made this temporary scope automatically enabled for all before(:all) hooks, I agree, it could create confusion (as users may expect the stubs to continue to be available in the examples) but the explicit opt-in nature of this should mitigate that problem. Even if the user doesn't read the docs of with_temporary_scope, the fact that it accepts a block strongly suggests that the stubs only live for the duration of the block and will not be available outside of it.

Member

myronmarston commented Dec 17, 2013

I just don't know if it belongs directly in rspec or as part of an extension library.

I think it makes sense to include it in rspec-mocks. Among other things, it could be useful for the sandboxing we do in rspec-core's specs to allow us to define example groups and examples and run them from within an example.

It's an edge case, IMO, and will not be problem free e.g. somebody will likely expect the stubs to be there in spite of all best intentions of documentation.

One thing I really like about this solution is that the user has to explicitly opt-in to this feature by wrapping their before(:all) code in RSpec::Mocks.with_temporary_scope { } (or someone similarly-named method). If we made this temporary scope automatically enabled for all before(:all) hooks, I agree, it could create confusion (as users may expect the stubs to continue to be available in the examples) but the explicit opt-in nature of this should mitigate that problem. Even if the user doesn't read the docs of with_temporary_scope, the fact that it accepts a block strongly suggests that the stubs only live for the duration of the block and will not be available outside of it.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Dec 17, 2013

Member

Sounds like you're convinced, so my work (as advisor) is done.

Member

dchelimsky commented Dec 17, 2013

Sounds like you're convinced, so my work (as advisor) is done.

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