Additional thoughts on constant stubbing

[myronmarston]

First off, thanks for merging my other PR. It's nice to see my contribution make it in :).

The conversation there and the README hints at the fact that the constant stubbing will likely become the default and potentially only behavior for fire_class_double. While I like the idea in principle, in practice I think there's an issue we need to resolve first. It works great for "terminal" constants (that is, constants that don't have constants nested within them) but there's a problem when stubbing a non-terminal constant. Specifically, once you've stubbed the constant, you can no longer access the nested constants until the example is finished and the original constant chain has been restored. As an example, consider this code:

class User < ActiveRecord::Base
  MAX_PROJECTS = 10

  class EmailParser
  end
end

If you use fire_replaced_class_double("User"), then referencing User::MAX_PROJECTS will result in a TypeError ("is not a class/module"). Likewise, all access is cut off to User::EmailParser.

I've thought a bit about how we might deal with this, and here's what I have in mind:

• Update the constant stubbing code so that if given a module or class as the test double, it duplicates nested constants on the double.
• Update fire_class_double to return a double that actually is a module or class. Currently it's a subclass of FireDouble, which is a subclass of RSpec::Mocks::Mock, so this would involve refactoring the code to use composition rather than inheritance, or putting the shared logic into a mixin.

I think this would solve these problems quite nicely. The one big question in my mind is, what does subclassing RSpec::Mocks::Mock give us that we'd be giving up by using an empty module in its place? Granted, I can go look at the rspec-mocks source, but I figured I'd ask your thoughts on this first :).

Anyhow, I've been enjoying contributing to rspec-fire, so I'd be happy to implement this if we can agree on how it should work.

@garybernhardt and @freelancing-god -- given your participation on the other thread, it'd be great to get your feedback here, too.

[pat]

A few thoughts while it's fresh - and I'm focused on behaviour, not on implementation - but if I was explicitly creating a constant double for User and I also needed access to MAX_PROJECTS, then I would stub that constant as well. I wouldn't expect it to be automatically defined. Same goes for User::EmailParser - if I need them, I should stub them. This gives me a clear picture of the external dependencies the class I'm testing has - and if I'm stubbing a lot of them, I should probably rethink my design.

So, if I'm calling user_class = stub_constant('User'), I'd like to be able to call stub_constant on that: user_class.stub_constant('EmailParser').

And of course, these doubles only get put into play if the existing constants don't exist, right?

[myronmarston]

if I was explicitly creating a constant double for User and I also needed access to MAX_PROJECTS, then I would stub that constant as well.

As things stand now, rspec-fire doesn't support this. Once you've put a test double in place of User, no constants under User are supported, because ruby only allows constants to be placed under modules and classes, and an rspec-fire double is not a module or class (hence my second bullet point above).

So, if I'm calling user_class = stub_constant('User'), I'd like to be able to call stub_constant on that: user_class.stub_constant('EmailParser').

I like that API :).

Same goes for User::EmailParser - if I need them, I should stub them. This gives me a clear picture of the external dependencies the class I'm testing has - and if I'm stubbing a lot of them, I should probably rethink my design.

While I agree with this in principle, in practice, this was an issue I had to work around this week, and I don't think it was due to a design problem. The User example I pasted above doesn't really adequately show what I was doing, but it was simpler to explain :). Here's a better example that maps to what I was doing:

It's a common good practice in gems to use a top-level constant as a namespace for everything in the gem. Sometimes, this top-level constant has methods, as well; as the main entry point for usage of the gem, it makes sense to put the main APIs here. I've been working on a gem like this, and there was one of the methods on this top-level constant that I wanted to stub, and I wanted to use an fire_replaced_class_double for my stubbing so that it would verify the existence and arity of the method I was stubbing. The problem was, once I used a fire_replaced_class_double for my top-level constant, I no longer had access to any of the other constants or classes in my gem :(. I managed to work around it by putting things in a particular order (and grabbing some references to constants before doubling my top-level constant), but it felt like a hack. It would have been much easier if the existing constants were transferred to my doubled constant.

So, it seems to me that there's a valid case for doing it both ways. As such, maybe it should default to one way but provide an option to do the other way? Something like stub_constant("MyGem", :transfer_nested_constants => true), or even stub_constant("MyGem", :transfer_nested_constants => [:Foo, :Bar]) to specify which ones to transfer.

And of course, these doubles only get put into play if the existing constants don't exist, right?

Nope. When you use fire_replaced_class_double("User"), it will always set User to the double, regardless of whether or not it exists, and it will always clean up after itself at the end of the example. Beyond that, there are a couple differences between when User was already loaded or not:

• If User was already loaded:
  • Any mocked or stubbed methods will have their interface checked against the original class.
  • To cleanup, the User constant is set back to the original class.
• If User was not already loaded:
  • No interface checking is done for mocked or stubbed methods (since there's no existing class to use as a correctness reference).
  • To cleanup, the User constant is removed.

[xaviershay]

I don't have strong feelings either way, happy to trust your judgement. (Sorry that's a total cop-out, but I figure better than not saying anything...)

[myronmarston]

Closing since my stuff has been merged.