Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onCall method for building sequential stub behavior #338

Merged
merged 12 commits into from
Oct 28, 2013

Conversation

tf
Copy link
Contributor

@tf tf commented Oct 18, 2013

The full discussion of this change can be found in #244.

Major changes:

  • Calling yields* or callArg* methods on stubs multiple times no longer defines sequential behavior but redefines the behavior.
  • New onCall(n), onFirstCall, onSecondCall, onThirdCall methods to define sequential behavior. All stub behavior defining methods (including returns*, throws*, withArgs) can be used. Arbitrary combinations of withArgs and onCall work.

Code organization changes:

  • Extract behavior class from parallel arrays in stubs.

tf added 12 commits October 3, 2013 15:25
* 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
* pass callIndex to behavior
* delegate withArgs back to stub invoking matching onCall
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.
@mantoni
Copy link
Member

mantoni commented Oct 23, 2013

Impressive amount of work!

From looking through the changes it does look good to me. The only thing I noticed are some single quotes mixed with double quotes in the test cases.

I'd volunteer to run my test suites at work using a snapshot build if @cjohansen could provide one?
Maybe here: http://sinonjs.org/releases/sinon-next.js

Thanks @tf, great stuff.

@cjohansen
Copy link
Contributor

@mantoni
Copy link
Member

mantoni commented Oct 23, 2013

That was quick! I've run the current test suites successfully with the pre-release against

  • Chrome 30
  • Firefox 23
  • IE 8
  • IE 9

There was one incompatibility that I found: Using yields in a chain does not work anymore.

var stub = sinon.stub();
stub.yields(3).yields(7);

var args = [];
stub(function (arg) { args.push(arg); });
stub(function (arg) { args.push(arg); });

assertEquals([3, 7], args); // throws AssertError: expected [3,7] but was [7,7]

This worked previously and would need to be changed with the pre-release:

stub.onFirstCall().yields(3).onSecondCall().yields(7);

I do like this because it reads a lot better and I'm not sure whether I used an "official" part of the API.

@cjohansen Do you think we need to support the chained yields notation?

@cjohansen
Copy link
Contributor

@cjohansen Do you think we need to support the chained yields notation?

Well, it depends on how much it is used. I'm very reluctant to break people's test suites :/

@mantoni
Copy link
Member

mantoni commented Oct 24, 2013

Hard to tell. It's not in the documentation and personally, I used it once. But can't speak for others, but I'd be fine with breaking this since we have an alternative notation to achieve the same thing. Could be stated in the release notes as a caveat.

@tf
Copy link
Contributor Author

tf commented Oct 24, 2013

If you take a look at my first post in the original discussion #244, this incompatible change actually was the whole point which got me started on this issue. The chaining behavior was introduced in some 1.x minor release and is undocumented. So I'd be very much in favor of changing it back. Please take a look at my first post in #244 again to see my use case which is not possible with the chaining API.

cjohansen added a commit that referenced this pull request Oct 28, 2013
onCall method for building sequential stub behavior
@cjohansen cjohansen merged commit bc97c5e into sinonjs:master Oct 28, 2013
@cjohansen
Copy link
Contributor

You're right. sorry!

@cjohansen
Copy link
Contributor

...and thanks for all your work on this!

@tf
Copy link
Contributor Author

tf commented Oct 29, 2013

Great to see this got merged! Is there a recommended process for contributing to the documentation? Shall i prepare a pull request to the doc site repository? How shall the sinon version requirement for the onCall feature be expressed?

@mantoni
Copy link
Member

mantoni commented Oct 29, 2013

You can send a documentation pull request here: https://github.com/cjohansen/sinon-web
I guess the version will be 1.8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants