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

Return a proper stub for custom subbed methods with callsFake #823

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@hurrymaplelad
Contributor

hurrymaplelad commented Aug 30, 2015

Here's the problem I was having:

// In the preamble of a suite, fail if an error is logged, even in a try ... catch
sinon.stub(logger, 'error', function() {
  setImmediate(function() {
    throw new Error("Error logged in test")
  });
})

// later, testing a path that should error
logger.error.resetBehavior() // <- logger.error has no reset behavior!
expect(logger.error).to.have.been.called

The quickest path to implement was to pull in the proposed callsFake from #768. I understand callsFake style behavior has also been discussed in #278. I hope this PR strikes the right balance of expanding the API a little bit to make the library more productive.

Benefits:

  • Custom stubs are now composable. Ex: sinon.stub(beep, 'boop').onSecondCall().callsFake(x => {value: x})
  • It's easier to switch from sinon.stub(beep, 'boop').returns(x) to sinon.stub(beep, 'boop').callsFake(x => {value: x}) as I'm thinking through my test. Less syntax thrashing.
  • Sinon is more consistent with Jasmine, making it easier for new developers and reducing context switching.
  • The api is more internally consistent and predictable: sinon.stub() now always returns a stub

Drawbacks:

  • There are 2 ways to define custom stub behavior. callsFake could be the only syntax after a major version bump.
@mantoni

This comment has been minimized.

Show comment
Hide comment
@mantoni

mantoni Aug 30, 2015

Member

Yes, I understand the reasoning behind it and I like the approach you took in the implementation. However, your last point is actually the problem with this: if there are two ways to define behavior, what should happen when callsFake and yields are both used? I'm not so much of a fan of replacing the other functions with something syntactically different.

So, if there is a way to make all the behaviors work consistently, I'm fine with this. Putting something in that adds to the confusion, not so much. Any ideas?

Member

mantoni commented Aug 30, 2015

Yes, I understand the reasoning behind it and I like the approach you took in the implementation. However, your last point is actually the problem with this: if there are two ways to define behavior, what should happen when callsFake and yields are both used? I'm not so much of a fan of replacing the other functions with something syntactically different.

So, if there is a way to make all the behaviors work consistently, I'm fine with this. Putting something in that adds to the confusion, not so much. Any ideas?

@hurrymaplelad

This comment has been minimized.

Show comment
Hide comment
@hurrymaplelad

hurrymaplelad Aug 31, 2015

Contributor

@mantoni, great point. Here's the rules in my head for composing behaviors. They may not totally match implementation today, but this is what I'd shoot for:

  1. Last-in-wins for any stubbed behavior:
stub().returns().throws() // only throws, doesn't return
stub().throws().returns(1).returns(2) // only returns 2, doesn't throw
stub().callsFake(->).yields(2) // only calls callback, doesn't call fake
  1. Qualified stubs take precedence over the unqualified examples above:
stub().returns(1).onFirstCall().returns(2) // returns 2 then 1 forever
stub().returns(1).onFirstCall().returns(2).throws() // throws then returns 1 forever

I imagine last-in-wins rules would be pretty easy to add more uniformly with an internal reset method on behaviors that's called by each of the throws/returns/callsFake style methods.

Happy to add that here or in a separate PR if that direction sounds good to you.

Contributor

hurrymaplelad commented Aug 31, 2015

@mantoni, great point. Here's the rules in my head for composing behaviors. They may not totally match implementation today, but this is what I'd shoot for:

  1. Last-in-wins for any stubbed behavior:
stub().returns().throws() // only throws, doesn't return
stub().throws().returns(1).returns(2) // only returns 2, doesn't throw
stub().callsFake(->).yields(2) // only calls callback, doesn't call fake
  1. Qualified stubs take precedence over the unqualified examples above:
stub().returns(1).onFirstCall().returns(2) // returns 2 then 1 forever
stub().returns(1).onFirstCall().returns(2).throws() // throws then returns 1 forever

I imagine last-in-wins rules would be pretty easy to add more uniformly with an internal reset method on behaviors that's called by each of the throws/returns/callsFake style methods.

Happy to add that here or in a separate PR if that direction sounds good to you.

@mantoni

This comment has been minimized.

Show comment
Hide comment
@mantoni

mantoni Aug 31, 2015

Member

@cjohansen @mroderick What are your thoughts on this?

Member

mantoni commented Aug 31, 2015

@cjohansen @mroderick What are your thoughts on this?

@cjohansen

This comment has been minimized.

Show comment
Hide comment
@cjohansen

cjohansen Sep 1, 2015

Contributor

Well this does make sense:

The api is more internally consistent and predictable: sinon.stub() now always returns a stub

