Sinon: resetHistory() is not working if restore() is called right before it

[Alexsey]

• Sinon version : 4.4.8
• Environment : NodeJS 8.10.0
• Other libraries you are using: -

What did you expect to happen?
call to sandbox.restore() should not affect the consequent call to sandbox.resetHistory()

What actually happens
call of sandbox.restore() breaks the consequent call of sandbox.resetHistory()

How to reproduce

const sinon = require('sinon')
const sandbox = sinon.createSandbox()

const stub = sandbox.stub()
stub()
sandbox.restore()
sandbox.resetHistory()
console.log(!!stub.lastCall) // true

but if we remove sandbox.restore() the sandbox.resetHistory() would do its job and everythings goes as expected

const sinon = require('sinon')
const sandbox = sinon.createSandbox()

const stub = sandbox.stub()
stub()
// sandbox.restore()
sandbox.resetHistory()
console.log(!!stub.lastCall) // false

[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.

[Alexsey]

Still there

[fearphage]

What's the use case here? Calling restore() means return everything to it's normal un-mocked/un-stubbed state. I don't understand how reseting history after that point is valuable. You're resetting history on stubs that are no longer being used/accessible. This seems like undefined behavior.

Example:

const stub = sinon.stub(console, 'log');

console.log('catpants');

console.log.restore();
console.log.resetHistory(); // throws an error since this isn't a stub anymore

I'm going to close this ticket for now, but feel free to reopen it with a detail use case of _why_ you're doing this and what your expectations are for the outcome.

[Alexsey]

We are using stubbing/mocking for the unit under test and proxyquire with stubs to fake dependencies. So it requires to un-stub/un-mock + reset stubs after each test in after (teardown). I can provide a simple example if it is not explicit enough. And it appears very confusing when the order of call of this function becomes significant

I want to notes that call restore and call to resetHistory is expected to affect different sets of fakes. That is why it is done on sandbox

And the expected behavior here is that each method should bring the state of the specific aspect of all the fakes created with this sandbox to the initial state, no matter what was their general state before the call

BTW I don't have permissions to reopen the ticket

[fearphage]

I can provide a simple example if it is not explicit enough.

Yes, this would be very helpful. I understand your setup and I've done similar things many times. I generally call resetHistory in the beforeEach though (instead of the teardown). I still can't see the benefit of resetting the history of nonexistent and/or inaccessible stubs, but I've reopened this to continue the conversation.

BTW I don't have permissions to reopen the ticket

@mroderick Can you look into this?

[Alexsey]

Yes, of course, it should be afterEach and not after

I have thought of some example that would be simpler than the one I described initially, but it doesn't mean that initial is not relevant or there are no others

Idea is that we are trying to test that add method that would add numbers that it gets from it's getNumbers. getNumbers is async so add returns it results asynchronously in the callback (just because :) ):

'use strict'

require('chai').should()
const sinon = require('sinon')

const calc = require('./calc')

const cb = sinon.stub()

describe('calc', () => {
  afterEach(() => {
    sinon.resetHistory()
    sinon.restore()
  })

  it('should add 2 and 2 and return 4', async () => {
    sinon.stub(calc, 'getNumbers').returns([2, 2])
    calc.add(cb)
    cb.args.should.eql([[4]])
  });

  it('should add three numbers', async () => {
    sinon.stub(calc, 'getNumbers').returns([1, 2, 3])
    calc.add(cb)
    cb.args.should.eql([[6]])
  });
});

These two tests depend on the order of resetHistory and restore call: if you switch them the tests will fall

[fearphage]

These tests seem overly complex. I would write these tests differently. Sharing stubs across tests is generally a bad idea (like your global cb).

describe('calc', () => {
  // I would normally store these on the test context (this.meh)
  // but you can't do that with arrow functions... which is why
  // I rarely use arrow functions with mocha
  let cb;
  let getNumbers;

  beforeEach(() => {
    // new stubs for every single test (so need to resetHistory)
    cb = sinon.stub(); 
    getNumbers = sinon.stub(calc, 'getNumbers');
  });

  afterEach(() => {
    getNumbers.restore();
  });

  describe('with 2 params', () => {
    beforeEach(() => {
      getNumbers.returns([2, 2]);
    });

    it('should return the correct result', aysnc () => {
      calc.add(cb);

      cb.args.should.eql([[4]]);
    });
  });

  describe('with 3 params', () => {
    beforeEach(() => {
      getNumbers.returns([1, 2, 3]);
    });

    it('should return the correct result', aysnc () => {
      calc.add(cb);

      cb.args.should.eql([[6]]);
    });
  });
});

I personally try to make tests as concise and focused as possible. By that, I mean I put setup code in setup (beforeEach) functions. This makes it easier for the reader to spot the differences in tests in my opinion. It's a bit more verbose, but it keeps the test code tight and meaningful.

If I wanted to force myself to use resetHistory and restore, I could have changed the test setup:

// This shared context across test runs is suboptimal in practice but should still work
before(() => {
  cb = sinon.stub();
  getNumbers = sinon.stub(calc, 'getNumbers');
});

beforeEach(() => {
  sinon.resetHistory(); 
});

after(() => {
  sinon.restore();
});

These two tests depend on the order of resetHistory and restore call: if you switch them the test fail

You have not shown the need/benefit to calling resetHistoy on nonexistent mocks/stubs. Perhaps you can share another use case? I still can't wrap my head around it. After you call restore, the goal is for the environment to be like sinon never existed at all. Everything should be back to it's original state. Calling reset history at that point serves no purpose. The scenario you're explaining seems like the following test file:

```js
// catpants-test.js
const catpants = require('..');
const sinon = require('sinon');

describe('catpants', () => {
afterEach(() => {
// calling resetHistory on a pristine environment
// with no mocks, fakes, stubs, or spies
sinon.resetHistory();
});

it('does not use sinon at all', () => {
assert.equal(catpants(1), 2);
});

it('also does not use sinon', () => {
assert.equal(catpants(3), 4);
});
});
````

Notice how I never mock/stub anything, but I still call resetHistory anyway? That's what calling it after restore seems like to me. You're resetting the history of nothing/things that don't exist.

[Alexsey]

I have written a big post about why shared stubs are not bad and why tests are not overly complex, but then delete it, because all this is not about if provided syntactic example is good or bad, but about if current behavior is a bug or not

And what is important in it is the usage of Sinon, and specifically the usage of resetHistory and restore: both calls are required - you can't remove any of them, and it's explicit and goes from their definitions. But their order is required as well, and it's implicit, error-prone and doesn't go inline with the documentation

I really don't understand why you are saying that I'm trying to reset history of something non-existing. Because resetting history of something non-existing means do nothing => action that can just be removed. But if you remove resetHistory from the example the tests will fall

You are saying "After you call restore, the goal is for the environment to be like sinon never existed at all", but it is not true - standalone fakes will remain with their behavior and their history. The only difference is that resetHistory for the sandbox they were created by would no longer work

Lets read the documentation of restore. It says Restores all fakes created through sandbox. That means restore must restore faked methods and properties on the objects. It should not mess with standalone fakes created with the sandbox. And it's not doing so. But it changes the state of the sandbox itself - destroy its link to standalone fakes created by this sandbox. So the resetHistory is no longer works. It should not do that! By the definition, it should not change the state of the sandbox! There is no reason to do that, and it is going against the documentation

[fearphage]

I really don't understand why you are saying that I'm trying to reset history of something non-existing.

That is how sinon is designed/coded. collection in the linked code holds the list of fakes/stubs. When you call restore on a sandbox, sinon has no knowledge of any of the previous stubs. It has no reference to them anymore. If you don't have a reference to them in your code, the fakes/stubs will be cleaned up next time garbage collection runs. As they code is written, they no longer exist to sinon. They are gone and forgotten. That's why resetting the history of 0 fakes (the number remaining after you call restore) doesn't work as expected.

I have written a big post about why shared stubs are not bad and why tests are not overly complex

I wouldn't mind hearing about this. I'm always open to learning something new and seeing the world from another viewpoint.

[Alexsey]

I think in this case a sandbox should not forget about created stubs. Because

1. Created stubs are out of the scope of responsibility of restore that goal is to restore faked properties of real objects
2. The current implementation of restore is doing two things, while good function should never do more than one
3. There is no way to just restore faked properties without losing connection to standalone stubs
4. Forgetinnges about created stubs has no practical value

About releasing the references - this side effect is also not represented by guides or documentation. I'm pretty sure that in tests that are not faking properties you will find no restore at all. I think it could be optimized by the usage of WeakSet instead of Array for ES6+ environments with feature detection with Array for ancient envs

About shared stubs. Let's go from the side of the theory: we have a Shared Fixture pattern and Immutable Shared Fixture pattern. Shared stubs are looks like Shared Fixture as long as one test may somehow affect another. But as long as we have resetBehavior in afterEach (and resetBehavior if needed) they become Immutable Shared Fixture. And Immutable Shared Fixture is a good way to reduce the setup code with no downsides

Another example where it really matters is the usage of proxyquire where you don't want to reload module under test for each test because that would increase the time of tests execution in tens of times

[fearphage]

Created stubs are out of the scope of responsibility of restore that goal is to restore faked properties of real objects

One of the goals of using a sandbox is to simplify cleanup. So when you call restore, the sandbox cleans up everything it has knowledge of.

The current implementation of restore is doing two things, while good function should never do more than one

Feel free to open a pull request.

There is no way to just restore faked properties without losing connection to standalone stubs

I'm not sure I fully understand this statement. Please explain further. Sandboxes only restore things they created. If you create a stub via sinon.stub(...), the sandbox will not restore that standalone stub.

Forgetinnges about created stubs has no practical value

What is the value of a reference to a stub that is not and will not be in use?

About releasing the references - this side effect is also not represented by guides or documentation.

None of the documentation says when references are released. Those are implementation details. What sinon does internally is not documented in general. The documentation is for the public/external API.

I think it could be optimized by the usage of WeakSet instead of Array for ES6+ environments with feature detection with Array for ancient envs

We are always open to pull requests.

About shared stubs. Let's go from the side of the theory

I was hoping for something a bit more concrete. If you're able to share some actual example of this in practice where you feel you need to resetHistory after you restore, then I'd like to see it.

Shared stubs are looks like Shared Fixture as long as one test may somehow affect another.

This is suboptimal to say the least. Tests that rely on and affect other tests or rely on a certain test order are brittle and not good tests in my opinion. I find it best for tests to be self-contained units (aside from setup and teardown). I've seen this pattern of ordered and interconnected tests lead to trouble on numerous occasions. It also breaks when you're trying to just run one test at a time (it.only).

Another example where it really matters is the usage of proxyquire where you don't want to reload module under test for each test because that would increase the time of tests execution in tens of times

I have used the following pattern: Proxyquire in the before (run once) with stub creation and then the beforeEach resets the stub before each test run. I've never had a problem with this. Have you considered a similar pattern?

Could you provide some concrete examples of your usage? I'm wondering if there's another way to get the same result you desire. Actual code would make this a lot easier to diagnose and discuss.

[komali2]

server.restore() doesn't seem to clear the server.requests array for us. Length will still be equal to whatever it was before the call to server.restore().

[fearphage]

@komali2 This sounds like a separate ticket. Please file a new bug report.

[fatso83]

Closing as it works as intended and I see no activity.