sinon.restore() && sandbox.restore() not restoring fakes

[Crowbrammer]

Describe the bug
Can't clear fakes sinon.restore() (nor resetHistory, resetBehavior, nor reset). I then tried creating a new sandbox, and clearing the sandbox. No dice. Is this intentional?

To Reproduce

it("Demonstrates how to restore a fake", () => {
  let sandbox = sinon.createSandbox();
  let thing = sandbox.fake();
  thing("Hi");
  expect(thing.calledOnce).to.be.true;
  expect(thing.lastArg).to.equal("Hi");
  sandbox.restore();
  sandbox.resetHistory();
  sandbox.resetBehavior();
  sandbox.reset();
  expect(thing.calledOnce).to.be.false; // Fails. thing.calledOnce === true
  expect(thing.lastArg).to.be.undefined; // Fails if above commented out. thing.lastArg === "Hi"
});

Expected behavior
I expected thing.lastArg to be either undefined or null. It gives me "Hi"

Screenshots
N/a

Context (please complete the following information):

• Library version: Sinon ^7.3.2
• Environment: Node 10.16.0
• Example URL: N/a
• Other libraries you are using: Mocha, Chai

Additional context
Just testing the capabilities of Sinon as I use it to test a backend.

[fatso83]

As it is, it looks like a bug, but to be honest, I am not totally sure about this. I remember a key point on fakes is that they were supposed to be immutable in some sense, but since they do have state (i.e. they know if they have been called), that immutability must be restricted to only the stubbing behavior, I assume.

@mroderick might be the right person to answer this ...

If this is the right behavior, we should at least update the docs.

[Crowbrammer]

@fatso83 Much appreciated! The .restore doc mentioned fakes:

from here.

[fatso83]

Was just bit by this bug myself, so this would be nice to get fixed.

[mroderick]

If no one else makes a PR for it in the next few days, I can see if I can find the time to fix it

[fatso83]

I looked into this now and the docs are a bit misleading, as is the test above. It actually seems to do (almost) what it says, but let's just define some terms first:

restore - restoring a sandbox means restoring all stubbed functions to their original state, meaning the stubs are removed from each object were they overwrote the original.
reset - resetting means that the state of each of the created fakes is cleared, but they are still present on the objects.

The problem with the above test is that it

1. Removes all the fakes from the sandbox
2. Then proceeds to go through the collection of fakes in the sandbox (now empty) and tries to reset the (empty) list of fakes

That doesn't make sense :-)

I have tested and both the reset and restore calls work as they should. Almost ... reset does clean the state of calledOnce, but lastArgument seems not to be cleared. Checking a bit more to see if I have lost some detail.

[fatso83]

Closing this as not reproducible, but opening a new issue on the fake.lastArg bug.

To verify: https://runkit.com/fatso83/sinon-issue-2065

[oscarr-reyes]

I can confirm that this bug is also happening to me, I have a test scenario that requires the implementation to be intact, but the function carries the stub result from a previous test, which means that sandbox.restore() is not doing what's intended to do.

This is happening to me on version 7.5.0

[mantoni]

@oscarr-reyes Can you check if 54df7f7 fixes it for you? I think it didn't make it into a new release yet.

[fatso83]

@oscarr-reyes Are you able to make a reproducible example? We have not yet actually seen this bug. If you look at the thread above, and my explanation, you'll see the only bug we found was a single flag hadn't been cleared. Everything else works. So unless we actually see some proof, there is no bug (apart from the lastArg flag).

[orekav]

I have facing the same issue.

I am sandboxing my "validateCaptcha" middleware and when I restore it continues issuing the stub.

/src/middlewares/captcha.js

const request = require("got");

const validateCaptcha = async (req, res, next) => {
	// if (process.env.NODE_ENV !== "production") return next();

	const captcha = req.body["g-recaptcha-response"] || req.body.g_recaptcha_response;

	// g-recaptcha-response is the key that browser will generate upon form submit.
	// if its blank or null means user has not selected the captcha, so return the error.
	if (captcha === undefined || captcha === "" || captcha === null)
		return res
			.status(400)
			.json({ message: "Please, verify that you are not a robot" });

	// Put your secret key here.
	const secretKey = process.env.GOOGLE_CAPTCHA_API_SECRET_KEY;

	// req.connection.remoteAddress will provide IP address of connected user.
	const verificationUrl = `https://www.google.com/recaptcha/api/siteverify?secret=${secretKey}&response=${captcha}`;

	try {

		// Hitting GET request to the URL, Google will respond with success or error scenario.
		const { body } = await request.post(verificationUrl, { responseType: "json" });

		// Success will be true or false depending upon captcha validation.
		if (body.success !== undefined && !body.success)
			return res
				.status(400)
				.json({ message: "Captcha validation error" });
	} catch (error) {
		return res
			.status(503)
			.json({ message: "Couldn't validate captcha" });
	}

	next();
};

module.exports = {
	validateCaptcha,
};

/test/api/user.spec.js

require("../setup.spec");
const supertest = require("supertest");
const captcha = require("../../src/middlewares/captcha");
const sinon = require("sinon");
let app;
let agent;
let sandbox;

const login = (anAgent, userCredentials) =>
	anAgent
		.post("/api/users/signIn")
		.send(userCredentials);

const logout = (anAgent) =>
	anAgent
		.post("/api/users/signOut");

describe("User endpoints", function () {
	this.beforeAll(async () => {
		sandbox = sinon.createSandbox();
		sandbox
			.stub(captcha, "validateCaptcha")
			.callsFake(async (req, res, next) => {
				console.log("Validate Captcha FAKE");
				return next();
			});
		// sandbox.restore();
	});

	this.beforeEach(async () => {
		app = require("../../src/app");
		agent = supertest.agent(app);
		const sequelize = require("../../src/models");
		await sequelize.sync({ force: true });

		const Customer = require("../../src/models/customer");
		const User = require("../../src/models/user");
		await Customer.create(
			{
				firstName: "Test",
				lastName: "Customer",
				user: {
					email: "foo@bar.test",
					password: "boo",
				},
			},
			{
				include: [User],
			}
		);
	});

	this.afterEach(async () => {
		sandbox.restore();
	});

	it("should create a new user", function (done) {
		agent
			.post("/api/users/signUp")
			.send({
				firstName: "firstName",
				lastName: "lastName",
				email: "email@domain.com",
				password: "aPassword",
			})
			.expect(201)
			.end((err, res) => {
				err ? done(err) : done();
			});
	});

	it("should sign in", function (done) {
		login(agent, { email: "foo@bar.test", password: "boo" })
			.expect(200)
			.end((err, res) => {
				const { message, user } = res.body;
				message.should.be.equal("valid");
				user.should.have.property("email").equal("foo@bar.test");
				user.should.have.property("customer").have.property("firstName").equal("Test");
				user.should.have.property("customer").have.property("lastName").equal("Customer");

				agent
					.get("/api/users/me")
					.expect(200)
					.end((err, res) => {
						const user = res.body;
						user.should.have.property("email").equal("foo@bar.test");
						user.should.have.property("customer").have.property("firstName").equal("Test");
						user.should.have.property("customer").have.property("lastName").equal("Customer");
						err ? done(err) : done();
					});
			});
	});

	it("should sign out", async function () {
		await login(agent, { email: "foo@bar.test", password: "boo" });
		await logout(agent)
			.expect(302)
			.expect("Location", "/");

		return agent
			.get("/api/users/me")
			.expect(401);
	});

	it("should return unauthorized", function (done) {
		agent
			.get("/api/users/me")
			.expect(401)
			.end((err, res) => {
				err ? done(err) : done();
			});
	});
});

What I'm doing to check that it doesn't work is:

1. Mock it for the first test
2. Restore it for the following ones

Inside beforeAll there is a comment "sandbox.restore();" that I have tried and there it worked... but it is useless

I am also doing require each time to my app.js

[orekav]

Five minutes ago I have modified it to:

require("../setup.spec");
const decache = require("decache");
const supertest = require("supertest");
const sequelize = require("../../src/models");

const sinon = require("sinon");
const sandbox = sinon.createSandbox();
let agent;

const login = (anAgent, userCredentials) =>
	anAgent
		.post("/api/users/signIn")
		.send(userCredentials);

const logout = (anAgent) =>
	anAgent
		.post("/api/users/signOut");

