Overridable yields vs sequential behavior

[tf]

Hi,

in version 1.5.0 the semantics of calling yields multiple times changed from overriding former instructions to recording a sequence of values to be yielded sequentially (see #157).

While the new behavior might be desirable in some cases, it makes one of our frequently applied test patterns almost impossible to use. We like to encapsulate the construction of commonly used stub objects in test helpers:

function stubbedCollaborator() {
  return {
    doSomething: sinon.stub().yields([]),
    answer: sinon.stub().returns(42)
  };
}

Overriding the default behavior of individual stub methods allows each test to focus on the setup relevant to its intent. Since the change, this is no longer possible forcing the test to duplicate the complete stubbing setup if it needs to diverge from the default.

I've looked into different ways to circumvent this issue, but have not yet come up with a side effect free solution.

While I have to say the old API made more sense to me, I guess others are already depending on these new semantics. So I'd suggest adding a new way of calling yields which will be overridden by sequential calls. Something along the lines of yieldsDefault, while this might not be an intuitive name. Or an option on stub?

I'd be really interested to hear your thoughts on this.

Cheers
Tim

[cjohansen]

Here's my thoughts: The change of semantics in 1.5 was absolutely not intended, but I'd failed to have automated tests for this behavior and didn't pay close enough attention. Even though 1.5 has been out for a while I've been thinking of revoking it and replacing it with 1.6 which restores the original behavior. However, time flies... @domenic mentioned he was going to look into a possible "best of both worlds" solution - any news on that?

[domenic]

Well, I already fixed the second part of #231 and I think that was all I meant to do---best of both worlds is a bit more than I promised :).

If we can agree on what the desired behavior is, I'm happy to come up with something that does both. E.g. maybe yields behaves as in 1.4 and below, but thenYields builds up sequences, or yields stays at its 1.5 behavior and resetYields clears the chain and starts a new one.

[cjohansen]

Well, I already fixed the second part of #231 and I think that was all I meant to do---best of both worlds is a bit
more than I promised :).

What, you're no magician? :)

E.g. maybe yields behaves as in 1.4 and below, but thenYields builds up sequences

This! If you do it, I'll be very grateful, otherwise I'll eventually get to it myself.

[tf]

Yes, I'd also be very much in favor of thenYields.

[domenic]

This! If you do it, I'll be very grateful, otherwise I'll eventually get to it myself.

OK! Sounds like a weekend plan. Hopefully this weekend, but you know how these things go.

[mantoni]

👍 for thenYields. Although these words pop into my head as well: thenReturns, thenThrows, thenCallsArg. No pressure though ;-)

[tf]

Thinking about this a little more, especially things like thenReturns lead to further subtleties. While returns and, for example, yields can be used together, their then... equivalents would not allow for a combined "yield x and return y" behavior to appear in a sequence.

var foo = sinon.stub();
foo.yields(4).returns(true);
foo.thenYields(5).thenReturns(false); // foo yields, next call returns false

While this surely is a rare case, it might point out a weakness in the proposed API. As an alternative, one could consider introducing a separate then method. This would also prevent a combinatorial explosion of methods (thenYieldsAsync etc.) and ease future growth of the stub API.

foo.yields(4).returns(true);
foo.then().yields(5).returns(false);
// While the above example amounts to
foo.yields(4).returns(true);
foo.then().yields(5);
foo.then().returns(false);

Making it work in conjunction with the withArgs method might result in an even more powerful language:

// Defining two independent sequences
foo.withArgs(1).returns(2);
foo.withArgs(1).then().returns(3);
foo.withArgs(2).returns(4);
foo.withArgs(2).then().returns(5);
foo(1); // => 2
foo(2); // => 4
foo(1); // => 3
foo(2); // => 5

// Argument based case distinction in sequence
foo.returns(0);
foo.then().withArgs(1).returns(3)
          .withArgs(2).returns(4);
foo('anything'); // => 2
foo(1); // => 3

Given the apparent complexity, I am still unsure whether this is of more than academic interest. I have yet to find a need for such sequencing functionality and originally opened this issue because even the much simpler existing sequencing functionality tripped me in my day to day work.

So maybe others that have actually used the sequence aspects of yields and friends should voice their opinion. If somebody is interested though, I have some more thoughts on how the above features could be integrated in the existing code base.

Best
Tim

[mantoni]

Thank you for the detailed write up. I agree that this might be something that is not used very frequently. However, I also see a value in this feature.

