Provide const_double alias for class_double.

[xaviershay]

References #391.

@myronmarston this seemed the simplest solution. Is there any behaviour you'd expect to be different? How do you feel about const_double instead of object_double? I felt like it was a more accurate name, sitting alongside class.

[coveralls]

Coverage decreased (-0.08%) when pulling 430d1a9 on issue-391 into 8e1a653 on master.

[myronmarston]

This is a good start, and it's nice to see that class_double "just worked" for a non-class constant reference.

Two things:

• Just like instance_double and class_double can accept the class object directly (rather than the string version of the constant that references the class), I'd like for this to be able to accept an object directly. For example, this alternate spec should work, I think:

        it 'can create a double that matches the interface of any arbitrary object' do
          o = const_double(LoadedClass.new)

          prevents { expect(o).to receive(:undefined_instance_method) }
          prevents { expect(o).to receive(:defined_class_method) }
          prevents { o.defined_instance_method }

          expect(o).to receive(:defined_instance_method)
          o.defined_instance_method
        end

• This doesn't yet work with your PR because of how ModuleReference only works with modules, and LoadedClass.new is not a module. So there's some changes needed there. I haven't followed it through to the conclusion to see what all those would be.
• Why the name const_double? I don't find that to be very descriptive at all. I think I prefer object_double, as it's a double that matches the interface of any arbitrary object, whether that object is provided directly or a string constant reference is provided.

[JonRowe]

const_double is also confusing me, looking at the code I think I understand but from the name alone I did not

[xaviershay]

Weird, const_double just made sense to me. Eh object_double is fine too, I'll change it and add in Myron's extra spec.

[JonRowe]

To me it's odd because the most common use of constants is for class names. So to then pass an object that's not a 'class' to it seems weird. Could just be me though ;)

[xaviershay]

I'm going to build this on top of #422 once that lands.

[xaviershay]

So the code is easy, figuring out how to name and guard everything is hard...

I'm currently trying to figure out (thinking out loud):

• What should ModuleReference be called if it also takes Objects? ObjectReference? Or do I construct it with a factory method that creates different objects for different types (that's probably a good strategy regardless, the current implementation is weird.)
• What should instance_double(LoadedClass.new) do? Raise an ArgumentError or provide an object_double under the hood?
• Does it still make sense to say that is this is provided as an alias to class_double? Or is it actually the other way around? class_double is a specialised alias of object_double. I think I want to rename ClassVerifyingDouble to ObjectVerifyingDouble and flip this around.

Obviously the current diff is WIP, I haven't changed any names or documentation yet.

[xaviershay]

OK I think I've addressed all my thinking out loud above, this is ready for review again.

@myronmarston in particular read over the added feature, because I pretty much crimped it all from stuff you've said in comments :) This isn't a feature I've felt a strong need for personally, so I may not have done it justice.

Weakest part at the moment is probably the feature for constant replacement. Suggestions welcome.

[myronmarston]

This is a really excellent description of object_double; nice work!

One other use case that might be worth mentioning is doubling an instance of a class that heavily uses method_missing. IIRC, I originally thought up object_double when you were documenting how to use the verified doubles feature for ActiveRecord::Base subclasses, since it uses method_missing so much, and the idea came out of that (as if you have the class loaded, using object_double seems preferrable to the other work arounds such as defining column attribute methods). Do you think it's worth mentioning something about this use case as well?

[xaviershay]

You're probably still reading, but I have addressed your first three comments.

[xaviershay]

I seem incapable of ever submitting a PR that is 1.8 compatible :/

[myronmarston]

Why not just do:

def when_loaded
  yield @object
end

I've read on multiple occasions that capturing the block as a proc (&block) carries a perf penalty compared to using yield (which makes some sense; procs have to support some things that yield doesn't, such as answering questions about arity, etc).

[xaviershay]

I just use &block out of habit, no good reason. yield works too.

[myronmarston]

What should ModuleReference be called if it also takes Objects? ObjectReference? Or do I construct it with a factory method that creates different objects for different types (that's probably a good strategy regardless, the current implementation is weird.)

I like the idea of a factory method.

However, I'm wondering if the difference we're looking for isn't so much module vs. object, but named reference (when given a string form of a constant) vs direct reference (when given an object directly).

Looking at your implementation, I think there's some weirdness that reorganizing on this axis may help resolve:

• ObjectReference#defined? always returns true, but in the MyApp::LOGGER case from the cuke, shouldn't one be able to double that when that const is not defined (i.e. when the full app is not loaded) just like with instance_double or class_double and the const is not defined?
• ObjectReference#name returns @object.to_s, but this doesn't seem right to me. I thought the point of #name was to return a string that, when resolved as a constant, would refer to the object. This is the case for class_double(MyClass) or class_double("MyClass")-- ModuleReference#name will return "MyClass", which resolves as a const to MyClass. This in turn is useful for chaining as_stubbed_const. Seems like as the implementation stands now, object_double(some_object_instance).as_stubbed_const will attempt to stub some_object_instance.to_s -- but that won't be a valid const name. In this case, it should raise a clear error, I think.

Putting these together, I think there are 3 cases, and 3 classes that would result:

def reference_for(object)
  case object
    when String then NamedObjectReference.new(object)
    when Module then DirectModuleReference.new(object)
    else DirectObjectReference.new(object)
  end
end

DirectModuleReference will basically be the current ModuleReference case when given a module. DirectObjectReference will be the current ObjectReference, except it won't support name like the current one does (probably raising an error instead). NamedObjectReference will be like the current ModuleReference when it takes a string.

I might be missing something but that seems to handle (and represent) all the cases well.

What should instance_double(LoadedClass.new) do? Raise an ArgumentError or provide an object_double under the hood?

ArgumentError, unless LoadedClass is a class factory like Struct. To me, instance_double is for things that have instance_methods. Coercing it would just create confusion.

Does it still make sense to say that is this is provided as an alias to class_double? Or is it actually the other way around? class_double is a specialised alias of object_double. I think I want to rename ClassVerifyingDouble to ObjectVerifyingDouble and flip this around.

I like that.

[xaviershay]

I like the idea of a factory method.

This turned out to not be necessary, instead there is just a conditional in object_double. I didn't feel adding a design pattern made the code clearer.

I thought the point of #name was to return a string that, when resolved as a constant, would refer to the object.

No, it's only used for exception messages. In this case, it does make sense on ObjectReference.

ObjectReference#defined? always returns true, but in the MyApp::LOGGER case from the cuke, shouldn't one be able to double that when that const is not defined (i.e. when the full app is not loaded) just like with instance_double or class_double and the const is not defined?

In that case, a ModuleReference is being used (since it is a string argument). We have no way of knowing whether it is an object or a module if it hasn't been loaded yet. I added a spec that demonstrates this.

object_double(some_object_instance).as_stubbed_const will attempt to stub some_object_instance.to_s -- but that won't be a valid const name. In this case, it should raise a clear error, I think.

I believe I have verified all the error messages in the spec, including this one, and checked that they are clear. Counter-examples welcome.

ArgumentError, unless LoadedClass is a class factory like Struct. To me, instance_double is for things that have instance_methods. Coercing it would just create confusion.

I came to the same conclusion, and that is current behaviour. I added a spec to verify the Struct case, which already works because Struct is a Module.

[xaviershay]

green build

[JonRowe]

(leaves to @myronmarston to review)

[myronmarston]

This is looking really good, @xaviershay. I have some responses to our conversation above, but these aren't merge blockers. I'd be happy to merge this as-is (once green) and submit a PR of my own to do the refactorings I have in mind (see below).

I thought the point of #name was to return a string that, when resolved as a constant, would refer to the object.

No, it's only used for exception messages. In this case, it does make sense on ObjectReference.

I looked into this some more and it looks like we're both right. name is used as a constant name here:

rspec-mocks/lib/rspec/mocks/verifying_double.rb

Line 63 in d5b2bac

ConstantMutator.stub(@doubled_module.name, self, options)

...which is the spot I was thinking of. It is also used in the error message here:

rspec-mocks/lib/rspec/mocks/error_generator.rb

Line 53 in d5b2bac

doubled_module.name,

...which is a use of name I was unaware of.

I think that while the same string works well for these two cases for a ModuleReference, the semantics of these two use cases are entirely different, and I think it would help make the code clearer and reduce confusion to split it into two methods: #description, which is used by the error generator, and #const_name, which is used when stubbing the const. For ModuleReference, these can simply be aliases. For ObjectReference, it can implement #description but leave #const_name unimplemented (as it's not supported, and not called from anywhere).

Also, should it be @object.inspect rather than @object.to_s for ObjectReference?

In that case, a ModuleReference is being used (since it is a string argument). We have no way of knowing whether it is an object or a module if it hasn't been loaded yet. I added a spec that demonstrates this.

Your specs cover all the cases well. I do think, however, that having a ModuleReference can refer to a non-module is confusing and suggests we have the names wrong. I think my idea to have NamedObjectReference, DirectModuleReference and DirectObjectReference solves this issue. It also removes the need for the case statement in ModuleReference (for Module vs String), allowing for one case statement total rather than one there plus the if you have in object_double.

Anyhow, one last suggestion: the dynamic_classes cuke should perhaps be revised in light of the new object_double feature since it could work well there.

[xaviershay]

#description, which is used by the error generator, and #const_name, which is used when stubbing the const.

I applied this refactoring, though used #const_to_replace since that then allowed me to move the ArgumentError inside ObjectReference and remove a conditional.

Also, should it be @object.inspect rather than @object.to_s for ObjectReference?

I was on the fence, after thinking about it some more I think inspect since if you try it with standard classes such as String that's what you would use.

I do think, however, that having a ModuleReference can refer to a non-module is confusing and suggests we have the names wrong.

Let's leave this for another PR. I started going down this route just now and didn't like how it was turning out, so I'd like to debate that separately.

dynamic_classes cuke should perhaps be revised

Added a sentence

[xaviershay]

...I should probably save the commit squashing until we're done so you can see the diff. Sorry about that.

[myronmarston]

You could actually test it in isolation with something like this:

# user.rb
class User < ActiveRecord::Base
  EXAMPLE_INSTANCE = new
end

# in the spec
user = object_double("User::EXAMPLE_INSTANCE")

...but it's kinda weird.

[xaviershay]

I'm going to leave that as an easter egg for people to discover themselves :D

[myronmarston]

Yep, I already merged this since I'm not sure I want to document that. Still, it's neat that it falls out of this is a workable solution :).

[myronmarston]

Great work, @xaviershay!