describe("User endpoints", function () {
	this.beforeAll(async () => {

	});

	this.beforeEach(async () => {
		decache("../../src/app");
		decache("../../src/middlewares/captcha");

		const captcha = require("../../src/middlewares/captcha");

		sandbox
			.stub(captcha, "validateCaptcha")
			.callsFake(async (req, res, next) => {
				console.log("Validate Captcha FAKE");
				return next();
			});

		const app = require("../../src/app");
		agent = supertest.agent(app);
		await sequelize.sync({ force: true });


		const Customer = require("../../src/models/customer");
		const User = require("../../src/models/user");
		await Customer.create(
			{
				firstName: "Test",
				lastName: "Customer",
				user: {
					email: "foo@bar.test",
					password: "boo",
				},
			},
			{
				include: [User],
			}
		);
	});

	this.afterEach(async () => {
		sandbox.restore();
	});

	it("should create a new user", function (done) {
		agent
			.post("/api/users/signUp")
			.send({
				firstName: "firstName",
				lastName: "lastName",
				email: "email@domain.com",
				password: "aPassword",
			})
			.expect(201)
			.end((err, res) => {
				err ? done(err) : done();
			});
	});

	it("should sign in", function (done) {
		login(agent, { email: "foo@bar.test", password: "boo" })
			.expect(200)
			.end((err, res) => {
				const { message, user } = res.body;
				message.should.be.equal("valid");
				user.should.have.property("email").equal("foo@bar.test");
				user.should.have.property("customer").have.property("firstName").equal("Test");
				user.should.have.property("customer").have.property("lastName").equal("Customer");

				agent
					.get("/api/users/me")
					.expect(200)
					.end((err, res) => {
						const user = res.body;
						user.should.have.property("email").equal("foo@bar.test");
						user.should.have.property("customer").have.property("firstName").equal("Test");
						user.should.have.property("customer").have.property("lastName").equal("Customer");
						err ? done(err) : done();
					});
			});
	});

	it("should sign out", async function () {
		await login(agent, { email: "foo@bar.test", password: "boo" });
		await logout(agent)
			.expect(302)
			.expect("Location", "/");

		return agent
			.get("/api/users/me")
			.expect(401);
	});

	it("should return unauthorized", function (done) {
		agent
			.get("/api/users/me")
			.expect(401)
			.end((err, res) => {
				err ? done(err) : done();
			});
	});
});

[scottpatrickwright]

I am seeing this issue as well. Why is this closed?

[scottpatrickwright]

It may be that the behaviour of restore is sensitive to the specific syntax used to set up the stub. Afaik both of these should be valid ways of doing this but the restore() is different. In one it works and in the other it has unexpected results.

const sinon = require('sinon')

// restore does not work as expected

let db = {
  query: () => {
    return 'my query 1'
  }
}
const dbStub = sinon.stub().resolves('somejson')
db.query = dbStub
db.query() // somejson
sinon.restore()
db.query() // resolves nothing

// restore works as expected

db = {
  query: () => {
    return 'my query 1'
  }
}
sinon.stub(db, 'query').resolves('somejson')
db.query() // somejson
sinon.restore()
db.query() //my query 1

[mroderick]

@scottpatrickwright when you do assignments directly to db.query, then sinon has no way of knowing what needs to be restored or what the value was before the assignment.

[fatso83]

@scottpatrickwright It's closed because there is no bug, as should be apparent from the thread. It was just user error, as in your case: it works as described and intended :-)

The Sinon sandbox can clean up things it knows about. That means Sinon has to have knowledge of the objects with which it interacts. The stub you describe above is made standalone. Sinon is not made aware of any of the objects you assign it to. That changes in the second example. You could also do sinon.replace(obj, fieldname, fake) to achieve the same. It's just JavaScript 🙂

[scottpatrickwright]

Fair enough - thanks for the explanation.

…

On Fri, Feb 19, 2021 at 7:02 AM Carl-Erik Kopseng ***@***.***> wrote: @scottpatrickwright <https://github.com/scottpatrickwright> It's closed because there is no bug, as should be apparent from the thread. It was just user error, as in your case: it works as described and intended :-) The Sinon sandbox can clean up things it knows about. That means Sinon has to have knowledge of the objects with which it interacts. The stub you describe above is made standalone. Sinon is not made aware of any of the objects you assign it to. That changes in the second example. You could also do sinon.replace(obj, fieldname, fake) to achieve the same. It's just JavaScript 🙂 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2065 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKKRTFVMGEYVIB5GSIDXKTS7ZHMRANCNFSM4IGBASQQ> .

[neondizen]

ok I have just run into this with v10.0.0. Earlier in the ticket there was a call for an example of sinon.restore() failing to restore fakes. Here's one:

Test code:

const AWS = require( 'aws-sdk' );
const sinon = require( 'sinon' );
const { expect } = require( 'chai' );

describe( '67322634', () => {
    afterEach( () => {
        sinon.restore();
    } );
    it( 'should get secret value', async () => {
        const data = {
            SecretString: JSON.stringify( { publicKey: 'secretUsername', privateKey: 'secretPassword' } ),
        };
        const secretsManagerStub = {
            getSecretValue: sinon.stub().callsFake( ( params, callback ) => {
                callback( null, data );
            } ),
        };
        const SecretsManagerStub = sinon.stub( AWS, 'SecretsManager' ).returns( secretsManagerStub );
        const main = require( './main' );
        const { username, password } = await main( '1' );
        expect( username ).to.equal( 'secretUsername' );
        expect( password ).to.equal( 'secretPassword' );
        sinon.assert.calledOnce( SecretsManagerStub );
        sinon.assert.calledOnceWithExactly(
            secretsManagerStub.getSecretValue,
            {
                SecretId: '1',
            },
            sinon.match.func,
        );
    } );

    it( 'should not get secret value if there is an error with SecretsManager', async () => {
        const secretsManagerStub = {
            getSecretValue: sinon.stub().callsFake( ( params, callback ) => {
                const err = new Error( 'There was an error' );
                callback( err );
            } ),
        };
        const SecretsManagerStub = sinon.stub( AWS, 'SecretsManager' ).returns( secretsManagerStub );
        const main = require( './main' );
        const { username, password } = await main( '1' );
        expect( username ).to.not.equal( 'secretUsername' );
        expect( password ).to.not.equal( 'secretPassword' );
        sinon.assert.calledOnce( SecretsManagerStub );
        sinon.assert.calledOnceWithExactly(
            secretsManagerStub.getSecretValue,
            {
                SecretId: '1',
            },
            sinon.match.func,
        );
    } );
} );

Driver code:

const AWS = require( 'aws-sdk' );
const secretsManager = new AWS.SecretsManager();

module.exports = async ( keyId ) => {
    return getSecret( keyId )
        .then( ( secret ) => {
            const username = secret.publicKey;
            const password = secret.privateKey;
            return { username, password };
        } )
        .catch( ( err ) => {
            console.error( err );
        } );
};

const getSecret = ( keyId ) => {
    return new Promise( ( resolve, reject ) => {
        secretsManager.getSecretValue(
            {
                SecretId: keyId,
            },
            ( err, data ) => {
                if ( err ) {
                    reject( err );
                } else {
                    resolve( JSON.parse( data.SecretString ) );
                }
            },
        );
    } );
};

Expected: sinon.restore() works

Actual: sinon.restore() does not reset correctly - the second unit test fails because it is using the same stub set up in the first unit test. If you reverse the test order, they fail the opposite way around. If you run the tests individually, they pass.

The example code is consistent with the guidance given in https://sinonjs.org/releases/latest/stubs/ and it does not work? Does sinon not know how to reset nested stubs?

[fatso83]

@neondizen Sinon does restore correctly, but it is not immune to usage errors 😃 As detailed in my response in your other issue #2376 (comment) the problem you experience is caused by

1. stubbing AWS.SecretsManager
2. importing main and thus creating a new instance of the stub
3. restoring the AWS.SecretsManager stub, but as this will not affect the instance that is already created and reused, you also reuse the object created by the previous stub

To make your tests "immutable" you would need to change the client code to create new secret managers for each invocation or make your tests reset the module somehow. I linked some ways of doing this in my reply.

[fatso83]

People that have read the thread and understood why it was closed and locked (because we have not seen a bug after inspecting several reports) and still mean they have an actual reproducible bug with code to back it up can visit #2473 (which is yet to be verified).