stubs/expectations hide false positives

[se3000]

Mocks and stubs don't alert you when mocking/stubbing a method that the object doesn't respond to. This can cover up places in the code where methods are being called that are no longer defined. In my experience, it is unrealistic to rely on integration tests to catch this kind of error... Mocks and stubs are written far more frequently than integration tests.

I recently wrote https://github.com/se3000/better_receive as an add on to RSpec to cover this case and have gotten good results and heard many appreciative stories from the people that use it.

Is this a feature you would pull in if I wrote a pull request for it?

Options I've considered:

• an alternate syntax for mocks/stubs with this assertion (ex: foo.better_receive :bar)
• and/or a config setting that makes this assertion the default when mocking/stubbing

[myronmarston]

I'd definitely like to get something in for rspec 3. You can see some previous work I did on this in #15 and also the conversation on the mailing list that led to that.

@xaviershay created rspec-fire awhile ago, and I'm a huge fan. I'm currently leaning towards integrating it (or something like it) in rspec 3. There's already been a discussion around moving its functionality into rspec-mocks in xaviershay/rspec-fire#18.

One thing I love about rspec-fire over the approach of better_receive (and tools like it) is that better_receive (as I read it, anyway) only works on partial mocks. I think that from a design perspective, you're far better served to use full-on test doubles, and rspec-fire gives you that interface-verification benefit while allowing you to use pure test doubles. That also allows you to run isolated specs w/o the dependency loaded. It also does arity checking, which is quite nice.

So, in rspec 3 I'm thinking I'll probably roll in rspec-fire somehow, and possibly also provide a config option for the same kind of interface verification when doing partial mocking.

[alindeman]

One thing I love about rspec-fire over the approach of better_receive (and tools like it) is that better_receive (as I read it, anyway) only works on partial mocks.

+1. I only use partial mocks in very specific circumstances these days. Usually, I try to stub and mock objects playing a specific role, rather than being used just because they are instances of a specific class. In those cases, pure test doubles are preferable to partial mocks (IMHO).

Occasionally, I'll write a shared example group that describes the role (e.g., the messages you're expected to respond to) and run them against the test doubles and the real objects playing those roles. This provides verification of the interfaces in a pretty elegant way too.

In fact, the portion of my career where I was often using partial mocks, I wrote some pretty terrible code :) I found it did not force me toward smaller objects and encouraged me to mock simply to gain performance improvements in the test suite without addressing systemic design problems.

[se3000]

So, in rspec 3 I'm thinking I'll probably roll in rspec-fire somehow, and possibly also provide a config option for the same kind of interface verification when doing partial mocking.

Great to here!

I think rspec-fire is awesome. Pulling it into RSpec 3 would be super helpful, but if you don't also check partial mocks it causes a difference in behavior when mocking a double as opposed to a "concrete object." Partial mocking is still widely used, systemic design problems or not. Checking the API on partial mocks would provide a consistent safety net.

Those mailing-list/issue discussions are insightful, and give me some ideas for better_receive. I was already planning on doing arity checks, but I'd gotten some feedback suggesting to make this check a first class citizen in RSpec. It seemed more worth while to put the effort towards RSpec instead of a separate library if possible. Is there a timeline for RSpec 3? If I can help, I'd be happy to help out with responds_to config options and getting this feature out the door.

[myronmarston]

I don't yet have a definite timeline for RSpec 3, but I hope to get started on it soon. Before starting it I'm trying to deal with all the outstanding github issues by fixing those that I can, or tagging things for rspec-3 if it needs to wait until then. Once the remaining issues (that have not been tagged with rspec-3) approaches 0, I plan to start on rspec 3. I plan to blog about it and announce it. Stay tuned.

[JonRowe]

I like this idea, but I was confused by 'mock' in the title of this. I think there's some scope for cleaning up the distinction between test doubles/mocks and method stubs / expectations...

[jfelchner]

@alindeman while it's certainly true that in a perfect world, it would be nice to say "I should be able to send ANY object into this method as long as it responds to foo", what I've personally found is that I'm bit more often by the API of my double and the API of the object that I'm almost always using getting out of sync.

Performing a partial mock on an object drastically mitigates that problem IMO.

