Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Overridable `yields` vs sequential behavior #244

Closed
tf opened this Issue · 50 comments

5 participants

@tf
tf commented

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
Owner

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
Owner

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
tf commented

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
Collaborator

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

@tf
tf commented

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
Collaborator

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
tf commented

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
Collaborator

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
tf commented

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
Collaborator

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
tf commented

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
Owner

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
tf commented

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
Collaborator

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
tf commented
@mantoni
Collaborator

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
Owner

+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
Collaborator

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

@tf
tf commented

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
Owner

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

@tf
tf commented

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
Owner

@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
Collaborator

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
Owner

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
Collaborator

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 :)

@mantoni
Collaborator

Ok. I've done sort of a "case study" on spies. I've refactored the spy call API out of the spy implementation and separated the test cases on this branch: https://github.com/cjohansen/Sinon.JS/tree/spy-calls

It's not ideal yet, but it cleans the implementation up quite a bit.

There is a new source file called "call.js" and a corresponding test case "call_test.js". This could work for Tim's behaviors as well ("behavior.js" and "behavior_test.js").

I personally don't feel the need to create separate folders for these (like "api" and "unit").

@cjohansen
Owner

Ooh, I like it a lot! Cutting down the size of those files is very welcome, nice work, @mantoni :)

@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf move async method generation to stub behavior (#244) 7fb204d
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf add onCall to stubs for building sequences (#244) 2d0b6f4
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf allow onCall to be combined with returns* and throwsException in stub…
… behavior sequences (#244)
a2590f7
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf move callback sequence tests in onCall section (#244) b975c9f
@tf
tf commented

I have made the discussed changes. There are now onCall, onFirstCall, onSecondCall and onThirdCall methods on stub which can be combined with yields, returns, withArgs and so on to build sequences. I've adapted the test suite to reflect the changes.

I have also fixed an error where making a stub async by calling an *Async method could not be undone.

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.

i agree. Still, I've adopted this pattern from the rest of the codebase. So maybe this should be addressed as a separate refactoring.

Many of the functions on behavior do mostly the same thing (setting four properties).

True. Getting the slice calls of the arguments array right is a little bit delicate. So I've skipped this for now.

There is a new source file called "call.js" and a corresponding test case "call_test.js". This could work for Tim's behaviors as well ("behavior.js" and "behavior_test.js").

I'd be all for putting sinon.behavior in its own file. I haven't done it yet since I was unsure which <script> tags etc in the different test suites have to be altered. Since I am only running node tests at the moment, I was a little reluctant.

So, please take a look at my changes.

Cheers
Tim

@tf
tf commented

I forgot to mention two limitations of the current approach:

  • Since behaviors do not have their own fakes, onCall can not be refined with withArgs.
// DOES NOT WORK
stub.onFirstCall()
  .withArgs(1).returns(1)
  .withArgs(2).returns(2);       

There simply is no withArgs method on the behavior returned by onFirstCall. It does work the other way around though:

// WORKS
stub.returns(3);
stub.withArgs(1)
  .returns(0)
  .onFirstCall().returns(1);      
  • onCall does not return stub but behavior.
// WRONG
var stub = sinon.stub().returns(0).onFirstCall().returns(1);
stub.yields(4); // Only yields on first call

I really see no other way though. And chaining works:

// WORKS
var stub = sinon.stub().returns(0);
stub
  .onFirstCall().returns(1)
  .onSecondCall().returns(2);

Behavior#onCall delegates back to stub.

@cjohansen
Owner

I don't necessarily have a problem with this, provided behavior.withArgs is defined to throw an exception with a helpful error message.

What is the expected outcome of this btw?

var stub = sinon.stub().withArgs(1).returns(0).onFirstCall().returns(1); 
stub(1); // ???
@mantoni
Collaborator

I guess it means "the stub returns 0 for calls with args [1] except for the first call where it returns 1".

stub(1); // 1
stub(1); // 0
stub(1); // 0
// ...
stub(2); // undefined
@cjohansen
Owner

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

@tf
tf commented

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
Owner

Is it complete? Sorry for falling out here.

@tf
tf commented

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
tf commented

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
tf commented

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
Collaborator

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
Owner

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 tf referenced this issue from a commit in tf/Sinon.JS
@tf tf refactor parallel stub arrays into behavior objects (#244) 96d193f
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf move async method generation to stub behavior (#244) eed5b48
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf add onCall to stubs for building sequences (#244) 0d59d53
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf allow onCall to be combined with returns* and throwsException in stub…
… behavior sequences (#244)
f371f19
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf move callback sequence tests in onCall section (#244) be6f615
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf make fakes delegate to parent stub behvior (#244)
* store a reference back to the parent stub on a fake
* if a fake neither has a behavior for the current call index nor a
  default bahavior, delegate to the parent
* as a result calling resetBehavior on a fake makes it fall back to
  its parent behavior again
7aa72b6
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf delegate withArgs on behavior back to stub (#244)
* pass callIndex to behavior
* delegate withArgs back to stub invoking matching onCall
19cd7e2
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf invoking onCall alone should not alter stub behavior (#244)
otherwise delegation of onFirstCall().withArgs(...) to
withArgs(...).onFirstCall() creates a blank behavior for first
call. Calls not matching the specified args do not fall back to
default stub behavior then.
dbbf231
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf extract behavior to own file (#244) 228e9c8
@tf
tf commented

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
tf commented

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
Collaborator
@tf tf referenced this issue from a commit in tf/Sinon.JS
@tf tf remove mentions of non existing thenYields (#244) 185c3d7
@tf
tf commented

Great. Submitted the PR.

@mantoni
Collaborator

Closing this issue since the PR #338 was merged.

@mantoni mantoni closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.