chai.spy.callsBackWith

[nickcarenza]

implements #65

[keithamus]

Sorry it took so long to get to this!

This looks like a good idea. @stalniy thoughts?

If we do merge this, @nickcarenza - you'll need to rebase this pr and not commit the chai-spies file. That gets build once per release.

[nickcarenza]

Ready to go!

[nickcarenza]

What is this waiting on?

[keithamus]

LGTM. @stalniy happy with this?

[stalniy]

Sorry guys for the delay, was busy with other projects.

Frankly speaking, I think that instead of adding APIs based on callbacks, we should define APIs based on Promises.

Nodejs supports promises natively and any callback based functions can be easily changed to
return Promise (e.g., require(‘util’).promisify, etc )

With promise based API your function will be much simpler. Also it will be easier to write tests for them.

Also you can use regular .bind to achieve the same (the example from issue):

var methodA = cb => cb();

methodA(cb.bind(null, new Error('foo'));
// cb would be called with one argument: new Error('foo')

methodA(cb.bind(null, null, {id:12}));
// cb would be called with two arguments: null and {id:12}

Probably it can be written even simpler with arrow functions.

I know that there is a likelihood that some people are forced to support old version of Nodejs but it doesn’t mean we should do this.

Also, we should take into consideration that node4 LTS ended on 2018-04-01.

So, I’m sorry to say that but I don’t recommend to merge this. Sorry @nickcarenza, but in the last version I even marked spy.returns to be deprecated because new ES6/7/8 features allows to write readable code without adding additional functions.

[keithamus]

@stalniy I'm a big fan of Promises and async/await but I think your premise that we can write callback free code is flawed. For example the event listener patten is heavily tied to callbacks which is prevalent in Node and DOM.

While I am all formore prescriptiveness in chai - I want to do it for the right reasons: I'd like people to write better test code. Callbacks vs Promises is not something I think we should prescribe. In fact chai itself doesn't prescribe this, given chai-as-promised.

[stalniy]

This is reasonable point but even in case of event emitters we can use simple arrow functions.

[keithamus]

How does that prove the case that we shouldn't provide a calledBackWith assertion?

[stalniy]

This function allows to create a spy which calls last argument immediately (it looks like it was designed specifically for callback based APIs).

Frankly speaking, I don’t see how it can be used with event emitters. Maybe you can show an example?

About arrow functions:

const method = spy()

emitter.on(‘event’, (e) => method(e.data))

// or
emitter.on(‘event’, method)

// Later
emitter.emit(‘event’, { active: true })

[keithamus]

An example would be:

it('doesSomeSideEffect', () => {
  const ee = { on: spy.callsBackWith('something') }
  expect(main(ee))
})

EventListener is perhaps a slightly weak argument because you can pass in an instance of EventEmitter and call emit, but there are plenty of other examples where that isn't the case - Iterators/Generators, Array iterator functions like map, reduce - all of these use a callbacks pattern.

[lucasfcosta]

IMO it's not a problem for us to accept API changes based on callbacks.

As @keithamus cited above, just because we can write Promise based code it doesn't mean that everyone will do that. I'd certainly use Promise/async await most of the time, but I don't think we should do that always, no matter how much I like those two features.

Being opinionated on how people design their code is not our goal, helping them write better tests is.

Also, the whole point of having first class functions is being able to pass that around.

I think that even the very concept of callback is something that can be discussed. When passing a function to map we're basically doing function composition, and that has been around (and praised) for a very long time (lambda calculus ftw) and is a great API.

I also maintain Sinon.js and by considering how the spies/stubs work there I think that this is a valid abstraction. If we're already abstracting the very concept of a spy I think it makes sense for us to allow users to define its behaviour by calling methods provided by them, otherwise we'd end up doing everything natively. If we consider that we should not have this kind of method we might as well not need any spies since we can just pass functions around.

Finally, by looking at the API I see that we already have other methods (not marked for deprecation) that need you to pass functions as arguments, so ultimately a point could be made that the API should be kept consistent.

[stalniy]

Thanks guys for your notes, all this makes sense. I'd like to clarify all details and ask few questions just to make sure that I totally understand the purpose of this helper.

So, lets clarify what this helper do:
it creates a spy which accepts endless amount of arguments and call the last argument (i.e., callback) with parameters which were passed to callsBackWith(...args). so,

const method = spy.callsBackWith(new Error())

method(1,2,3,4,5, error => console.log(error))
// and 
method(error => console.log(error))
// do totally the same thing

method(1,2,3) // throws exception because callback is not provided

In my understanding this helper is useful when you want to stub functions which accept callback and test how callback works based on what passed to that callback, right? If so, why can't we test callback directly without additional wrapper?

Also, I'd like to ask few questions

1. @nickcarenza could you please show/explain how you use this function in your tests?
2. This feature does not work with sandboxed spies:

const sandbox = chai.spy.sandbox()
sandbox.spy.callsBackWith // undefined

Is this expected/intended behavior?

Sorry for probably stupid questions :) but the reason I'm arguing is because I don't understand how can this helper be used, how it helps to make tests better

[nickcarenza]

Sure @stalniy. I have been using this helper function to stub functions that are called by the function I am testing.

// code
function testSubject(a,b,callback) {
  subFuncA(a, function(err, res) {...
    if (err) ...
    subFuncB(b, function(err, res) {...
      if (err) ...
}

// tests
describe('testSubject', function() {
  it('should respond appropriately to subFunc error', function(done){
    subFuncA = chai.spy.callsBackWith(new Error(...))
    testSubject(a,b,function(err, res) {...
  it('should do something based on subFunc response', function(done){
    subFuncB = chai.spy.callsBackWith(null, {items:[...]})
    testSubject(a,b,function(err, res) {...

I hope that helps.

I don't know anything about chai.spy.sandbox.