Like anything, it's a tool and if one is using it just for performance improvements without fixing design issues, it's going to cause problems. Just as 2,000 acceptance tests can be slow and brittle. It doesn't mean acceptance tests are bad, just that they are being used incorrectly.

I guess all that to say that I would love to see RSpec be able to partially mock (with verification) an object. See my proposed DSL here

[JonRowe]

@jfelchner this is something @myronmarston and the rest of us will be looking at for V3 keep your eyes peeled for the announcement blog post ;)

[jfelchner]

@JonRowe I'd love to start getting more involved. If there's something I can do, please let me know. Even if it's just documentation or issue triage.

[myronmarston]

So here's what I have in mind for RSpec 3:

Partial Mocks

For partial mocks, I was thinking we'd add a config option:

RSpec.configure do |rspec|
  rspec.mock_with :rspec do |mocks|
    mocks.verify_partial_mocks = true # not in love with this name, but you get the point
  end
end

With this option set to true, rspec-mocks will not allow you to set a message expectation or a stub on a partial mock that would change the object's interface in any way.

• It'll verify that the method responds to the given message.
• When you use with to match arguments, it'll verify that the arity of your with expression is within the arity range of the method. (I say "arity range" because default arguments and splats often make arity a range and not a single value).
• When the message is received, it'll also do the arity check at that time. This is so that if you don't use a with expression (e.g. some_object.stub(foo: 5)), it'll still verify the passed arguments are of an appropriate arity -- otherwise, the system-under-test could call some_object.foo("too", "many", "arguments").

Test Doubles

Pure test doubles, on their own, do not provide rspec-mocks enough metadata to know what interface to verify against. I'm a fan of rspec-fire's approach to solving this problem and plan to port a version of it to rspec-mocks.

New double declaration methods:

  # pass the class name as a string if you want this spec file
  # to be runnable in isolation without `EmailNotifier` loaded.
  email_notifier = class_double("EmailNotifier")

  # ... or pass the class constant itself, as that'll give you a clear error ir you misspell it
  email_notifier = class_double(EmailNotifier)

  # `instance_double` can take a class constant or string name as well
  another_double = instance_double(SomeOtherClass)

• class_double creates a test double that will verify its interface against the methods (not the instance_methods) of the named class.
• instance_double creates a test double that will verify its interface against the instance_methods (not the methods) of the named class.
• You can pass either the class constant or the class name as a string. The advantage of passing the string is it allows the spec to be run in isolation w/o the dependency loaded. rspec-mocks will only attempt to verify stubs and message expectations if the named constant is loaded. If the class will always be loaded (e.g. because you require it from the implementation file you are testing), you're probably better off passing the class constant, as that will protect you against misspellings (if you misspell the class name, it'll never get verified against a real interface, as rspec will never be able to find said interface).
• As with partial mocks, it'll verify both that the method is defined on the real interface and the arity. In addition, it'll verify the arity when the message is received (again, as with partial mocks).
• If you declare it as a null double (e.g. class_double("ClassName").as_null_object), it'll only respond to all the methods the interface supports, rather than any message (like a normal double does).
• class_double will integrate nicely with constant stubbing. class_double(ClassName).as_stubbed_const will create the class test double and replace the ClassName constant with the double for the duration of the test.

How this will differ from rspec-fire:

• rspec-fire doesn't do anything to as_null_object, so that still makes the double respond to every message.
• rspec-fire only supports passing the class name as a string. I've found myself passing ClassConstant.name in order to ensure I didn't misspell it.
• rspec-fire doesn't verify the arity of messages when they are received. (This is the source of Arity on stubbed methods xaviershay/rspec-fire#29).
• rspec-fire hooks into should_receive and stub and thus doesn't work with the new expect-based syntax. I didn't mention it above, but this will, obviously :).

So...that's what I have in mind for RSpec 3. Feedback welcome!

BTW, I'll be linking to this from the "The plan for RSpec 3" blog post I'm working on now, so I expect this'll get more eyes on it after I post that.

[jfelchner]

@myronmarston this looks great. After your explanation of how class_double and instance_double work, I see why they are needed. I think that they are perfect plumbing commands, but I'd love to see some porcelain on top that will do the right thing 80-90% of the time.

Also, I know you're against partial mocks, but after thinking more on it, there are times when passing an object in (DI) just isn't something that I want to do. I'm sure you'll agree that there are cases (even more prevalent with those who are just getting started with mocking, in which case they don't even know what DI is) where the verbosity cost of DI isn't worth it. I'm not saying that you should coddle to that case, I'm just asking you to please keep it in the back of your mind.

Of course this may not even be possible without monkey-patching the partial mock in which case I don't think that cost justifies the benefit of being able to do partial mocks.

[xaviershay]

there are times when passing an object in (DI) just isn't something that I want to do

concrete example?

[xaviershay]

Also @myronmarston that looks great!

[jfelchner]

@xaviershay nothing that isn't pedantic for contrived. 😄 I know that it has happened to me in the past. Even without an example, DI is a really, really "Good Thing" but as with anything there are tradeoffs.

One general example is if I have a class with a private method and that method is referencing another class in my library which is also private, all the extra noise required to:

• pass that object into a constructor
• verify it is passed in
• if it's not, set a default
• then replace the class reference with an instance variable reference (which is slightly less clear IMO)

all that is not worth the small amount of coupling to a class which, if it changes has a low amount of side effects. In addition to that, if you're doing DI just to make testing easier, you're probably going to be setting a default (which you want to do in most situations since it makes the API cleaner), and therefore, you're still coupling the class you're injecting into with the class you're injecting.

class MyLibrary::Injected
  def process_the_things(*things)
    # Process Code
  end
end

class InjectUntoMe
  attr_accessor :injected_class

  def initialize(desired_injected = MyLibrary::Injected)
    self.injected_class = desired_injected
  end

  def process(*things)
    injected_class.process_the_things(*things)
  end
end

vs

class InjectUntoMe
  def process(*things)
    MyLibrary::Injected.process_the_things(*things)
  end
end

I think that this case happens enough that we should try to handle it if we can. (I use "we" very loosely in this context since I probably won't be writing any of this. 😉)

[myronmarston]

Also, I know you're against partial mocks

I'm not against partial mocks and have never said I am. I just prefer full-on test doubles. There are situations where I still use partial mocks.

[jfelchner]

@myronmarston I didn't mean to misrepresent you. Sorry about that. I tend to write more in black and white terms when my intention is to be more gray. I'll try to watch out for that in the future.

[myronmarston]

@myronmarston I didn't mean to misrepresent you. Sorry about that. I tend to write more in black and white terms when my intention is to be more gray. I'll try to watch out for that in the future.

No worries :).

[xaviershay]

@jfelchner in that example I'd do the latter case and double it like class_double('MyLibrary::Injected', process_the_things: true).as_stubbed_const.

I don't see why you would want to use a normal double(process_the_things: true) in this case, but even if you did you could still use .as_stubbed_const.

"traditional" DI versus using a validated double are orthogonal concerns. This proposal only cares about the latter. Sorry but I still don't quite understand your argument :/

[jfelchner]

@xaviershay I think it just goes to show my ignorance and confusion in what all of the different options do since I completely missed the purpose of as_stubbed_const, which, you're right, is exactly what I want in that case.

So I think at this point, functionality-wise, what @myronmarston outlined looks perfect to me. Sorry to take you both the long way around on this. The only other thing I'll say is: I have what I would consider to be an intermediate knowledge of this stuff and the API still caused me quite a bit of confusion, I'd wager a beginner to mocking would be even more confused.

[xaviershay]

Ah, glad we're on the same page now :)

Point taken re beginners, though I suspect better documentation and intro examples are what is needed. I don't see a more obvious way to communicate the concepts in code at the moment. (Would love to be proven wrong though!)

[myronmarston]

@jfelchner in that example I'd do the latter case and double it like class_double('MyLibrary::Injected', process_the_things: true).as_stubbed_const

While I think the class_double/constant-stubbing combination is pretty useful and provides another mechanism for facilitating isolation testing that we didn't have before, I think a more standard DI approach is generally to be preferred, and I plan to document as_stubbed_const to recommend that it be used with care, and that simpler approaches be used when possible. Here's my thinking here:

• Tests are meant to provide design feedback, and constant stubbing is a tool that requires zero changes to your implementation, and thus can easily be used as a way to hide/silence the design feedback your tests are providing.
• To me, the point of using DI isn't just the improved testability it provides or the theoretic "now I can pass a different implementation for this collaborator" benefit. DI also makes a class easier to reason about (by surfacing the dependencies in the constructor; code that has hidden dependencies is harder to reason about than code that has an explicit listing of what it requires to work), and makes the class inherently less coupled to the rest of the system, which increases the likelihood that the code could be extracted into a library someday or copied into another project w/o needing significant modification. When you rely on constant stubbing as a round-about way of doing DI, you remove all these benefits.
• Constant stubbing is, to a lot of folks, "deep ruby magic". To understand what as_stubbed_const does, you need a moderate-to-expert understanding of ruby (specifically, the fact that constants are resolved at runtime at the call site and can be changed fluidly). In contrast, vanilla DI (i.e. using an optional argument to initialize) requires zero special ruby knowledge. If you've done isolated testing in other languages, you'll immediately understand/recognize it. This can be particularly important when working with teammates who are new to ruby.

In spite of all my caution above, I think that constant stubbing (and particularly the class_double/as_stubbed_const combination) are extremely useful and worth having in rspec-mocks, for situations like these:

• You don't control the instantiation of the your object-under-test. (This is commonly the case when testing things like rails controllers -- rails controls the initialize method, not you).
• In those relatively rare situations where you need to mock or stub something in an integration or acceptance test. While mocking or stubbing in a non-isolated test is generally a code smell, there are cases where there's no other way to simulate a particular situation, and in such cases you're not directly instantiating and exercising one object, so standard DI isn't very doable.

(I'm sure there are other good uses of it I haven't thought of, too).

I don't see why you would want to use a normal double(process_the_things: true) in this case, but even if you did you could still use .as_stubbed_const.

You won't be able to use double with .as_stubbed_const. double doesn't have any metadata about what class you are doubling and thus won't know what constant to stub.

On a side note, I'm thinking that as_stubbed_const is perhaps not the right name. It has nice a nice symmetry with as_null_object, but as_null_object fundamentally alters the character of the test double (e.g. making it respond to everything like a null object), where as as_stubbed_const changes the constant so that it refers to the double but doesn't change the double itself at all. Any thoughts on a better name?

[aaronjensen]

It doesn't looke likt it's been mentioned, so I'll mention it here. there's some pretty interesting stuff in this project: https://github.com/psyho/bogus

[ryanong]

I finished this a while ago. A complete re-write of stubbing and mocking.

https://github.com/ryanong/spy

I basically re-implimented all of rspec-mocks but in less than 1000 lines of code.

[myronmarston]

@ryanong -- I wasn't aware this was a fewest LOC contest, but if it is, I imagine @rkh could write almost-rspec-mocks in < 20 LOC :).

[ryanong]

Never meant to be a loc contest. Just check it out! There are some neat ways of handling the false positives, global object pollution. I loved rspec-mocks but I've run into too many false positive tests because of it too. So I tried to make a similar tool that would fire more errors. Check out the way I create mock objects to prevent stub objects bein only fantasies!

Also it isn't almost rspec. I copied the rspec mock tests over and made sure mine passed. The only thing that isn't there is constant inheritance.

[xaviershay]

@ryanong re preventing fantasy stub objects, please explicitly compare your approach to that of rspec-fire, because from a quick look at your code I'm pretty sure it isn't as powerful (doesn't allow doubles to be run in isolation).

[xaviershay]

@myronmarston re naming of as_stubbed_const, it's weird because having a method that changes global state as an instance method of a double is kind of wrong. Maybe this?

my_double = replace_constant("Blah", my_method: true)

Though maybe you'd need:

replace_constant_with_instance_of("Blah")
replace_constant_with_class("Blah")

Which is accurate but kinda long.

[myronmarston]

@aaronjensen -- Bogus does indeed look nifty. I've skimmed it and the contract tests in particular seems really cool. I don't have any experience with it yet, though.

@xaviershay -- I think you're right that as_stubbed_const is misplaced from a pure OO perspective. I think I still like it better than the alternatives we've discussed, though. When benefit of it is that it supports options for the constant stubbing...specifically, transfer_nested_constants: true. For now, let's go with as_stubbed_const unless someone comes up with a better API.

[pd]

@xaviershay @myronmarston -- the replace_constant naming seems much nicer, at least in its short form. How about piggybacking on class_double and instance_double:

my_double = replace_constant('Blah', class_double('Blah', my_method: true))

The duplication of 'Blah' may be a bit unsightly. And it seems odd to be able to replace constant 'SomeClass' with a class double of 'SomeOtherClass', tho I don't think that would be an issue in practice.

This interface would retain the ability to support options for how the constant is replaced:

my_double = replace_constant('Blah', class_double('Blah'), transfer_nested_constants: true)

[myronmarston]

@pd: replace_constant looks like an alias of stub_const, as it would behave exactly the same. I suppose we could consider just directing people to use stub_const...

[xaviershay]

Duplicating the class name is unacceptable to me, that's why as_stubbed_const exists in the first place :)

@myronmarston you could always do an optional double hash thing:

replace_const('Blah', {transfer_nested_constants: true}, my_method: true)

(I don't know if I like this, but it's an option.)

I haven't really through it through, is there any reason you wouldn't want to transfer constants? Could we just make it non-optional? Maybe performance I guess.

[xaviershay]

Added bonus: given most of the time you're replacing a constant with another class, make that the default behaviour of replace_const and add an instance: true option. Maybe getting a bit clever.

[xaviershay]

Throwing out some other options I don't like in case they inspire something:

replacing_instance_double('MyClass')
replacing_class_double('MyClass')

replacing.instance_double('MyClass')
replacing(transfer_nested_constants: true).instance_double('MyClass')

You could "mix" options into the regular methods hash, then provide the two hash option if you actually want to stub it as a method.

replace_const('Blah', instance: true, my_method: true)

replace_const('Blah', my_method: true).with_class # Default
replace_const('Blah', my_method: true).with_instance
replace_const('Blah', my_method: true).transferring_nested_constants.with_instance

Not sure if you could get it to work without an explicit kicker method though (default case).

This is maybe my favourite of this batch:

replace('MyClass').with(some_fake_object)
replace('MyClass').with_instance_double(my_method: true)
replace('MyClass').transferring_nested_constants.with_class_double(my_method: true)

[myronmarston]

I haven't really through it through, is there any reason you wouldn't want to transfer constants? Could we just make it non-optional? Maybe performance I guess.

This goes back to an original comment from @pat from over a year ago. I was originally planning on having it always transfer constants but @pat expressed a preference for not transferring them by default. I thought that the opt-in transfer_nested_constants option read more naturally than the inverse, and decided to go with that.

Since then, however, I've become kinda annoyed that the constants aren't transferred by default, and have thought about maybe making them do so in RSpec 3. I'm of a split mind about it, honestly.

Added bonus: given most of the time you're replacing a constant with another class, make that the default behaviour of replace_const and add an instance: true option. Maybe getting a bit clever.

What would the instance: true option do?

replacing_instance_double('MyClass')

Likewise, what would this do? I understand what replacing/stubbing a constant with a class double is for...but doing so with an instance_double doesn't make sense to me: are you saying that this would replace the MyClass constant with a double that that validates against the MyClass instance interface?

[xaviershay]

Yes, instance: true would replace with an instance_double. The theoretical use case for this would be replacing:

MyValidator = Validator.new

... but maybe you never actually should do that. In my code I can't find any instance of me doing this (always it's a class_double, and github search doesn't find any usages of anything.

I haven't used or felt the need to use transfer_nested_constants at all.

[ryanong]

@xaviershay What do you mean by run doubles in isolation? like within a block? or within a test?

Or do you mean that when I create a double I effect the original Class?

The way I create double is I actually create a new Class object that inherits from the Class you want to create a double from

https://github.com/ryanong/spy/blob/master/lib/spy/mock.rb#L41

Then I include the Spy::Mock to this new class which will overwrite all the methods of the given class with a raise warning.

You can actually re-use this entire "Mock Class" quite simply.

class Book
  AUTHOR = "Steinbeck"
end
MockBook = Spy::Mock.new(Book)
mocked_book1 = MockBook.new
mocked_book2 = MockBook.new
MockBook::AUTHOR == "Steinbeck" # You still inherit constants!

Also there is a lot of discuss about the usage of replace constants and transferring nested constants. Do you use this feature often? When developing Spy I did a search on google and github and couldn't find any projects that used it. And if you create class_doubles the way I did in spy I think it covers most use cases especially with #and_call_through

[xaviershay]

@ryanong I mean without the class you are mocking even loaded. See the rspec-fire README.

[ryanong]

@xaviershay Ah this is where I think we have different opinions. I originally let that happen in my first version but I changed it so I got less false positives.

Lets pull up your class_double example in your readme.

So we are stubbing this EmailNotifier class. Lets say we change the real method name #notify to #mail but not the name in this test. This test would pass. I know it is slower loading classes but I prefer less false positives to speed.

[JonRowe]

You misunderstand @xaviershay's point.

If you use class_double "EmailNotifier" when you've loaded EmailNotifier it will verify the double matches EmailNotifier, which is the same behaviour as you are attempting to achieve.

If you haven't loaded the EmailNotifier class (for example, you run an individual spec) it will behave like a normal double.

This allows you to rapidly develop one class, running only it's tests, with isolated unit tests, but then have the security of full verification when you run the entire suite.

As a side effect it also allows outside in TDD, as you can create calls to methods that don't yet exist, yet this will cause a failure when you move down into the class.

[ryanong]

Ah thanks @JonRowe that is pretty neat. I don't like letting test call methods that don't exist yet but I can see with enough discipline it being quite useful.

[JonRowe]

Outside in is often used by people doing more BDD style code than TDD, as you often start by describing what you want code to be and look like, and using that to dictate the design, rather than letting your existing code, or preconceptions about how something should work bubble up from the inside.

[ryanong]

I've used it before. I use it particularly with read me development. (How I designed the Spy API) http://tom.preston-werner.com/2010/08/23/readme-driven-development.html

However whenever I hit that point at which I have to create a new method or interface I make that new api before I continue with what my dream api. This is to make sure my dream API is actually viable. Devil in the details kind of thing. I will probably add this feature into Spy when I get the chance. Keep on rocking it.

[jfelchner]

After seeing @xaviershay's example:

replace('MyClass').with(some_fake_object)
replace('MyClass').with_instance_double(my_method: true)
replace('MyClass').transferring_nested_constants.with_class_double(my_method: true)

I think that this is much more in the spirit of the direction that RSpec's syntax is headed. Small, verb-ed, non-monkeypatched, chainable DSL methods that do one thing well and are also very readable.

One thought that I had was that, because the case in almost all instances would be to replace a constant with another class (as stated above) it could look like this:

replace('MyClass').with(some_fake_object)
replace('MyClass').with_double(instance: true).allowing(my_method: true)
replace('MyClass').with_double.allowing(my_method: true)

Benefits to this:

• with_double is extremely clear IMO and it hides the instance version for the majority of people who don't need/care. Contrast this to with_class_double, the first thing I think is: "Why does it say class, is there another one? Is this the one I'm supposed to use?"
• allowing maps very closely with the the new mocking syntax so there will be less cognitive dissonance.

But once you see the above, you start to think, "Well if the instance version is bad practice and we don't really want to support it, why can't we simplify even further?"

replace('MyClass').with some_fake_object
replace('MyClass').with double(my_method: true)

In regards to nested constants, always doing it is less of a WTF moment and IMO should be the default. In the few cases where it's not needed, the DSL wouldn't look as good, but it's good enough for an edge case such as that.

replace('MyClass', include_nested_constants: false).with some_fake_object

PS: I'm not sure implementation-wise if include_nested_constants would be an option to replace or to with but you get the idea.

I read all of the discussion but I hope I didn't misunderstand something. Thoughts?

[xaviershay]

So it's been a while since I've actually looked at rspec-fire source code, turns out you actually need a class double for transferring constants to even make sense. I'm inclined to just not support constant replacement with an instance double until someone makes a very good case for it.