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

withArgs(sinon.match()) doesn't use last defined return definition #1053

Closed
jishi opened this issue May 27, 2016 · 9 comments · Fixed by #1183
Closed

withArgs(sinon.match()) doesn't use last defined return definition #1053

jishi opened this issue May 27, 2016 · 9 comments · Fixed by #1183

Comments

@jishi
Copy link

jishi commented May 27, 2016

  • Sinon version : 1.17.4
  • Environment : OS X
  • Example URL :
  • Other libraries you are using:

What did you expect to happen?

I would expect that the last withArgs return declaration to to have precedence when multiple declarations would match the invocation.

What actually happens

The first one seems to have precedence opposed to when you are using strict values instead of sinon.match

How to reproduce

var x = sinon.stub().returns(1);
x.returns(2);

x() // -> 2

var x = sinon.stub();
x.withArgs(1).returns(1);
x.withArgs(1).returns(2);

x(1) // -> 2

var x = sinon.stub();
x.withArgs(sinon.match('1')).returns(1);
x.withArgs(sinon.match('1')).returns(2);

x('1') // -> 1 (wat?)

Probably due to that "strict" declarations actually overwrite the withArgs precondition, and that is not possible with match-objects. However, if they were applied on the opposite order (last defined, first matched), it would match the normal behavior, I think.

@fatso83
Copy link
Contributor

fatso83 commented May 27, 2016

Thanks for reporting. I am unsure if this should be fixed in v1.17 in case anyone relies on this behavior, but a fix for this could be put into master for the upcoming 2.0 release which features some breaking changes.

@jishi
Copy link
Author

jishi commented May 27, 2016

Yeah, since the behavior seem to be undocumented, there is no "fact" to base it on. In an ideal world, it would be even better if it would work like css selectors, meaning most detailed match would have precedence but I have no idea on how that would actually be implemented...

@fatso83
Copy link
Contributor

fatso83 commented May 27, 2016

"Most detailed" would be very hard, especially given javascript's dynamic typing and type conversion rules. If you could patch the basic case (the one submitted) then that would be a candidate for 2.0.

@fatso83
Copy link
Contributor

fatso83 commented Jul 27, 2016

@jishi : do you think you could supply us with a patch? seems you have a good idea on solving this.

@jishi
Copy link
Author

jishi commented Aug 1, 2016

I can look into it. It should go in the master branch then?

@fatso83
Copy link
Contributor

fatso83 commented Aug 1, 2016

Help is always welcome. master is the right branch. We might backport it, of course.

This might be related to #817.

@mroderick
Copy link
Member

Hey @jishi, any chance of getting a PR for this issue?

@jishi
Copy link
Author

jishi commented Nov 10, 2016

I haven't had time to look at it, but I'll free up some time before end of the week.

@jishi
Copy link
Author

jishi commented Nov 10, 2016

Seemed easier than anticipated, made a pull request for review

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

Successfully merging a pull request may close this issue.

3 participants