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

Implemented spy.returns method #35

Merged
merged 1 commit into from Oct 13, 2015

Conversation

@stalniy
Copy link
Contributor

commented Jul 26, 2015

server.isAvailable = chai.spy.returns(true);

expect(server.isAvailable()).to.be.true;
@keithamus

This comment has been minimized.

Copy link
Member

commented Jul 26, 2015

While I love the idea of having a returns method, I wonder if it'd be better on the spy prototype - because we can use it in multiple places then. e.g:

server.isAvailable = chai.spy().returns(true);
// or
chai.spy.on(server, 'isAvailable').returns(true);

expect(server.isAvailable()).to.be.true;

server.isAvailable.returns(false);

expect(server.isAvailable()).to.be.false;
@stalniy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2015

@keithamus I don't like the idea of mutating inner implementation of spy function. If you want to return false just create another spy. That's more clear:

server.isAvailable = chai.spy.returns(true);
expect(server.isAvailable()).to.be.true;

server.isAvailable = chai.spy.returns(false);
expect(server.isAvailable()).to.be.false;

* EDIT*
There is no need to use spy.on if you don't want to use inner implementation of object's method.

@keithamus

This comment has been minimized.

Copy link
Member

commented Jul 26, 2015

@stalniy a spy is already mutable, that precedent has been set. The spy isn't just a function, it has a prototype, it has .reset() which mutates the state. We're far from the only spy framework to do so (Sinon and Jasmine).

I'm quite comfortable adding .returns() to the prototype. And using spy.on to subsequently override behaviour could also be quite useful.

Thoughts?

@stalniy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2015

@keithamus I have been thinking about this for a while, for example jasmine has and.returnValue and and.callFake because their spy constructor doesn't accept implementation like yours does. Immutable state is good because you have guarantee that nobody can change it and you protect yourself from writing mess like:

var spy = spy(function() {
  // some complex logic
  return 1;
});
expect(spy() + 1).to.equal(2);

spy.returns(2); // why would somebody overrides my complex logic (single responsibility!)?
expect(spy() + 1).to.equal(3);

spy.returns(3);
expect(spy() + 1).to.equal(4);

People will put in the code too much resetting logic and complexity will grow.

I disagree with jasmine style of spyOn. For example, lets take a look at design:

Chai-spies Jasmine Comment
chai.spy.on(foo, 'getBar') spyOn(foo, 'getBar').and.callThrough(); spy on object method sounds more clear and straightforward than spy on object method and call through. spy on emphasize that I want to spy existing object's method but in jasmine it replaces implementation instead (unclear behavior, there is a difference between what I write and what I get)
foo.getBar = chai.spy.returns(true) spyOn(foo, 'getBar').and.returnValue(true) here I want to replace the implementation of method and code tells you about this in the most clear manner. chai-spies - set foo.getBar to chai spy which returns true, jasmine - spy on foo getBar and return value

And again if you want to change method implementation no need to use spy.on just create regular spy, spy.on is a sugar for cases when you want to spy existing object's method (also in the next major version spy.on will return passed object, so you won't be able to write spy.on(foo, 'bar').returns(true)).

I haven't worked with sinon but what I see in chai-spies is clear, simple and the most intuitive implementation of spy framework. So, lets keep it simple and intuitive =)

P.S.: I disagree with having reset method for spy. I can't imagine the proper use case for this, instead of changing spy's state you always can create new one (it's not smth that takes too much memory or time).

EDIT: the only issue which I currently see with using direct assignment on objects method is that then in afterEach somebody would like to restore original method and for this he needs to create additional variable (e.g. jasmine cleans up all spies and restores original methods in afterEach callback)

@PhilVargas

This comment has been minimized.

Copy link

commented Aug 15, 2015

any resolution on this? this would be a great feature to have

keithamus added a commit that referenced this pull request Oct 13, 2015
Implemented spy.returns method
@keithamus keithamus merged commit 58f50ec into chaijs:master Oct 13, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@keithamus

This comment has been minimized.

Copy link
Member

commented Oct 13, 2015

@stalniy you make some good points. I've merged this for now - because I think this is a pretty safe starting point. It'd be nice to get a good roadmap in place for chai-spies - to properly bikeshed out what we do in situations like these; formulate a kind of "design policy" if you will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.