Now that I read your use case examples, a different way of approaching the problem came to mind.
We always thought about repetitive calls to yields and friends. However, we could also extend the withArgs concept to calls:

foo.returns(2);
foo.onCall(1).returns(3);

This would solve your yields and returns problem:

foo.onCall(2).yields('foo').returns(true);

and it could even be used in this way:

foo.withArgs(1).onCall(3).returns(5);

I also imagine this to be a simpler implementation.

What do you think?

[domenic]

@mantoni I like that quite a lot, and avoiding the combinatorial explosion is a big plus. @cjohansen thoughts?

[tf]

So just to make sure I fully understand: The stub in your first example would return 3 on the first call, and 2 afterwards, right? The question whether the passed index is zero or one based might be a future source of confusion.

Moreover, what is the expected behavior if I skip a number (e.g. call onCall(1) and onCall(3))? Does it fall back to the default on the second call?

I'm not really sure whether it is desirable to explicitly write out the sequence of indexes. The only advantage I see is the possibility to construct a stub behavior of a call in multiple statements, by repeatedly calling onCall(n) with the same value for n, where in the then case one would have to resort to a chaining API. Is that worth the additional bookkeeping of enumerating the sequence manually?

In terms of implementation complexity then would rather easily be derived from onCall by internally increasing a counter.

I might be missing something though.

[mantoni]

So just to make sure I fully understand: The stub in your first example would return 3 on the first call, and 2 afterwards, right? The question whether the passed index is zero or one based might be a future source of confusion.

It would be zero based, just like getCall(index). Therefore in my first example above, the stub would always return 2, except for the second call where the return value is 3.

Moreover, what is the expected behavior if I skip a number (e.g. call onCall(1) and onCall(3))? Does it fall back to the default on the second call?

It would fall back to whatever was programmed by default. This would be in line with withArgs where the described behavior only applies if the given arguments match.

I'm not really sure whether it is desirable to explicitly write out the sequence of indexes. The only advantage I see is the possibility to construct a stub behavior of a call in multiple statements, by repeatedly calling onCall(n) with the same value for n, where in the then case one would have to resort to a chaining API. Is that worth the additional bookkeeping of enumerating the sequence manually?

Hm, I was more seeing it this way: I have a default behavior for a method and I want something different to happen on, let's say, the 4th call. That would be tedious with then....

[tf]

Ah ok, I wasn't even aware of getCall. If specifying the 4th call is a use case, then this is the desired API. Though it still seems a bit confusing to me that foo.onCall(1).returns(1) means "foo on call two returns one". Especially, since it reads even more like a sentence than getCall(1).

[mantoni]

Especially, since it reads even more like a sentence

True. For calls there is firstcall, secondCall and thirdCall. We could do the same for onCall and have onFirstCall, onSecondCall and onThirdCall for better readability.

[tf]

This would definitely be a great fit with the rest of the API. I like this API better than my initial proposal, even more so since on second thought then is quickly becoming sort of a reserved word in the context of promises.

[cjohansen]

I like @mantoni's suggestion as well. As @tf commented, it sits really well with the rest of the API. Good collaborative thinking everyone :)

[tf]

Ok, maybe then its time to start talking about how to best implement the above ideas. Some thoughts:

• Looking at the current implementation of Stub, I think we might be missing the abstraction of Behaviour. The group of arrays named callbackArgAts, callbackArgProps etc together make up a collection of objects describing the callback behavior of a stub at subsequent invocations. The getChangingValue function is a rather dirty workaround to perform property look ups in those "objects". Making the concept of behavior explicit would drastically clean up the code. getChangingValue would become something like getCurrentBehavior.
• The functionality could then be changed so that, instead of building up a list of behaviors, methods like yields would delegate to a default behavior which in turn would override its settings on each call. A onCall(n) method could populate an array of special behaviors for the nth call.
• One could then move the return value information inside the behavior objects, aligning the chaining semantics of returns with those of the callback methods.

Thoughs?

[mantoni]

I'm sorry, but I have problems following your thoughts.

My first attempt would have been to do the same thing that we did for withArgs which creates and returns a new fake and then uses it in invoke. I would rename matchingFakes to matchingFakesForArgs and introduce matchingFakesForCall.

I might as well overlook something here. Not sure.

[tf]

So onCall would live on spy together with withArgs? What would be the
use case for spy.onCall(3). I can only imagine cases involving stubs.

[mantoni]

Oh yes. You are right about that. Does not make sense on spies. For assertions, one would use getCall(n).

[MrBigDog2U]

