Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Make `any_instance` opt-in for RSpec 3.0 #336

Closed
myronmarston opened this Issue · 20 comments

8 participants

@myronmarston
Owner

any_instance is a code smell and is easily the most complicated part of rspec-mocks. I think it's an OK tool to use to get legacy code under test but I don't recommend it's use for new code (instead, your tests should be driving your design and encouraging you to allow dependencies to be injected).

Given that, it'd be good to have a way to disable any_instance all together when you're on a greenfield project. Beyond that, I think it's best to make any_instance require you to opt-in to make it available via a config setting, as that sends a signal that you should think before using it.

Thoughts?

/cc @alindeman @dchelimsky @JonRowe @samphippen @soulcutter

@JonRowe
Owner

Radical suggestion, let's remove it entirely.

We would probably have to open that up to a poll to see how many people would support that suggestion... perhaps more sensibly could we extract it into a separate gem? Then we can direct people to opt in to it, and we could remove the complexity from the main library?

@samphippen
Collaborator

@JonRowe I think that's too much change for the immediate future. Perhaps a removal in 4. I think making it optional, and disabled by default is a much smaller change for 3. @myronmarston: I assume this goes with a deprecation warning in 2-14 and 2-99

@alindeman
Collaborator

Radical suggestion, let's remove it entirely.

I have a little bit of bias, since I pushed for the feature to begin with but ... I think it's an important feature for wrapping legacy code with tests.

There's a lot of code out there where it's hard to inject dependencies. In my opinion, one valid and ideal use case for any_instance are tests that increase someone's confidence in legacy code enough to tease apart explicit dependencies and abstractions.

But I agree it's probably overused, and do favor educating folks about it.

I think we should consider enabling it by default in 3.0, but allowing it to be disabled ... and generating new spec_helper.rb files that have it disabled by default.

@dchelimsky
Owner

I was resistant to adding support for any_instance from day 1, but eventually gave in to community pressure based on the fact that Rails finders are model class methods. Conventional Rails usage requires that you stub a finder to return an instance that you can then stub or mock, which makes examples verbose:

comment = stub_model(comment, :id => 42)
Comment.stub(:find).with(42).and_return(comment)
comment.should_receive(:like).with(37)
put like, :id => 42, :user_id => 37

Conventionally, this results in (or is the result of) an implementation like this:

def like
  @comment = Comment.find(params[:id])
  @comment.like(params[:user_id])
end

any_instance simplifies this quite a bit:

comment = Comment.create
Comment.any_instance.should_receive(:like).with(37)
put like, :id => comment.id, :user_id => 37

This is not only simpler, but it is less brittle, allowing the particular finder to change.

Until a better solution to the problem of stubbing finders comes along (I tried a few years ago but in the end there were too many scenarios to try to cover to make it work reliably), I think it's important to keep any_instance around. I'd even leave it on as a default, and encourage people to use it sparingly in documentation, guides, blogs, etc.

@dchelimsky
Owner

I think it's an important feature for wrapping legacy code with tests.

This is not only about legacy code. Specs for conventional Rails code that gets written every day are simplified and better decoupled using any_instance.

I think we should consider enabling it by default in 3.0, but allowing it to be disabled ... and generating new spec_helper.rb files that have it disabled by default.

:+1: with a possible exception on the spec_helper for rspec-rails.

@alindeman
Collaborator

I think it's an important feature for wrapping legacy code with tests.

This is not only about legacy code. Specs for conventional Rails code that gets written every day are simplified and better decoupled using any_instance.

That's a good point. I think I forgot about that case a bit because I write less and less controller specs. I now agree it is an important consideration. You can't really initialize controllers on your own, and the ways to inject things into controllers is awkward and not widely used.

I think we should consider enabling it by default in 3.0, but allowing it to be disabled ... and generating new spec_helper.rb files that have it disabled by default.

:+1: with a possible exception on the spec_helper for rspec-rails.

Makes sense to me.

@JonRowe
Owner

I did say it was a radical suggestion ;)

@timlinquist

@dchelimsky I agree that it's a useful feature for that particular use case. The problem I have with that is that any_instance is widely used outside of controller tests. It's incredibly difficult to debug. I often find myself acking for any_instance when I'm seeing some strange behavior in a particular test/set of tests.

Perhaps another alternative is to include this behavior somehow with rspec-rails so it's really only applicable to Rails apps?

@alindeman I hadn't really ever thought about using any_instance to get legacy code under tests. That sounds like a really great use case for this feature. I wonder how often folks use it in that manner? Curiosity really; Imagine it's hard to gather that information.

I like the suggestion for allowing it to at least be configured. That gives users the control over their own code bases if they find it enough of a smell in their test suites.

@myronmarston

Circling back around on this...

It sounds like we're all on the same page about making it configurable. The main question is: is it enabled by default or disabled by default? Here's my vote:

  • Make it disabled by default (as in, don't even load the code for it).
  • rspec-rails can either enable it by default or generate a config like in spec_helper.rb that enables it.

One reason I'd like it disabled by default (at least in stand-alone rspec-mocks w/o rspec-rails) is that any_instance has a fair bit of code to support it, and I'm always looking for ways to improve the boot time of rspec. Loading less code at require 'rspec/mocks' time is a simple way to do that :). I also recommend not using any_instance when you're not dealing with legacy code or rails code -- so having the default reflect this is good, I think.

@xaviershay
Collaborator

Disable by default. We have strong opinions about it, they should be reflected in the library.

@JonRowe
Owner

One thing @dchelimsky has mentioned before, is that we are running a high risk of alienating developers with the changes we are making, I would love to disable this by default but I think doing so would further alienate our developers. How about we take a similar approach to the syntax configuration that @samphippen did, we leave it enabled but issue a warning asking people to explicitly turn it on or off.

@xaviershay
Collaborator

I can get onboard with that. Still allows us to be opinionated, doesn't actually break anything.

@soulcutter
Collaborator

Warning and asking for explicit configuration one way or another feels right to me if we change anything here. I do see it as a powerful feature used in moderation, and given its (over)use it seems drastic to remove it or even disable it by default.

Truthfully I'm not convinced we should change it at all.

@myronmarston
Owner

Hmm, I'm still on the fence, but you argue well :). I'll keep thinking about.

@xaviershay
Collaborator

hey y'all we need to make a call on this. @alindeman can you make that WIP into a PR?

@alindeman
Collaborator

Yah, I have a plane ride tomorrow. I'll give it a go then.

@alindeman
Collaborator

I made some progress, but not enough to ship tonight. I need to do some rspec-rails things instead for the next bit. If we still want to do this, I'll continue it next week.

@myronmarston myronmarston modified the milestone: RSpec 3
@xaviershay
Collaborator

Why is this a release blocker? I feel like we've decided not to disable by default, and it's now down to whether or not we warn for explicit configuration - neither of which option would be a breaking change.

@myronmarston
Owner

Why is this a release blocker? I feel like we've decided not to disable by default, and it's now down to whether or not we warn for explicit configuration - neither of which option would be a breaking change.

You're right, not sure what I was thinking. Changed.

@myronmarston

Gonna close this. We're not doing it for 3.0.

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.