Provide a default value (Mock.new) for stub_const. #163

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@stevenharman

Currently you are required to provide a value for the stubbed constant,
but there are times that I don't actually care what it is because I'm
just going to stub the usage of the constant anyhow. For example:

stub_const('DeathStar', stub)
Starship.stub(:status) { :operational }

This allows you to

stub_const('DeathStar')
Starship.stub(:status) { :operational }

  # or
stub_const('DeathStar').stub(:status) { :operational }
@stevenharman stevenharman Provide a default value (Mock.new) for stub_const.
Currently you are required to provide a value for the stubbed constant,
but there are times that I don't actually care what it is because I'm
just going to stub the usage of the constant anyhow. For example:

```ruby
stub_const('DeathStar', stub)
Starship.stub(:status) { :operational }
```

This allows you to

```ruby
stub_const('DeathStar')
Starship.stub(:status) { :operational }

  # or
stub_const('DeathStar').stub(:status) { :operational }
```
9b827a7
@travisbot

This pull request passes (merged 9b827a7 into f36ad4d).

@myronmarston
Member

Thanks for the suggestion. Before the 2.11 release, we discussed whether or not to provide a default. At the time, we were considering making Class.new the default (see this comment from @alindeman and following). We wound up deciding to hold off on a default because it's easy to add a default in the future when there's a community consensus, but it's hard to remove or change a default once it's there.

So...I find it interesting that the default you suggest (Mock.new) is different from the default originally suggested (Class.new). One issue with Mock.new is that it is unable to support nested constants; only modules and classes can have further constants off of them.

If we provide a default, I think it should be a "test double class"--similar to the type underlying a fire_class_double:

  • Either Class.new or Module.new...
  • ...extended with RSpec::Mocks::TestDouble to get the behavior of a mock object...
  • ...with helpful #name,/#to_s/#inspect definitions.

I think this would provide the "best of both worlds" and was actually the motivation for extracting the code for a mock object behind a module (RSpec::Mocks::TestDouble) so that it can be extended onto a Class.new instance or a Module.new instance.

One big question: should it be Module.new or Class.new? Both support nested comments. If it's Module.new then people can use it as a module and extend or include it onto another class or object. If it's Class.new they can't do that, but they can construct a new instance of it.

Thoughts?

@myronmarston
Member

@stevenharman / @alindeman -- any thoughts on my comments above?

@alindeman
Contributor

Your overarching suggestion seems like a good one. As to whether the basis is Module.new or Class.new, I'm honestly not sure .. and not sure what would convince me one way or the other.

Admittedly, I've not actually used stub_const much myself (yet?).

Maybe there could be stub_class and stub_module? Or is that just API bloat?

@myronmarston
Member

Maybe there could be stub_class and stub_module? Or is that just API bloat?

I think that'd be API bloat. We already support both with a more powerful, general abstraction:

stub_const("Const", Class.new)
stub_const("Const", Module.new)

...so why provide an alternate, less powerful, more specific abstraction? It doesn't even save much typing.

Given the lack of a clear winner, should we table this for now until a clear winner emerges?

People can always define a stub_class or stub_module helper method in their own projects if they have a preference.

@dchelimsky
Member

Given the lack of a clear winner, should we table this for now until a clear winner emerges?

Yep.

@alindeman alindeman closed this Sep 27, 2012
@stevenharman

Sorry about not getting back sooner - I was out of the country and intentionally disconnected.

I agree that there is little need to add stub_class and stub_module to the API.

My original intent was only to make the extremely simple case of "loading a constant so I could stub it for interactions" a little easier. That said, and given @myronmarston's point that my suggested default value of stub would impose some limitations with regard to nested constants, I'm fine to just ignore the whole thing and chalk it up to my personal aesthetic. Thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment