Fix onCall(#) with chained behaviors

[nfriedly]

Purpose (TL;DR) - mandatory

Restores previous Sinon behavior and fixes issue #1736 by making all chained behaviors (yields, returns, throws, etc.) added after an .onCall(#) apply to that call rather than only the first behavior applying, and subsequent ones reverting to the default behavior.

Background (Problem in detail) - optional

As mentioned in the original issue, in previous versions of Sinon, the following code added a yeilds and a returns behavior to call 0:

stub.onCall(0).yields(x).returns(y);

However, in the current release of Sinon, that code would cause the first call to yield x and return undefined, and then all subsequent calls would return y.

The codebase I'm working on currently, uses the above pattern in numerous places for mocking http.request (which both returns and yields), and upgrading to the current version of Sinon caused a number of tests to start failing.

Solution - optional

behaviorMethods always return this (the behavior object) instead of the original stub. I also changed the behavior object to be a function that calls through to the original stub.

WARNING

It's possible that some users inadvertently depend on the buggy behavior with code such as this:

var stub = sinon.stub().onCall(0).returns('foo');
stub.returns('bar');

In the current release, the first call will return 'foo', and subsequent calls will return 'bar'. With the fix, it will return 'bar' and then undefined. However, I would argue that the above code is buggy and this fix merely exposes that bug, and therfore this fix shouldn't constitute a breaking change.

How to verify - mandatory

1. Check out this branch
2. npm install
3. npm test

Checklist for author

• npm run lint passes
• References to standard library functions are cached.

[fatso83]

I think this looks fine. Good to have documenting tests.

I somewhat agree with you, but how would one make a stub that returns bar in most cases, but 'foo' in the 42nd call when using your fix?

[fatso83]

what does this do? I am guessing this is an easy way of wrapping the stub in a closure so you can change the copy instead of the original. am I right?

[nfriedly]

Yes. We could replace it with

var behavior = extend(function(){
   var args = Array.prototype.slice.call(arguments);
   return stub.apply(null, args);
}, proto);

But the .bind version is more concise.

We don't want to extend the stub directly because that would overwrite other things.

[nfriedly]

how would one make a stub that returns bar in most cases, but 'foo' in the 42nd call when using your fix?

Like this:

var stub = sinon.stub();
stub.returns('foo');
stub.onCall(42).returns('bar');

(The second two lines could be swapped without changing the behavior.)

Or, a chained version:

var stub = sinon.stub().returns('foo').onCall(42).returns('bar');

(The behavior depends on the order here.)

We could perhaps add a method to get back to the default behavior, e.g.

var stub = sinon.stub().onCall(42).returns('bar').otherwise().returns('foo');

and otherwise() would just be a function that returned this.stub. Probably needs a better name, though.

And, like I said, this change restores the behavior that was present in Sinon 1.x. I'm not exactly sure when it changed, but I'm sure we could track it down if it matters.

[fatso83]

@mroderick This seemed fine and I finally understood the nuances of the implications, but I am not sure this should result in a new major or a patch! It is an API change, but as the previous change never was official/published I regard this as more of a bugfix that gives us the old behaviour back.

[fatso83]

I chose to release this as 4.4.10, given that I think this should be regarded as a bug fix.

[fatso83]

And thanks, @nfriedly, for the great work!

[mroderick]

Good work team!