It should be possible to wrap same method in each sandbox again

[christian-schwaderer]

Is your feature request related to a problem? Please describe.
I have a common test method which is called by many other tests. Inside that common method I need some stubs. The very same methods might or might not already be wrapped by the calling test. I thought the solution would be different sandboxes e.g. something like

function commonTestFunction() {
  const sandboxTwo = sinon.createSandbox();
  const loggerTwoStub = sandboxTwo.stub(logger, 'error');
}

it('should test something', () => {
  const sandboxOne = sinon.createSandbox();
  const loggerOneStub = sandboxOne.stub(logger, 'error');
  commonTestFunction();
})

However, it doesn't work that way. Sinon will always throw that it cannot wrap method error in the logger object because it is already wrapped - despite the different sandboxes.

Describe the solution you'd like
Sinon should allow to wrap same method in each sandbox again. Either by default or in an option when creating the sandbox e.g.
sinon.createSandbox({allowWrappingSameMethodAgain: true});

[fatso83]

This is by design. You just need to cleanup the sandbox. Call restore on it, for instance in a afterEach in your test framework

No need for custom sandbox. Just use the default: sinon.restore()

[christian-schwaderer]

I guess that it's by design. But that design does not really fit my own set-up. Probably I did make my point clear enough. Let's take this example:

function commonTestFunction() {
  const sandboxTwo = sinon.createSandbox();
  const loggerTwoStub = sandboxTwo.stub(logger, 'error');
}

it('test A', () => {
  const someOtherStub = sandboxOne.stub(foo, 'dahoo');
  commonTestFunction();
  myFunction()
  expect(someOtherStub .... 
  // commonTestFunction cannot call sinon.restore() because it would "destroy" `someOtherStub`
})

it('test B', () => {
  const sandboxOne = sinon.createSandbox();
  const loggerOneStub = sandboxOne.stub(logger, 'error');
  commonTestFunction();
  // Continue using loggerOneStub
})

You see, calling sinon.restore anywhere is at least inconvenient for me since I would need to re-stub all my stubs afterwards.

[forty]

I would simply add a sandbox parameter to commonTestFunction and pass your current test sandbox as argument

Sorry I misunderstood

[fatso83]

I have yet to see a good use case for custom sandboxes, outside of library code. Let's rewrite that test to follow conventions:

afterEach(()=> sinon.restore());

// this could also be called from a `beforeEach` to have them all pre-initialised
function createCommonStubs() {
  return {
	logger: sinon.stub(logger, 'error'),
    httpTransport: sinon.stub(http, 'request'),
    // etc
  }
}

it('test A', () => {
  const someOtherStub = sinon.stub(foo, 'dahoo');
  createCommonStubs();
  myFunction()
  expect(someOtherStub .... 
})

it('test B', () => {
  const { logger } createCommonStubs();
  // Continue using the stubbed logger
})

Do we still have an issue?

Mutating the same globals from different places is super confusing in itself and adding use of multiple sandboxes on top is not helping 🧠 Just simplify and it takes care of the whole issue 😃

Am I missing anything?

[christian-schwaderer]

Okay, as always when I try to give simplified examples I'm failing to get my point through.
commonTestFunction is not something like createCommonStubs. It is not returning any stubs. It is for doing actual tests. There are expects in it. It is a kind of internal library to be used from multiple different repos, encapsulating some common test logic.

So, again my requirement is: commonTestFunction need to stub certain functions without
a) "knowing" if they are already stubbed in the calling test
b) having the stubs created in the calling test (if they exist) in scope
c) destroying the the calling test by restoring all stubs.

In my understanding, this is exactly what sandboxes should be for: Different functions should be able to create stubs independent from each other.
If that's not possible, then for my use case it would be helpful if there were a method on a sandbox which would return its stubs by object/method. That way I would not need to re-create existing stubs in commonTestFunction, but could just re-use them without having to get them as params (which wold be super-inconvenient for me). If such a method already exists, I could not find it.

[fatso83]

How exactly should the "without (...) destroying the the calling test by restoring all stubs" work? If the calling test has the tests in its sandbox and the commonTestFunction puts them in another sandbox, what are you trying to stub? When the commonTestFunction sandbox restores, is it then restoring the previous stubs, so that the other sandbox will be responsible for restoring the original functions. This makes sense if you are sure you can always restore in order. What will happen if you do not restore in order? You are not sure that you will be recreating the original object. If you are to use some kind of detection of an existing stub, to avoid creating a new, then you will quickly share state (stubs) between different sandboxes. That is a wormhole of additional bugs.

There are many complicating cases here, which is why I would advice against multi-sandbox code or adding code that implies that we intend to support those cases. Tests should be simple to reason about, which is why a mutating commonTestFunction shared by many tests seems a bit off.

which would return its stubs by object/method

The building blocks for this is in place right now. sandbox#getFakes() is not meant to be a public method, as it accesses the underlying collection of fakes created by the sandbox, but you can use it and we could document it as a public method. Once you have that, you can create a little utility to partition the fakes per object.

function fakesPerObject(sandbox) {
  const fakes = sandbox.getFakes();
  const fakesPerObject = {};
  fakes.forEach((f) => {
    if (!fakesPerObject[f.rootObj]) fakesPerObject[f.rootObj] = [];
    fakesPerObject[f.rootObj].push(f);
  });
  return fakesPerObject;
}

So ... I do not think we want to add explicit support for this, but it seems building bits are in place for you to achieve it external to Sinon.