However, I'm not sure I understand the reasoning behind the original example. What were you hoping to achieve with this?

logger.error.resetBehavior()

I think the point of callsFake vs yields is mute, as we already have this problem with returns and throws. @hurrymaplelad's suggested solution to this looks reasonable to me.

Contributor

cjohansen commented Sep 1, 2015

Well this does make sense:

The api is more internally consistent and predictable: sinon.stub() now always returns a stub

However, I'm not sure I understand the reasoning behind the original example. What were you hoping to achieve with this?

logger.error.resetBehavior()

I think the point of callsFake vs yields is mute, as we already have this problem with returns and throws. @hurrymaplelad's suggested solution to this looks reasonable to me.

@mantoni

This comment has been minimized.

Show comment
Hide comment
@mantoni

mantoni Sep 1, 2015

Member

@cjohansen Yes, right. On second thought, returns and yields even make sense together, while returns and throws doesn't.

Member

mantoni commented Sep 1, 2015

@cjohansen Yes, right. On second thought, returns and yields even make sense together, while returns and throws doesn't.

@hurrymaplelad

This comment has been minimized.

Show comment
Hide comment
@hurrymaplelad

hurrymaplelad Sep 1, 2015

Contributor

@cjohansen In the example above I want any calls to logger.error to fail the test suite, and I want to be able to whitelist individual specs. I'm using resetBehavior() to do the whitelisting. Specs that reset logger.error may then assert it was called with the expected error. I'd also be fine using .returns() or any method that causes logger.error not to fail the suite. Currently I can't 'cause it's not a stub.

In general I like to use this pattern when I'm testing code that needs to handle errors, often integrations with external services. The code under test often has a liberal try catch, with a logger.error in the catch to prevent errors from propagating in distributed systems. In these tests, I want to treat logger.error like an uncaught exception, cause it could be syntax error. If a test isn't covering the error handling path and asserting on the logged message, it should fail.

Contributor

hurrymaplelad commented Sep 1, 2015

@cjohansen In the example above I want any calls to logger.error to fail the test suite, and I want to be able to whitelist individual specs. I'm using resetBehavior() to do the whitelisting. Specs that reset logger.error may then assert it was called with the expected error. I'd also be fine using .returns() or any method that causes logger.error not to fail the suite. Currently I can't 'cause it's not a stub.

In general I like to use this pattern when I'm testing code that needs to handle errors, often integrations with external services. The code under test often has a liberal try catch, with a logger.error in the catch to prevent errors from propagating in distributed systems. In these tests, I want to treat logger.error like an uncaught exception, cause it could be syntax error. If a test isn't covering the error handling path and asserting on the logged message, it should fail.

@cjohansen

This comment has been minimized.

Show comment
Hide comment
@cjohansen

cjohansen Sep 1, 2015

Contributor

Why aren't you using .throws to fail the test suite?

Contributor

cjohansen commented Sep 1, 2015

Why aren't you using .throws to fail the test suite?

@hurrymaplelad

This comment has been minimized.

Show comment
Hide comment
@hurrymaplelad

hurrymaplelad Sep 5, 2015

Contributor

@cjohansen great question! I'd love to use .throws but it's not powerful enough to reliably fail the test suite in places where I like to log errors. The most common case for me is testing HTTP endpoints. Here's a smallish example where logger.error.throws() is not enough:

express = require 'express'
request = require 'request'
app = express()
logger =
  error: -> throw new Error('Unexpected logger.error')

app.get '/', (req, res) ->
  throw new Error 'Oops!'

app.use (err, req, res, next) ->
  logger.error err
  next()

app.use (err, req, res, next) ->
  res.status(500).send()

server = app.listen 3000, ->
  server.unref()

# In my test code:
request.get 'http://localhost:3000'
# This will hit logger.error and throw, but express will catch the error and return a 500.  
Contributor

hurrymaplelad commented Sep 5, 2015

@cjohansen great question! I'd love to use .throws but it's not powerful enough to reliably fail the test suite in places where I like to log errors. The most common case for me is testing HTTP endpoints. Here's a smallish example where logger.error.throws() is not enough:

express = require 'express'
request = require 'request'
app = express()
logger =
  error: -> throw new Error('Unexpected logger.error')

app.get '/', (req, res) ->
  throw new Error 'Oops!'

app.use (err, req, res, next) ->
  logger.error err
  next()

app.use (err, req, res, next) ->
  res.status(500).send()

server = app.listen 3000, ->
  server.unref()

# In my test code:
request.get 'http://localhost:3000'
# This will hit logger.error and throw, but express will catch the error and return a 500.  
@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Feb 10, 2016

Contributor

Looking through older pull requests I have come to this, which has been stuck for half a year. I have to admit I am not getting which problem this is fixing, except for returning a proper stub. Any ideas on how to make this progress?

Contributor

fatso83 commented Feb 10, 2016

Looking through older pull requests I have come to this, which has been stuck for half a year. I have to admit I am not getting which problem this is fixing, except for returning a proper stub. Any ideas on how to make this progress?

@hurrymaplelad

This comment has been minimized.

Show comment
Hide comment
@hurrymaplelad

hurrymaplelad Feb 21, 2016

Contributor

@fatso83 thanks for the bump!

I'd like to fail a suite that tests an HTTP server through it's API if it calls logger.error. I'd also like test authors to be able to whitelist individual test to not fail the suite by re-defining logger.error.

This is all possible today with a layer of tooling on top of sinon, but it looks like there's an opportunity to shore-up sinon and delete my extra layer.

As far as moving it forward, I've heard hesitation but no strong objections. I'm happy to put together a PR, but I'd like to hear that maintainers are onboard with direction before I put in the work.

Contributor

hurrymaplelad commented Feb 21, 2016

@fatso83 thanks for the bump!

I'd like to fail a suite that tests an HTTP server through it's API if it calls logger.error. I'd also like test authors to be able to whitelist individual test to not fail the suite by re-defining logger.error.

This is all possible today with a layer of tooling on top of sinon, but it looks like there's an opportunity to shore-up sinon and delete my extra layer.

As far as moving it forward, I've heard hesitation but no strong objections. I'm happy to put together a PR, but I'd like to hear that maintainers are onboard with direction before I put in the work.

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Jul 27, 2016

Contributor

This PR was quite confusing as the first part did not really address the actual changes, and a lot of the discussion went into how and whys of the supplied test case, which further obscured things.

Basically this PR is about replacing:

var stub = sinon.stub(foo, 'bar', myCustomFunc); // does not return a real stub

with

var stub = sinon.stub(foo, 'bar').callsFake(myCustomFunc); // returns a real stub

A tad bit more verbose, but to me it seems fine.

It's great to remove the inconsistency of todays API, where custom stubs are not true stubs, but to avoid functional duplicity (and inconsistency) we should remove the current way of declaring custom stubs as well. That's a breaking change which should be included before 2.0 is out.

The maintainers (including me) seem to agree that the proposed rules for composing behaviors seem sound. They are not directly related and could be merged before this. Would you care to create a PR for that, @hurrymaplelad? Following that, I would like to merge this, but it would need to remove the current custom stubbing method as well, me thinks.

Contributor

fatso83 commented Jul 27, 2016

This PR was quite confusing as the first part did not really address the actual changes, and a lot of the discussion went into how and whys of the supplied test case, which further obscured things.

Basically this PR is about replacing:

var stub = sinon.stub(foo, 'bar', myCustomFunc); // does not return a real stub

with

var stub = sinon.stub(foo, 'bar').callsFake(myCustomFunc); // returns a real stub

A tad bit more verbose, but to me it seems fine.

It's great to remove the inconsistency of todays API, where custom stubs are not true stubs, but to avoid functional duplicity (and inconsistency) we should remove the current way of declaring custom stubs as well. That's a breaking change which should be included before 2.0 is out.

The maintainers (including me) seem to agree that the proposed rules for composing behaviors seem sound. They are not directly related and could be merged before this. Would you care to create a PR for that, @hurrymaplelad? Following that, I would like to merge this, but it would need to remove the current custom stubbing method as well, me thinks.

@jnystrom

This comment has been minimized.

Show comment
Hide comment
@jnystrom

jnystrom Aug 18, 2016

With this change would I be able to call different fakes based one onFirstCall vs onSecondCall? I would like to be able to do:

sinon.stub(service, 'method')
       .onFirstCall().callsFake((data)=>data)
       .onSecondCall().callsFake((data) => data + ' second call');

Is it possible to reference the arg passed into the stub when the function is actually called many times and you want to deal with it differently on different calls?

jnystrom commented Aug 18, 2016

With this change would I be able to call different fakes based one onFirstCall vs onSecondCall? I would like to be able to do:

sinon.stub(service, 'method')
       .onFirstCall().callsFake((data)=>data)
       .onSecondCall().callsFake((data) => data + ' second call');

Is it possible to reference the arg passed into the stub when the function is actually called many times and you want to deal with it differently on different calls?

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Aug 18, 2016

Contributor

@jnystrom: that's my understanding. I am not totally sure what you are asking concerning the second question. If you create a fake stubbing implementation you have direct access to the argument at all times.

Contributor

fatso83 commented Aug 18, 2016

@jnystrom: that's my understanding. I am not totally sure what you are asking concerning the second question. If you create a fake stubbing implementation you have direct access to the argument at all times.

@jnystrom

