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

.returns not overriding .callThrough #1499

Closed
henit opened this issue Jul 27, 2017 · 4 comments
Closed

.returns not overriding .callThrough #1499

henit opened this issue Jul 27, 2017 · 4 comments
Labels

Comments

@henit
Copy link

henit commented Jul 27, 2017

Sinon 2.4.1

let obj = {
    foo: () => 42
};

sinon.stub(obj, 'foo').callThrough();

console.info(obj.foo()); // 42

obj.foo.returns(43);
console.info(obj.foo()); // 42

obj.foo.resolves(44); 
console.info(obj.foo()); // Promise { 44 }

So, stub.resolves overrides callThrough, but not stub.returns (unless i call stub.reset between). Bug or feature? If feature, can you please explain why it works this way.

@fatso83
Copy link
Contributor

fatso83 commented Jul 28, 2017

Should it? Basically it doesn't make sense to overwrite the previous behaviour, so how this behaves is unspecified. We could of course try to detect all nonsensical use of the API, but I'd just let adults be adults and not add superfluous behaviour to the stubs. Why specify that it should call through if you intend to always let it return another value? My two cents - others might disagree.

@henit
Copy link
Author

henit commented Jul 31, 2017

If the library is used in a way that "doesn't make sense", that is not a too unlikely scenario as it is a general test library that is used by thousands of different devs in a endless set of scenarios and architectures. As a test library does not have the same performance requirements as a lib used in production, I would personally vote to throw exception when the lib is used wrong, but I can live with the lack of it as long as I understand why things work the way they do.

My point was that I don't understand why one different behaviour (setting .resolves) overrides the previously set .callThrough, while another (setting .returns) does not. It seems to make more sens that all behaviour that is not possible to combine with what has been set previously would either be ignored (as it is when setting .returns in the above example) or overridden (as it is with .resolves).

@fatso83
Copy link
Contributor

fatso83 commented Aug 1, 2017

Sure, consistent behavior would be the best. The "last call overrides previous calls" is how it is with other methods (see for instance #1432). So either we could try to do this here as well, or we can start implementing logic in various places to try and catch illogical configuration combinations. While I always prefer throwing an error on wrong usage, trying to implement the logic seems like quite the endeavour - as you would need to add calls to this "config monitor" all over Sinon.

@stale
Copy link

stale bot commented Jan 13, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 13, 2018
@stale stale bot closed this as completed Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants