One-liner conditional stub returns an unconditional stub #817

Closed
searls opened this Issue Aug 25, 2015 · 22 comments

Comments

Projects
None yet
10 participants
@searls

searls commented Aug 25, 2015

When a conditional stub is setup in a one-liner (such that the value that's retained by the test is whatever gets returned by returns()), then the stub is in fact unconditional, which is very surprising and almost definitely wrong.

Expected

When I do this in a test:

stub = sinon.stub().withArgs("foo").returns("bar")`

I expect:

expect(stub("not foo")).toBe(undefined)

But it is actually "bar".

Workaround

Apparently, what every Sinon user has been doing to this point is:

stub = sinon.stub()
stub.withArgs("foo").returns("bar")`

Which behaves as you'd expect, but is (IMHO) needlessly verbose.

Contributor guideline notes

  • Env: Affects all versions
  • Version: 1.16.1
  • How: Using console in the homepage, using npm, using the download script
  • Other libs: no
@cjohansen

This comment has been minimized.

Show comment
Hide comment
@cjohansen

cjohansen Aug 26, 2015

Contributor

This has probably snuck in with the withArgs modifier. I agree that the current behavior is less than optimal and should probably be changed. However, that would be a very breaking change, and is not likely to happen anytime soon.

The reason for this behavior is the following:

sinon.stub().withArgs("foo").returns("bar").withArgs("fooz").returns("ball")

This is a great illustration of the pitfalls of highly chainable APIs, and in this case,Sinon has unfortunately opted to support the wrong case IMO.

Anyway, as I said, too many people depend on this functionality at this point to allow us to just "fix" it, but we certainly should consider fixing it long term.

Contributor

cjohansen commented Aug 26, 2015

This has probably snuck in with the withArgs modifier. I agree that the current behavior is less than optimal and should probably be changed. However, that would be a very breaking change, and is not likely to happen anytime soon.

The reason for this behavior is the following:

sinon.stub().withArgs("foo").returns("bar").withArgs("fooz").returns("ball")

This is a great illustration of the pitfalls of highly chainable APIs, and in this case,Sinon has unfortunately opted to support the wrong case IMO.

Anyway, as I said, too many people depend on this functionality at this point to allow us to just "fix" it, but we certainly should consider fixing it long term.

@searls

This comment has been minimized.

Show comment
Hide comment
@searls

searls Aug 26, 2015

I understand the tension and appreciate your candor (agreeing but also favoring stability).

I think in the short-term what I'll do with my own project using Sinon.js is to wrap this API with functions that configure the narrow set of doubles I want to see, which will work around this issue.

The only recommendation I have for you as a maintainer is perhaps to consider an alternative API for setting up a one-and-done stub configuration.

searls commented Aug 26, 2015

I understand the tension and appreciate your candor (agreeing but also favoring stability).

I think in the short-term what I'll do with my own project using Sinon.js is to wrap this API with functions that configure the narrow set of doubles I want to see, which will work around this issue.

The only recommendation I have for you as a maintainer is perhaps to consider an alternative API for setting up a one-and-done stub configuration.

@jasonkarns

This comment has been minimized.

Show comment
Hide comment
@jasonkarns

jasonkarns Aug 26, 2015

Contributor

Could this be addressed in a major version bump? I have many of these two-liner instances throughout my test suite that I would much prefer to make one-liners.

And I'll toss in my suspicion that there are more people who would prefer withArgs/returns to return the narrow stub than there are people who do multiple withArgs chaining. I don't think I've ever seen multiple withArgs in a single chain in the wild, though I encounter the two-line method frequently. [confirmation bias alert]

Contributor

jasonkarns commented Aug 26, 2015

Could this be addressed in a major version bump? I have many of these two-liner instances throughout my test suite that I would much prefer to make one-liners.

And I'll toss in my suspicion that there are more people who would prefer withArgs/returns to return the narrow stub than there are people who do multiple withArgs chaining. I don't think I've ever seen multiple withArgs in a single chain in the wild, though I encounter the two-line method frequently. [confirmation bias alert]

@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Aug 26, 2015

Contributor

If we can come up with a reasonable name for it (calledWithArgs?), then I can't see why we can't have both styles present in the API. Deprecate the old style in favour of the new hotness, but leave it in until at least the next major version.

Having both styles will be confusing for newcomers, unless we improve documentation around these features, which is probably long overdue anyway.

Aside: I agree method chaining is often the cause of headaches. It's very difficult to incrementally change design, when everything is dependent on every little detail of the API being the same. Since we've gone this far down the rabbit hole, we might as well keep going ...

Contributor

mroderick commented Aug 26, 2015

If we can come up with a reasonable name for it (calledWithArgs?), then I can't see why we can't have both styles present in the API. Deprecate the old style in favour of the new hotness, but leave it in until at least the next major version.

Having both styles will be confusing for newcomers, unless we improve documentation around these features, which is probably long overdue anyway.

Aside: I agree method chaining is often the cause of headaches. It's very difficult to incrementally change design, when everything is dependent on every little detail of the API being the same. Since we've gone this far down the rabbit hole, we might as well keep going ...

@searls

This comment has been minimized.

Show comment
Hide comment
@searls

searls Aug 26, 2015

Hey @cjohansen after looking at your example one more time:

stub = sinon.stub().withArgs("foo").returns("bar").withArgs("fooz").returns("ball")

I tried punching this in to see what it would do:

stub() // => "ball"
stub("foo") // => "ball"
stub("fooz") // => "ball"

Anyone using a long chain like this is also likely to be surprised that the last-returns-in-wins and that it does so without regarding withArgs, right?

After looking at this example I'm struggling to imagine an example of someone relying on a long chain like this one and actually desiring this behavior. If I had to bet, I'd say this change would break a lot of tests for the better (because they're currently false positives from too-permissive of stubs)

searls commented Aug 26, 2015

Hey @cjohansen after looking at your example one more time:

stub = sinon.stub().withArgs("foo").returns("bar").withArgs("fooz").returns("ball")

I tried punching this in to see what it would do:

stub() // => "ball"
stub("foo") // => "ball"
stub("fooz") // => "ball"

Anyone using a long chain like this is also likely to be surprised that the last-returns-in-wins and that it does so without regarding withArgs, right?

After looking at this example I'm struggling to imagine an example of someone relying on a long chain like this one and actually desiring this behavior. If I had to bet, I'd say this change would break a lot of tests for the better (because they're currently false positives from too-permissive of stubs)

@cjohansen

This comment has been minimized.

Show comment
Hide comment
@cjohansen

cjohansen Aug 28, 2015

Contributor

Wow, that is interesting. Seeing this I can't even understand what the point of withArgs is? 😕 This is definitely an argument for just making this behave correctly.

Contributor

cjohansen commented Aug 28, 2015

Wow, that is interesting. Seeing this I can't even understand what the point of withArgs is? 😕 This is definitely an argument for just making this behave correctly.

@searls

This comment has been minimized.

Show comment
Hide comment
@searls

searls Aug 31, 2015

Thanks for keeping an open mind to that @cjohansen I really appreciate it. What's the best way to proceed from here? Would you be open to a PR?

searls commented Aug 31, 2015

Thanks for keeping an open mind to that @cjohansen I really appreciate it. What's the best way to proceed from here? Would you be open to a PR?

@mantoni

This comment has been minimized.

Show comment
Hide comment
@mantoni

mantoni Aug 31, 2015

Member

This is somewhat related to #823.

Member

mantoni commented Aug 31, 2015

This is somewhat related to #823.

@cjohansen

This comment has been minimized.

Show comment
Hide comment
@cjohansen

cjohansen Sep 1, 2015

Contributor

@searls sure, a PR would be great. The current behavior is completely broken for chains defining more than one behavior anyway.

Contributor

cjohansen commented Sep 1, 2015

@searls sure, a PR would be great. The current behavior is completely broken for chains defining more than one behavior anyway.

@searls

This comment has been minimized.

Show comment
Hide comment
@searls

searls Sep 1, 2015

Cool! I'll schedule some time to look at this next week

searls commented Sep 1, 2015

Cool! I'll schedule some time to look at this next week

@hellboy81

This comment has been minimized.

Show comment
Hide comment
@hellboy81

hellboy81 Sep 2, 2015

This chaining works sometimes:

findStub = sandbox.stub()
            findStub
                .withArgs(...)
               .returns(..)
              .withArgs(...)
               .returns(...)

This does not works properly:

findStub = sandbox.stub().withArgs(...)
               .returns(..)
              .withArgs(...)
               .returns(...)

This chaining works sometimes:

findStub = sandbox.stub()
            findStub
                .withArgs(...)
               .returns(..)
              .withArgs(...)
               .returns(...)

This does not works properly:

findStub = sandbox.stub().withArgs(...)
               .returns(..)
              .withArgs(...)
               .returns(...)
@searls

This comment has been minimized.

Show comment
Hide comment
@searls

searls Sep 10, 2015

@cjohansen I'm really sorry for being "that guy" but I've shifted gears away from using Sinon.js for the project I'm working on, so I won't be tackling this with a PR on my own. Feel free to close/deprioritize accordingly based on your own interests, of course.

searls commented Sep 10, 2015

@cjohansen I'm really sorry for being "that guy" but I've shifted gears away from using Sinon.js for the project I'm working on, so I won't be tackling this with a PR on my own. Feel free to close/deprioritize accordingly based on your own interests, of course.

@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Sep 10, 2015

Contributor

@cjohansen I'm really sorry for being "that guy" but I've shifted gears away from using Sinon.js for the project I'm working on, so I won't be tackling this with a PR on my own. Feel free to close/deprioritize accordingly based on your own interests, of course.

Thank you for getting back on this! I'll apply the help-wanted label, and see if it'll grab the attention of someone with time to take a deeper look into this.

Also, thank you for your investigation into this issue!

Contributor

mroderick commented Sep 10, 2015

@cjohansen I'm really sorry for being "that guy" but I've shifted gears away from using Sinon.js for the project I'm working on, so I won't be tackling this with a PR on my own. Feel free to close/deprioritize accordingly based on your own interests, of course.

Thank you for getting back on this! I'll apply the help-wanted label, and see if it'll grab the attention of someone with time to take a deeper look into this.

Also, thank you for your investigation into this issue!

@hellboy81

This comment has been minimized.

Show comment
Hide comment
@hellboy81

hellboy81 Sep 10, 2015

pay4bugz ;-)

pay4bugz ;-)

@launchcg-ztonia

This comment has been minimized.

Show comment
Hide comment
@launchcg-ztonia

launchcg-ztonia Apr 1, 2016

Whoa whoa. There is a property on 'withArgs' modified stubs called 'parent' which refers back to the root of the stub.

When I do my chaining I just provide explicitly that I want to pass back up the chain.

var stubby = sinon.stub().throws()
.withArgs("foo").returns("bar").parent
.withArgs("foo2").returns("bar2").parent;

Works pretty well for me. Worried your fix will be a breaking change for me :(

Whoa whoa. There is a property on 'withArgs' modified stubs called 'parent' which refers back to the root of the stub.

When I do my chaining I just provide explicitly that I want to pass back up the chain.

var stubby = sinon.stub().throws()
.withArgs("foo").returns("bar").parent
.withArgs("foo2").returns("bar2").parent;

Works pretty well for me. Worried your fix will be a breaking change for me :(

@mantoni

This comment has been minimized.

Show comment
Hide comment
@mantoni

mantoni Apr 2, 2016

Member

@launchcg-ztonia You're depending on implementation details that are not part of the official API. This will almost certainly break at some point.

Member

mantoni commented Apr 2, 2016

@launchcg-ztonia You're depending on implementation details that are not part of the official API. This will almost certainly break at some point.

@apoorva-shah

This comment has been minimized.

Show comment
Hide comment
@apoorva-shah

apoorva-shah Feb 9, 2017

Here, I want to assert that callback function is called or not. So how can I do it using sinon js. Please suggest.

var send = function (templateId, callback) {

    const request = mailjet
        .post("send")
        .request(params)
    request
        .then((result) => {
            if (typeof callback === 'function') {
                callback(null, result.body);
            }

        })
        .catch((err) => {
            if (typeof callback === 'function') {
                callback(err, null);
            }
        })
    } else {
        callback(err, null);
    }
};

var mailjetClient = require('../../node_modules/node-mailjet/mailjet-client');

sinon.stub(mailjet, 'post').withArgs('send').returns(mailjetClient);
sinon.stub(mailjetClient, 'request').returns(Promise);
I am getting following error:

TypeError: Attempted to wrap undefined property request as function

Here, I want to assert that callback function is called or not. So how can I do it using sinon js. Please suggest.

var send = function (templateId, callback) {

    const request = mailjet
        .post("send")
        .request(params)
    request
        .then((result) => {
            if (typeof callback === 'function') {
                callback(null, result.body);
            }

        })
        .catch((err) => {
            if (typeof callback === 'function') {
                callback(err, null);
            }
        })
    } else {
        callback(err, null);
    }
};

var mailjetClient = require('../../node_modules/node-mailjet/mailjet-client');

sinon.stub(mailjet, 'post').withArgs('send').returns(mailjetClient);
sinon.stub(mailjetClient, 'request').returns(Promise);
I am getting following error:

TypeError: Attempted to wrap undefined property request as function
@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Feb 10, 2017

Contributor

@apoorva-shah: the javascript output is enough to help you. there is no property called request to stub on that object. and please don't use this issue tracker for usage questions: try stack overflow or our Gitter channel.

Contributor

fatso83 commented Feb 10, 2017

@apoorva-shah: the javascript output is enough to help you. there is no property called request to stub on that object. and please don't use this issue tracker for usage questions: try stack overflow or our Gitter channel.

@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick May 27, 2017

Contributor

It would be great if someone could summarise the result of this discussion as a failing test case added to issues-test.js. Then we would know if we accidentally fix it ;)

Contributor

mroderick commented May 27, 2017

It would be great if someone could summarise the result of this discussion as a failing test case added to issues-test.js. Then we would know if we accidentally fix it ;)

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot 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 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 Jan 20, 2018

@richardson-trevor

This comment has been minimized.

Show comment
Hide comment
@richardson-trevor

richardson-trevor Jan 26, 2018

Any chance this will be re-considered in a near-future release? This still appears to be an issue.

Any chance this will be re-considered in a near-future release? This still appears to be an issue.

@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Jan 26, 2018

Contributor

Any chance this will be re-considered in a near-future release?

The issue has been around for years, and we are not seeing much traction for anyone trying to fix it. There is a considerable complexity in the stub api.

I've been working on a new API, that is much simpler #1586. If you're curious, you could try it out and leave feedback on that pull request.

Contributor

mroderick commented Jan 26, 2018

Any chance this will be re-considered in a near-future release?

The issue has been around for years, and we are not seeing much traction for anyone trying to fix it. There is a considerable complexity in the stub api.

I've been working on a new API, that is much simpler #1586. If you're curious, you could try it out and leave feedback on that pull request.

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