This comment has been minimized.

Show comment
Hide comment
@jnystrom

jnystrom Aug 18, 2016

@fatso83, thank you. Below is an example:

 const repository = new Repository();
    sinon.stub(repository, 'save').onFirstCall()
        .returns(Promise.resolve(initialSurprise))
        .onSecondCall().returns(
            Promise.resolve({ _id: '1234123' }))
        .onThirdCall().returns(
            Promise.resolve({ _id: '1234123' }));

I would like to be able to be able to return something different based on the args passed each time, like:

 const repository = new Repository();
    sinon.stub(repository, 'save').onFirstCall((data) => Promise.resolve(initialSurprise)))
        .onSecondCall((data) => Promise.resolve({ _id: '12345', data: data + 'second call' }))
        .onThirdCall((data) => Promise.resolve({ _id: '123456', data: data + 'third call' }));

Is there a way to get the args passed in at each sequential call within the stub?

jnystrom commented Aug 18, 2016

@fatso83, thank you. Below is an example:

 const repository = new Repository();
    sinon.stub(repository, 'save').onFirstCall()
        .returns(Promise.resolve(initialSurprise))
        .onSecondCall().returns(
            Promise.resolve({ _id: '1234123' }))
        .onThirdCall().returns(
            Promise.resolve({ _id: '1234123' }));

I would like to be able to be able to return something different based on the args passed each time, like:

 const repository = new Repository();
    sinon.stub(repository, 'save').onFirstCall((data) => Promise.resolve(initialSurprise)))
        .onSecondCall((data) => Promise.resolve({ _id: '12345', data: data + 'second call' }))
        .onThirdCall((data) => Promise.resolve({ _id: '123456', data: data + 'third call' }));

Is there a way to get the args passed in at each sequential call within the stub?

@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Nov 10, 2016

Member

Is this still relevant? If so, then it needs a rebase before we can move forwards with it. Perhaps it needs to be re-opened against v1.17 branch?

Member

mroderick commented Nov 10, 2016

Is this still relevant? If so, then it needs a rebase before we can move forwards with it. Perhaps it needs to be re-opened against v1.17 branch?

@hurrymaplelad

This comment has been minimized.

Show comment
Hide comment
@hurrymaplelad

hurrymaplelad Nov 10, 2016

Contributor

Still relevant! I'm back from summer travels now and happy to pick this up. I like the direction proposed in by @fatso83:

The maintainers (including me) seem to agree that the proposed rules for composing behaviors seem sound. They are not directly related and could be merged before this. Would you care to create a PR for that, @hurrymaplelad? Following that, I would like to merge this, but it would need to remove the current custom stubbing method as well, me thinks.

I'll create two separate PRs:

  • one for consistently composing behaviors, PR'd into master or 1.17
  • a second favoring callsFake() and removing the 3 argument syntax for stub(), PR'd into 2.0
    • I'll aim to add codemod scripts to automate the syntax change for folks who want to upgrade.
Contributor

hurrymaplelad commented Nov 10, 2016

Still relevant! I'm back from summer travels now and happy to pick this up. I like the direction proposed in by @fatso83:

The maintainers (including me) seem to agree that the proposed rules for composing behaviors seem sound. They are not directly related and could be merged before this. Would you care to create a PR for that, @hurrymaplelad? Following that, I would like to merge this, but it would need to remove the current custom stubbing method as well, me thinks.

I'll create two separate PRs:

  • one for consistently composing behaviors, PR'd into master or 1.17
  • a second favoring callsFake() and removing the 3 argument syntax for stub(), PR'd into 2.0
    • I'll aim to add codemod scripts to automate the syntax change for folks who want to upgrade.
@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Nov 10, 2016

Member

👍

Member

mroderick commented Nov 10, 2016

👍

@hurrymaplelad

This comment has been minimized.

Show comment
Hide comment
@hurrymaplelad

hurrymaplelad Dec 5, 2016

Contributor

Looks like we'll only need the second PR. All my test cases for consistently composing behaviors pass on master.

Contributor

hurrymaplelad commented Dec 5, 2016

Looks like we'll only need the second PR. All my test cases for consistently composing behaviors pass on master.

@hurrymaplelad

This comment has been minimized.

Show comment
Hide comment
@hurrymaplelad

hurrymaplelad Dec 5, 2016

Contributor

First pass at a codemod script is up: https://github.com/hurrymaplelad/sinon-codemod

PR for stub(x,y,z) -> stub(x,y).callsFake(z) is next.

Contributor

hurrymaplelad commented Dec 5, 2016

First pass at a codemod script is up: https://github.com/hurrymaplelad/sinon-codemod

PR for stub(x,y,z) -> stub(x,y).callsFake(z) is next.

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