Sandbox promise reset also clear promiseLibrary

[immanuel192]

• Sinon version : 2.3.6 - 4.5.0
• Environment : any
• Example URL : none
• Other libraries you are using: Bluebird

What did you expect to happen?

• Giving this code

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

const sandbox = sinon.sandbox.create().usingPromise(promise);
const test = {
  test: {
    field1: sandbox.stub(),
    field2: sandbox.stub()
  }
};

test.test.field1.resolves('abc');
const t1 = test.test.field1();

sandbox.reset();
test.test.field1.resolves('abc');
const t2 = test.test.field1();

sandbox.reset();
sandbox.usingPromise(promise);
test.test.field1.resolves('abc');
const t3 = test.test.field1();

sandbox.reset();
test.test.field1.usingPromise(promise);
test.test.field1.resolves('abc');
const t4 = test.test.field1();

• First time resolve : sinon returns instance of Bluebird (const t1)
• After you call sandbox.reset(), sinon will clear everything, especially promiseLibrary. It means that sinon will resolves instance of Native ES6 Promise instead of Bluebird (const t2, t3)
• In order to play around to make it works again, we need to manually call usingPromise (const t4)

What actually happens

• Calling sandbox.reset will also remove promiseLibrary, even in defaultBehaviour

How to reproduce

• Check the code above

[immanuel192]

I suspect the issue because of this.behaviors = []; in the code below

https://github.com/sinonjs/sinon/blob/bda67f6d54c7f2512d507de45ae9ac007d8a60cb/lib/sinon/stub.js#L122-L138

[stale[bot]]

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.

[fearphage]

@immanuel192 You were very close. The issue is actually the line above it this.defaultBehavior = null;. The promiseLibrary information is stored on this property. When it's wiped out, the connection is broken.

https://github.com/sinonjs/sinon/blob/c3a978a893bdf0279f455840af5172558c06633c/lib/sinon/behavior.js#L113-L115

@sinonjs/core Is this a bug? I could see both sides of this argument.

• reset should reset everything
• reset should not touch promise preferences

[mroderick]

With the current behaviour (reset resets everything), it is inconvenient and non-obvious to have to tell the sandbox about the promise implementation after each reset.

If we change the behaviour of reset to not touch promise preferences, we will alter the behaviour of people's tests in new and unexpected ways (invisible).

// a.test.js
var promise = require('bluebird');
var sinon = require('sinon').usingPromise(promise);

describe('myModule', function() {
    afterEach(function(){
        sinon.reset();    
        // doesn't change promise preferences
    })
});

// b.test.js
// this test does not use bluebird, but is loaded after `a.test.js`
var sinon = require('sinon');
// surprise! the default sandbox is using bluebird
// because `b.test.js` was loaded after `a.test.js`
// and reset doesn't remove promise preferences

Here's a solution that could address both concerns

• Replace usePromise on the default sandbox with a method that throws an Error saying that usePromise cannot be used on the default sandbox and that you should create a sandbox with createSandbox.
• Change reset to not wipe promise implementation preferences

This change of behaviour should be in a MAJOR release.

What do you think @immanuel192 @fearphage?

[immanuel192]

@mroderick Here is my current code to solve the problem.

export const newSandbox = () => {
  const sandbox = sinon.sandbox.create()
    .usingPromise(Promise);

  //
  sandbox.reset_ = sandbox.reset.bind(sandbox);
  sandbox.reset = () => {
    sandbox.reset_();
    for (const fake of (sandbox.fakes || [])) {
      fake.usingPromise && fake.usingPromise(Promise);
    }
  };
  return sandbox;
};

[fearphage]

@immanuel192 FYI if you overwrite the global Promise (as it appears you have in the code above), you don't need to do any of this. Sinon uses the global Promise by default. You don't even need to call usingPromise ever at that point.

global.Promise = require('bluebird');

[immanuel192]

@fearphage thanks. From my understanding, it will also override native Promise which may coz concerns and not very good for all cases. What do you think?

[fearphage]

it will also override native Promise which may coz concerns and not very good for all cases.

I have no idea about adverse outcomes from overwriting the global promise. They may exist, but I just don't know about them personally.

[immanuel192]

@mroderick I prefer to change behaviour of reset in sandbox. When reset a sandbox, it should not remove the registered Promise