stub#usingPromise and stub#onNthCall have unexpected interactions

[kara-ryli]

Using the latest version of just about everything:

    "node": 6.11.0",
    "bluebird": "3.5.0",
    "chai": "4.0.2",
    "chai-as-promised": "7.0.0",
    "mocha": "3.4.2",
    "sinon": "2.3.5",
    "sinon-chai": "2.11.0"

I run into a couple interactions between stub#usingPromise and stub#onNthCall.

My test is testing an internal method call that returns a Bluebird promise. I want to make sure that subsequent calls after a failure work correctly.

Code:

const Bluebird = require('bluebird');
const chai = require('chai');
const chaiAsPromised = require('chai-as-promised');
const sinon = require('sinon');
const sinonChai = require('sinon-chai');

const expect = chai.expect;

chai.use(sinonChai);
chai.use(chaiAsPromised);

describe('repro', () => {
  it('should work', () => {
    var s = sinon.stub();
    function test() {
      return s().tap(() => true);
    }
    s.
      onFirstCall().usingPromise(Bluebird).rejects(new Error('oops')).
      onSecondCall().usingPromise(Bluebird).resolves('yay');
    return expect(test()).to.be.rejected.then(() => {
      return expect(test()).to.be.eventually.eql('yay');
    });
  });

  it('should maybe work', () => {
    var s = sinon.stub();
    function test() {
      return s().tap(() => true);
    }
    s.
      onFirstCall().rejects(new Error('oops')).usingPromise(Bluebird).
      onSecondCall().resolves('yay').usingPromise(Bluebird);
    return expect(test()).to.be.rejected.then(() => {
      return expect(test()).to.be.eventually.eql('yay');
    });
  });

  it('should also work', function () {
    var s = sinon.stub();
    function test() {
      return s().tap(() => true);
    }
    s.usingPromise(Bluebird).
      onFirstCall().rejects(new Error('oops')).
      onSecondCall().resolves('yay');
    return expect(test()).to.be.rejected.then(() => {
      return expect(test()).to.be.eventually.eql('yay');
    });
  });
});

None of these tests pass, failing on TypeError: s(...).tap is not a function. However, I can get them to pass by changing my implementation:

describe('regressions', () => {
  it("works", () => {
    var s = sinon.stub();
    function test() {
      return Bluebird.try(() => s()).tap(() => true);
    }
    s.
      onFirstCall().rejects(new Error('oops')).
      onSecondCall().resolves('yay');
    return expect(test()).to.be.rejected.then(() => {
      return expect(test()).to.be.eventually.eql('yay');
    });
  });

  it('also works', () => {
    var s1 = sinon.stub();
    var s2 = sinon.stub();
    function test1() {
      return s1().tap(() => true);
    }
    function test2() {
      return s2().tap(() => true);
    }
    s1.usingPromise(Bluebird).rejects(new Error('oops'));
    s2.usingPromise(Bluebird).resolves('yay');
    return expect(test1()).to.be.rejected.then(() => {
      return expect(test2()).to.be.eventually.eql('yay');
    });
  });
});

But I'd prefer not to change the implementation to fit the tests.

[mroderick]

I can't quite wrap my head around this just yet, so I can tell if this is a bug or not.

In the meantime, there's an easier way to use third party promise libraries with Sinon

const Bluebird = require('bluebird');
const sandbox = require('sinon').sandbox.create().usingPromise(Bluebird);
// all stubs created with this sandbox will use Bluebird for promises

Perhaps you can try using that to see if that exposes a flaw in your implementation or makes a bug in Sinon easier to identify?

[fulax]

Hi there. I'm hitting a similar issue, with a simpler test case:

const bluebird = require('bluebird');
const sinon = require('sinon');

const myStub = sinon.stub().usingPromise(bluebird.Promise);

// works
myStub.resolves(['default array']);
myStub().tap(console.log);

// does not work: TypeError: myStub(...).tap is not a function
myStub.onSecondCall().resolves(['second array']);
myStub().tap(console.log);

I tested it using both sinon 2.3.5 and 2.3.6, same behavior. I'm using bluebird 3.5.0, with nodejs v6.11.0 on Debian Jessie.

Using sinon's sandbox gave me the same unexpected result.

[fatso83]

@fulax and @ry7n , do you know if this worked correctly at some earlier point? I'd like to find out if we have a regression, or if this bug was there all along (if a bug - which it looks like).

[kara-ryli]

@fatso83: I noticed this issue upgrading from [email protected] + sinon-as-promised to [email protected], so I never saw such a test work in 2.x. I think a simple example case of the correct ordering is fine, i.e. can you re-write this:

  it('should work', () => {
    var s = sinon.stub();
    function test() {
      return s().tap(() => true);
    }
    s.usingPromise(Bluebird).
      onFirstCall().rejects(new Error('oops')).
      onSecondCall().resolves('yay');
    return expect(test()).to.be.rejected.then(() => {
      return expect(test()).to.be.eventually.eql('yay');
    });

To pass without modifying the test() function? But from a bunch of iterating, I couldn't find a combo of #usingPromise and #onNthCall that worked correctly.

[HugoMuller]

I guess that the internal stub's attribute promiseLibrary, (set with usingPromise) is not propagated to the behaviors generated with onCall(n). More precisely, this.promiseLibrary is undefined.

I tried to run the @fulax's code in debug mode, and set a break point in the body of the invoke method, just here: return (this.promiseLibrary || Promise).resolve(this.returnValue);

You will see that this.promiseLibrary is correctly set for the original stub (this.promiseLibrary === bluebird constructor). But it is undefined for the behavior defined with onSecondCall().

Edit: Adding this couple of lines in proto.create before returning the brand new behavior seems to solve the issue.

if(stub.defaultBehavior && stub.defaultBehavior.promiseLibrary){
  behavior.promiseLibrary = stub.defaultBehavior.promiseLibrary;
}

[fatso83]

@HugoMuller good find! any chance of wrapping it into a PR?

[HugoMuller]

Yeah, I'm working on it ;)

[mroderick]

Fixed by #1484

[brian-mann]

The exact same problem still exists when using withArgs instead of onCall.

[mroderick]

The exact same problem still exists when using withArgs instead of onCall.

@HugoMuller since you already understand this part well, would you mind investigating that?

[HugoMuller]

Ok, I'll take a look at this :)

[HugoMuller]

done => #1497

[fatso83]

I ❤️ the speed at which things are being fixed and releases are getting out these days. Even though it was a long birth, we must have been doing something right with the work leading up to 2.0 😄