I have run into a similar situation so, all of a sudden, this topic has taken on new significance for me. I have a stub that needs to return different values on different calls (to simulate an environment change during the test - specifically, a transition from online to offline).

Tim, I like your concept of Behavior. I've never really cared for using parallel arrays for storing a list of groups of values. This would allow each call to specify either a callsArg, yields or returns value (although I'm not sure if there would be a use case where combining callArg on one call with returns on another since there is no return value from callArg).

Is anyone moving forward on this? I can make a local change to allow me to specify multiple returns (I don't really have the time to do the full implementation described above). If this is something that might be available soon though, I might just disable the offending test for now.

[domenic]

I like the behavior idea, and would be all for it if someone like @tf wanted to take over this feature from me :).

[cjohansen]

+1 on capturing Behavior as a separate entity. As Sinon has grown, the multitude of parallel arrays are trying to tell us something, and it seems like @tf's suggestion may be a good solution. @tf: Want to take a stab at your idea? I'll be happy to provide feedback as you go.

[mantoni]

Now that I looked at the code in the right place, I'm also convinced. Sorry @tf for the confusion.

[tf]

I'll take a look. Though, if I do not get around to it during the weekend, it may end up being some time late next week since we will be pretty swamped at work.

[cjohansen]

@tf No one is in a rush, take your time ;)

[tf]

I've started refactoring out sinon.behavior on a branch. The tests are unchanged. npm test passes. I've tried to emulate the coding style found elsewhere as closely as possible.

• For now stubs only delegate callback related functionality to behaviors.
• Async methods are still generated on stub since the tests suite has some explicit assumptions about how the delegation works there and I did not want to change the test suite in this refactoring step.
• The definition of sinon.behavior still sits at the beginning of stub.js a little awkwardly. It can't be moved down at the moment, since the delegation methods on stub are generated by enumerating sinon.behavior.
• I'm a little undecided where the tests for sinon.behavior should live. There is no real separation between unit tests and API-level acceptance tests in the test suite. As it stands behavior is a mere implementation detail of stub. So from an acceptance perspective the sinon API has not changed, so neither should the tests. From a unit perspective, methods like yields now live on behavior and should be tested there. For stub one would rather test the delegation logic. This might also make it easer to add functionality to behavior in the future.

I'd be glad if somebody could take a look at the code. If things are moving in the right direction, I'd be happy to proceed.

[cjohansen]

@tf looking good! Very nice to have this separated out. Many of the functions on behavior do mostly the same thing (setting four properties). Maybe these could be set by a single "private" function, so we can reduce the duplicated boilerplate. Other than that, I like this direction a lot.

Async methods are still generated on stub since the tests suite has some explicit assumptions about how the delegation works there and I did not want to change the test suite in this refactoring step.

I appreciate your disciplined approach. Still, it does sound like those tests need some "loosening up".

I'm a little undecided where the tests for sinon.behavior should live. There is no real separation between unit tests and API-level acceptance tests in the test suite.

Given that the API surface has grown quite a bit since I started this project, it might make sense to separate the tests this way. It's a good idea, anyway.

I'd be glad if somebody could take a look at the code. If things are moving in the right direction, I'd be happy to proceed.

I think it looks good. Maybe @mantoni wants to contribute a second opinion? :)

[mantoni]

I also like the direction this is going. Here are some thought I had while reading through the implementation:

One thing I found a little difficult to read is the way sinon.behavior is created and the actual instances are created. I think this could be simplified by making sinon.behavior a function that takes the stubFunctionName and then returns the behavior instance. That would make create obsolete - it deletes itself after creation anyway.

The question about where the test cases should go is a good one. My personal opinion on this topic is, that only the API should be tested. I would not consider sinon.behavior a part of the API though. The benefit from only testing the API is that we are free to make drastic refactorings in the implementation - just like what @tf did.

However, I understand that this can be seen as "integration testing" if you make a decision between unit and API testing. I also agree that the test cases are grown a lot and could be split up - but that's a different story. I'll support whatever decision is made on this - just wanted to throw in my thoughts and hear what you think about it.

Other than that: great work @tf - this is going to be a big improvement.

[cjohansen]

The question about where the test cases should go is a good one. My personal opinion on this topic is, that only the API should be tested. I would not consider sinon.behavior a part of the API though. The benefit from only testing the API is that we are free to make drastic refactorings in the implementation - just like what @tf did.

I agree on this. However, when underlying details are shared you have to decide between unit testing components that aren't really part of the API or duplicating tests at a higher level (where they are visible). FWIW, I find it generally hard to strike the right balance in this regard.

[mantoni]

when underlying details are shared you have to decide between unit testing components that aren't really part of the API or duplicating tests at a higher level

True. I already found myself copy-paste-adjusting test cases - that does show something :)

[cjohansen]

That makes sense. Is that what happens, too? :)

[tf]

Yes.

[MrBigDog2U]

Sounds like the development of this is progressing or may even be complete. Has any thought been given toward when this will be merged into the code base?

[cjohansen]

Is it complete? Sorry for falling out here.

[tf]

Just yesterday I discovered an issue which probably also exists in the current implementation, but which I'd like to iron out before submitting a pull request. Basically fakes do not inherit behavior.

var stub = stub();
stub.returns(1);
stub.withArgs(1).onFirstCall().returns(2);

stub(1) // => 2
stub(1) // => undefined, while 1 would be expected

I guess stubs need to propagate behavior to their fakes or fakes need to know their parent stubs.

When this is fixed the missing withArgs on behavior is easy to add since

stub.onFirstCall()
  .withArgs(1).returns(0)
  .withArgs(2).returns(1);

is the same as

stub.withArgs(1).onFirstCall().returns(0);
stub.withArgs(2).onFirstCall().returns(1);

So if we let the behavior know its index in the list of behaviors, behavior#withArgs can be implemented by delegation.

I try to look into these things as soon as possible, which might not be before the end of the week.

[tf]

Ok, I realize this has been lying around for far too long and this discussion has already grown to an enormous length. I have it working now on my machine, but there are still two things I want to discuss with you guys before I prepare a pull request.

Issue 1

The way I tweaked it, fakes inherit behavior from their parent stubs. Moreover stub.onFirstCall().withArgs(...) delegates to stub.withArgs(...).onFirstCall(). That way one can do:

stub.returns(1);
stub.onFirstCall().withArgs(5).returns(2);

stub(5) // => 2
stub(5) // => 1

As a side effect of this behavior inheritance, resetting the behavior of a fake now makes matching calls fall back to the parent stub again:

stub.returns(1);
stub.withArgs(5).returns(2);
stub.withArgs(5).resetBehavior();

stub(5) // => 1, used to be undefined

To me this seems way more intuitive now, but I'm not sure if it breaks tests for someone.

Issue 2

The seconds issue I've already mentioned above: onCall returns a behavior not a stub. So calling

var stub = sinon.stub().returns(0).onFirstCall().returns(1);
stub(1);

causes an exception. On the other hand, withArgs always has been flawed in a similar way: it returns the fake not the original spy:

var stub = sinon.stub().returns(0).withArgs(6).returns(1);
stub(0); // => 1 not 0

Once we have those sorted out, I'll be glad to prepare a pull request.

Best
Tim

[tf]

Since this issue seems to be getting some attention lately, I'd like to bump it again.

If we can agree that the two issues above are acceptable, I'd try to rebase my branch against master.

[mantoni]

Thanks for taking it up again @tf.

I think "Issue 1" is more a feature than an issue. It makes the behavior act more intuitive.

"Issue 2" is something we should address in withArgs as well. The result of withArgs and onCall should not be callable at all, or it should throw a meaningful error. I'm unsure whether this can be changed for withArgs before 2.0, but we should not make the same mistake for behaviors. The return values of withArgs and onCall should only be usable to program behavior or express expectations. It makes no sense to call them.

@cjohansen What's your opinion?

[cjohansen]

1. Completely fine. resetBehavior should probably not be used most of the time anyway.

2. I agree with @mantoni that this seems a little broken. However, as it's already broken I'm not sure we should make it a blocker at this point. I suggest getting this in place, and worrying about this issue later.

[tf]

Alright. The rebase applied cleanly. I've added a bunch more tests including withArgs support on behaviors. npm test and the HTML based buster test suite both pass.

There is obviously much room for improvement and refactoring, but I just wanted to push it out there now.

Please take a look, play around with it to see if there are cases I might not have thought about.

[tf]

Bump.

Shall I make a pull request out of this, so the changes can be reviewed more easily? I'd hate for this to go stale.

[mantoni]

PR away! I'm AFK until next week. Will have a look then.

Bump.
Shall I make a pull request out of this, so the changes can be reviewed more easily? I'd hate for this to go stale.

—
Reply to this email directly or view it on GitHub.

[tf]

Great. Submitted the PR.

[mantoni]

Closing this issue since the PR #338 was merged.