Rename Expectation.should_call method to something more descriptive

[ollipa]

I think it would be good to rename Expectation.should_call to something more descriptive to distinguish it better from Expectation.should_receive. When I did code reviews or wrote tests myself I quite often accidentally used should_call (spying) when I should have called should_receive (mock) or vice versa.

I suggest that we rename should_call to should_call_spy. Other suggestions that we discussed before included renaming both methods to something like call_mock and call_spy. However, I think it's better to make minimal breaking changes to the API.

[adarshk7]

I agree that should_call and should_receive should be more descriptive, but I still think changing should_call to should_call_spy isn't the right choice. Maybe I'm biased because I'm used to the current API, but should_call and should_receive already make more sense to me than should_call_spy.

One of the main reasons why flexmock is a nice-to-use library is because the API lets you write mocks and spies in a very natural way, when you use chaining (something you could read as an English sentence.)

So something like, "let me hook flexmock to something and now something should receive some call and I choose to return a mock, or something calls a function and I choose an expectation (optionally)". It reads somewhat naturally, such that should_call vs should_receive, at least when you chain things, you're clearly specifying the responsibilities for yourself or flexmock.

That being said, I completely agree that this specific wording is not descriptive enough, especially when you look at it standalone without the context of reading a chain as a "natural sentence" and we should change it to something as such.

But also, I think should_call_spy either breaks the "natural language flow" of the chaining or then it implies something completely different, in terms of the responsibility ("something should call the a spy, that I'll pass in as arguments to should_call_spy"), whereas in reality, the user just specifies what is being called, and the spying is in fact in the domain of flexmock's responsibility)

A few alternatives I could think of, for improving the API would be:

1. Rename should_call to expect_call and should_receive to mock_call or mock_attribute.
2. Just rename should_receive to mock_call.

I prefer option 1, but think that option 2 could be a good alternative in case we want to avoid a bigger change to the API.

To reason option 1 a bit, as an example,

flexmock(something).expect_call('function').with_args('a', 2).and_return(True)

reads like, "Hook flexmock on something and expect a call to the method function with arguments 'a' and 2 and it will return True"

And,

flexmock(something).mock_call('function').and_return(True)

reads like, "Hook flexmock on something and mock method function and make it returnTrue"

[christophe-riolo]

I agree with your opinion, and I really like expect_call but I think that should_call_spy actually reads decently :

flexmock(something).should_call_spy('function').with_args('a', 2).and_return(True)

Maybe a better option would be to have a slight change in that manner:

flexmock(something).should_call_spied('function').with_args('a', 2).and_return(True)
flexmock(something).should_call_mocked('function').with_args('a', 2).and_return(True)

What do you think, @adarshk7 @ollipa ?

[ollipa]

@adarshk7, I think it's hard to make the language always to flow naturally when the methods are chained. For example, expect_call('function').with_args('a', 2) make sense but expect_call('function').and_return(3) doesn't sound so natural anymore (for example, expect_call('function').should_return(3) would be more natural in this case).

I would not go that far that I would also rename should_receive because I think already makes sense and I would try to avoid more breaking changes to the API. expect_call does sound a bit better than should_call but I think it doesn't really make the distinction between mocking and spying that clear.

@christophe-riolo, although should_call_spied and should_call_mocked make sense, I personally don't like that the method calls would get so long.

Another idea I got is what if we rename should_call to should_proxy? It implies clearly that the method call is spied (proxied) and I think it sounds quite natural with the other methods:

# call to `method` is proxied and args 1 & 2 are passed to the proxied method.
flexmock(SomeClass).should_proxy('method').with_args(1, 2)
# call to `method` is proxied and the proxied method is expected to return True.
flexmock(SomeClass).should_proxy('method').and_return(True)

should_proxy is also shorter than previously suggested should_call_spy.

[christophe-riolo]

I don't like that it's so long either, but it's almost the same as should_call_spy so I though it was improving still :)
should_proxy is nice and short but I don't think it carries the meaning of "calling", it's more what we do when we mock than when we call the mock, so "should proxy with args and return" don't make as much sense as "should call with args and return"

[adarshk7]

(for example, expect_call('function').should_return(3) would be more natural in this case).

I think there's a lot of work that needs to go into the API in this regard. Even though this warrants a lot more changes, I think the long term plan should be to make the difference between spies and mocks a lot clearer, so far as to separate the API and the underlying code.

What I mean is that flexmock(something).expect_call('abc') returns a Spy object which would be completely different from flexmock(something).mock_call('abc') which would return a Mock object.

With that flexmock(something).expect_call('abc').and_return would not even exist and you are forced to call flexmock(something).expect_call('abc').should_return.

I understand this would truly be a very breaking change, so with this (or perhaps an even better plan) in mind, I think we should consider a bigger picture of a long term change and move along that path in increments. Where I mean that if we think "expect call and should return" sounds good, let's still change it to expect_call with the plan in mind that in some subsequent release (perhaps major) we will also change and_return to should_return for spies.

The same goes for the mock side of things as well.

I also believe that this clear separation of concern will also help clarify the underlying code quite a bit.

Additionally, I think with this approach I had these alternatives in mind spy_on and mock.

[LecrisUT]

Why not mock_call and spy_call. I think expect/check language could clash with other plugins like pytest-chcek. The only thing is that the and_* syntax no longer fits well (neither with expect though), but this would flow with with_*/should_* or just drop the prefix and have returns/raises/yields