Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow receive with multiple methods #368

Closed
avit opened this issue Jul 17, 2013 · 21 comments
Closed

allow receive with multiple methods #368

avit opened this issue Jul 17, 2013 · 21 comments

Comments

@avit
Copy link

@avit avit commented Jul 17, 2013

Previously it was possible to quickly stub methods thus:

obj.stub!(first: 1, last: 2)

Now these "should" be done as separate declarations with messier syntax:

allow(obj).to receive(:first) { 1 }
allow(obj).to receive(:last) { 2 }

Is there a way around this? Would it be feasible to have at least:

allow(obj).to receive(first: 1, last: 2)

or something like:

target(object).stub(first: 1, last: 2)
@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jul 17, 2013

What about this:

allow(obj).to receive_messages(first: 1, last: 2)

Then it's very explicit that it is the multi-case.

@avit
Copy link
Author

@avit avit commented Jul 17, 2013

Yes, I like that. Could the wording be more fluid for either single- or multi-use, perhaps:

allow(obj).to receive_and_return(first: 1)

Then it looks like a shorthand for receive(:first).and_return(1) but handles either single or multi. What's the preference? Flexible syntax, or explicitly different? Let me know and I can write a PR.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Jul 18, 2013

I think I prefer

allow(obj).to receive_messages(first: 1, last: 2)

over

allow(obj).to receive_and_return(first: 1)

the wording seems nicer.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jul 18, 2013

I think I like receive_messages better, too.

@cupakromer
Copy link
Member

@cupakromer cupakromer commented Jul 29, 2013

👍 for receive_messages

@avit
Copy link
Author

@avit avit commented Jul 29, 2013

Yes, I'm on board with receive_messages, I'll try and code this up soon.

@rosenfeld
Copy link

@rosenfeld rosenfeld commented Aug 1, 2013

As I stated in #389 I believe we should keep the original matcher receive as in:

allow(controller).to receive(:authenticate_user!, current_user: user, ...)

Isn't it possible?

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Aug 1, 2013

It's possible, but receive_messages seems more explicit and readable to me. Why do you prefer complicating receive by overloading it?

@avit
Copy link
Author

@avit avit commented Aug 1, 2013

I can see the appeal too: one less method to remember in the DSL, is it worth having a different name for 1 vs. many stubs? Which of the following should be receive_messages?

allow(obj).to receive(:first)
allow(obj).to receive(:first => 1)
allow(obj).to receive(:first, :last)
allow(obj).to receive(:first => 1, :last => 2)
@rosenfeld
Copy link

@rosenfeld rosenfeld commented Aug 1, 2013

receive_messages is not different from receive. After all what does receive receive if not messages?

It's just longer and another method to remember, like @avit said.

@rosenfeld
Copy link

@rosenfeld rosenfeld commented Aug 1, 2013

If someone tries to use it as:

allow(obj).to receive(a: 1).and_returns{...}

Just raise an exception and say that this usage is not supported yet until we discuss how to better chain it in such usage cases.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Aug 2, 2013

I'm ok with having the extra DSL method if it removes the overloading and reduces the internal complexity, especially if it removes the chaining conundrum.

@rosenfeld
Copy link

@rosenfeld rosenfeld commented Aug 2, 2013

Just to be clear, I don't really mind if it will be called receive or anything else. I just happen to prefer receive but I'll be fine with any name you choose. I'm just really interested on this being available as soon as possible, like in the next minor release for instance. So, if my arguments for using receive is slowing down the decision upon implementing this feature, please just ignore my comments.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Aug 2, 2013

There will only be patch releases, no more minors, before version 3.0. 2.99 serves only to add deprecation warnings for 3.0.

@cupakromer
Copy link
Member

@cupakromer cupakromer commented Aug 2, 2013

@rosenfeld So my issue with the overloading of receive is it's twin when used with expect:

expect(obj).to receive(:first)            # Parity
expect(obj).to receive(first: 1)          # Not overly expressive, but understandable
expect(obj).to receive(:first, :last)     # Is this ordered? How do I chain `.with`?
expect(obj).to receive(first: 1, last: 2) # Same concern as above

By having a close parity between the two uses, it makes it easier to remember when you can and should use each as the API is the same. With that being said, I do not think that receive_messages should be added to expect. It violates the single expectation guideline we follow and it's implementation is a bit questionable.

@rosenfeld
Copy link

@rosenfeld rosenfeld commented Aug 2, 2013

Yes, that makes sense, @cupakromer. I agree with everything you said. I'd just prefer a shorter name then receive_message if possible, but that's not a big deal. Also, if we're going to keep the long name, maybe change it to something else with more meaning since receive and receive_message mean the same to me...

What about something like this?

allow(obj, first: 1, last: 2)
@rosenfeld
Copy link

@rosenfeld rosenfeld commented Aug 2, 2013

For the example above we could introduce stub instead of using allow if you prefer to...

For expectations something like this might work:

expect(obj).to respond_with(first: 1, last: 2)

For ordered and chaining I don't think it worths adding a shortcut DSL... Can you think of any examples where it would be useful?

@cupakromer
Copy link
Member

@cupakromer cupakromer commented Aug 3, 2013

For the example above we could introduce stub instead of using allow if you prefer to...

RSpec is actively moving away from stub (see here and the associated Deprecate Stub for Mock).

What about something like this?

allow(obj, first: 1, last: 2)

I'm hesitant to see allow overloaded like that. For a double that syntax still should still work on creation:

dbl = double( 'Book', title: 'Testing with RSpec', price: 0.99 )

Due to that, I see this discussion related more to partial mocking on non-double objects, though I do occasionally add a message stub on a double in a one-off test.

Note there is current planning to make a double more intelligent. So that may help too.

For expectations something like this might work:

expect(obj).to respond_with(first: 1, last: 2)

For ordered and chaining I don't think it worths adding a shortcut DSL... Can you think of any examples where it would be useful?

Again, just looking at the code, I'm not sure what this is supposed to be expressing. Is it an ordered expectation? Or are you just mashing two expectations into one test? Though based on your comment I can infer the latter.

Reading through tests, I would prefer the current, more explicit, options to defining ordered / complex message expectations.

Just my 2¢ take it with a grain of salt.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Aug 5, 2013

I can see the appeal too: one less method to remember in the DSL, is it worth having a different name for 1 vs. many stubs? Which of the following should be receive_messages?

allow(obj).to receive(:first)
allow(obj).to receive(:first => 1)
allow(obj).to receive(:first, :last)
allow(obj).to receive(:first => 1, :last => 2)

IMO, only the first should be receive. The two hash forms should be receive_messages, and the list of messages names (:first, :last) wouldn't be directly supported (though you could achieve the same result with allow(obj).to receive_messages(first: nil, last: nil)).

Currently receive only accepts a single message name (and does not accept a hash) and I'd like to keep it that way. I find the simplicity and consistency of having a method accept only one type of argument preferable to having a method accept multiple different types of arguments -- so having receive for a symbol and receive_messages for a hash appeals to me.

@rosenfeld
Copy link

@rosenfeld rosenfeld commented Aug 5, 2013

That's fine to me, @myronmarston. Go ahead.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Sep 12, 2013

Fixed by #399.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.