Improve situation of using behaviour objects as stubs

[fatso83]

• Sinon version : 4.4.10

As #739, #1756, #1738, #1748 (and probably more) show, the fluent API for behaviours make people think that the returned object from the behaviour adding methods is a stub - which it is not (supposed to be, AFAIK). This situation should be improved - if possible.

An obvious improvement is adding documentation that shows "do and don't"s.

Another thing is actively catching use of the returned behaviour object as a stub and throwing. Sometimes it works today, and sometimes it doesn't. This would make using a behaviour object always not work, and would then break existing tests (which might be ok, given a bump in major version, but would still probably irk some people).

As this is supposedly what the work on the immutable fakes is supposed to address, I don't think "fixing" it using the existing spies and stubs is the right way to go about it.

Thoughts?

[scofalik]

I stumbled upon this today when the project I am working on started to throw an error in one of the tests. I was quite shocked to discover that stub object does not have .called and the other properties.

After reading linked issues I understand what was the problem, changed the test's code and it is working.

I am not much into Sinon's internals, so I will speak from user's point of view. In my opinion, it is incorrect and misleading to develop an API this way. Reading the documentation, a user gets the feeling, that they are able to create a stub and define it's behavior in one line, through "chainable API". This is a common and widely understood concept. So, in Sinon's stub's case, you are misled to think that you are given "chainable API", but you are not because consecutive calls do not return the original object.

So, basically stub implements behavior API, but for some reason, behavior is returned instead of stub. I understand that there might be strong reasons to have it that way, but it is counter-intuitive.

First of all, it feels bad to be forced to write stuff like this:

const callB = sinon.stub();
callB.onFirstCall().returns( true ).onSecondCall().returns( false );

Second, docs give you no clue about it. It should be in a big red box, so no one will skip it when scrolling to the code samples.

Third, it was working before. I expect a lot of broken tests after updating to 4.4.10.

[fatso83]

@scofalik I do agree the situation is sub-optimal, but this isn't documented behaviour (which is what this issue is addressing), so changing it isn't changing the official API (I do see this is nitpicking), and 4.4.10 just reverts back to the old behaviour. Being consistent and removing inconsistencies like this is what the fakes feature in the next branch is addressing.

I am hoping that @sinonjs/core can provide some input here, as it's not a clear cut case what to do.

[scofalik]

Of course, the problem here is not that something works differently than described in docs. The problem here is that provided API looks like it lets you do something, but in fact, it doesn't. Stub API cannot be changed like this, so it won't change. Improving docs and promoting fakes is probably the only reasonable solution.

What saddens me is that this is kind-of a breaking change, but it is introduced in a patch version.

[mroderick]

In order for mainline sinon to move forwards while we discuss this issue, I have reverted the commit that gave us 4.4.10 (from #1738).

[zuzusik]

Reading the documentation, a user gets the feeling, that they are able to create a stub and define it's behavior in one line, through "chainable API". This is a common and widely understood concept.

yeah, totally agree - this is a common and widely used concept and I really can't understand why Sinon was always trying to fight it sooooo hard

[mroderick]

yeah, totally agree - this is a common and widely used concept and I really can't understand why Sinon was always trying to fight it sooooo hard

Let's try to keep this issue focused on the task at hand.

[zuzusik]

I would also like to remind the issue from which my PR with chaining came in: #1100:

Sinon version : 1.17.4

let a = sinon.stub().returns(12); // a is a stub function
let b = sinon.stub().onCall(0).returns(12); // b is not a stub function (but some object)

so, actually, my PR can also be considered as a fix and not a change in API

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.