Warn when allowances/expectations are set on any instance recorders. #433
Conversation
|
@myronmarston please review this when you can :) |
|
Why don't we just raise an error instead? |
The point is to notify the user that that's almost certainly not what they want (which a warning is sufficient for), not to totally disallow mocking or stubbing a particular kind of object (which an error would do). In #375 it was mentioned that rspec-mocks' own specs set mock expectations on the any instance recorder, so we'd have to change those, and it seems conceivable that an rspec extension gem could want to do the same thing. |
| if AnyInstance::Recorder === subject | ||
| RSpec.warning( | ||
| "`#{expression}(#{subject.klass}.any_instance).to` " << | ||
| "is probably not what you meant, it does not stub on " << |
There was a problem hiding this comment.
The use of stub here is confusing, as expect sets up a mock expectation.
There was a problem hiding this comment.
would you be ok with "it does not allow" and "it does not expect" as then I can just use the "expression" variable, otherwise I'll add another argument that passes the correct language from the correct places.
|
Looks good in general...nice work @samphippen :). |
|
So if it is what I want, I can't turn the warnings off. Probably not worth worrying about. Hope it's obnoxious enough that people won't use it. |
|
@xaviershay I worried about that briefly, but I think it's so edge case that it's fine. (It's only going to be us and/or 3rd party developers of extensions) in which case I think you just stub out RSpec.warning for that test. |
|
LGTM to me too so merging. |
Warn when allowances/expectations are set on any instance recorders.
I did have a couple comments above that seem worth addressing... |
|
Doh, somehow Github wasn't showing those in the thread but was showing "Looks good in general...nice work @samphippen :)." |
|
I guess I should refresh tabs more often... |
Closes #375