Sinon: Restoring stub fails when it executed .value()

[XxshiftxX]

Describe the bug
I made a stub with mongoose.connection's readyState Property.
And I executed sinon.restore() when test finished (with afterEach)

It throws TypeError when my stub restored.

To Reproduce

1. Required 2 modules: mongoose, sinon
2. Make stub with this code.

sinon.stub(mongoose.connection, 'readyState').value(1);

1. Restore stub with sinon.restore().
2. It throws error.

D:\dev\personal\sinon-test\node_modules\sinon\lib\sinon\stub.js:103
                Object.defineProperty(object, property, actualDescriptor);
                       ^

TypeError: Cannot redefine property: readyState
    at Function.defineProperty (<anonymous>)
    at Function.restore (D:\dev\personal\sinon-test\node_modules\sinon\lib\sinon\stub.js:103:24)
    at D:\dev\personal\sinon-test\node_modules\sinon\lib\sinon\sandbox.js:30:21
    at Array.filter (<anonymous>)
    at applyOnEach (D:\dev\personal\sinon-test\node_modules\sinon\lib\sinon\sandbox.js:29:5)
    at Sandbox.restore (D:\dev\personal\sinon-test\node_modules\sinon\lib\sinon\sandbox.js:164:9)
    at Object.<anonymous> (D:\dev\personal\sinon-test\test.js:11:7)
    at Module._compile (internal/modules/cjs/loader.js:959:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:995:10)
    at Module.load (internal/modules/cjs/loader.js:815:32)
    at Function.Module._load (internal/modules/cjs/loader.js:727:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1047:10)
    at internal/main/run_main_module.js:17:11

Expected behavior
I tested this case with this code:

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

console.log(mongoose.connection.readyState);

sinon.stub(mongoose.connection, "readyState").value(1);

console.log(mongoose.connection.readyState);

sinon.restore();

console.log(mongoose.connection.readyState);

and It should prints 0, 1, 0.
but It throws error above before second 0 printed

Screenshots

Context (please complete the following information):

• Library version: ^9.0.0
• Environment: Windows 10 & node.js v12.13.1
• Other libraries you are using: mongoose

[mroderick]

Thank you for reporting this issue.

Here's what I've learned so far in my investigation of this

• The property you're replacing is an accessor, which means you should really be using .get(), instead of .value()
• The accessors are defined on the Connection.prototype using Object.defineProperty
• Object.defineProperty has configurable:false as default

get vs. defineProperty

From https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get#get_vs._defineProperty

While using the get keyword and Object.defineProperty() have similar results, there is a subtle difference between the two when used on classes.

When using get the property will be defined on the instance's prototype, while using Object.defineProperty() the property will be defined on the instance it is applied to.

Assuming I've understood the above correctly, then the authors of Mongoose are trying to get the behaviour described for classes, in a file that doesn't use the class keyword. Considering they want the code to also work in browsers, which might not all support the class that's a reasonable choice.

That means that these are equivalent:

// ES2015
class Connection() {
    this._readyState = 0;

    get: function() {
        return this._readyState;
    },
    set: function(val) {
        this._readyState = val;
    }
}


// ES5.1
function Connection() {
    this._readyState = 0;
}

Object.defineProperty(Connection.prototype, "readyState", {
    get: function() {
        return this._readyState;
    },
    set: function(val) {
        this._readyState = val;
    }
});

Any solution for the issue, will have to be able to support both ways of creating accessors, on the prototype (as above) and on instances (see below).

// constructor
function Connection() {
    this._readyState = 0;
}

// instance
var connection = new Connection();

// expanding the instance with accessors, these won't exist on the prototype
Object.defineProperty(connection, "readyState", {
    get: function() {
        return this._readyState;
    },
    set: function(val) {
        this._readyState = val;
    },
    configurable: false
});

[mroderick]

The existing test at https://github.com/sinonjs/sinon/blob/master/test/stub-test.js#L3453-L3464 is a bit misleading, as it doesn't actually test restoring the stub.

Adding a stub.restore() to the end of that test, causes the same error to be thrown.

[mantoni]

More experiments:

This fails:

var o = {};
Object.defineProperty(o, "test", { value: 1 });
Object.defineProperty(o, "test", { value: 2 });

This works:

var o = {};
Object.defineProperty(o, "test", { writable: true, value: 1 });
Object.defineProperty(o, "test", { writable: true, value: 2 });

[mantoni]

Here is clearly something wrong with property replacement / restoring:

'use strict';

const sinon = require('sinon');

const proto = {};
const obj = {};
Object.setPrototypeOf(obj, proto);
Object.defineProperty(proto, 'test', { writable: true, value: 1 });

sinon.replace(obj, 'test', 2);
console.log(Object.getOwnPropertyDescriptor(obj, 'test'));

sinon.restore();
console.log(Object.getOwnPropertyDescriptor(obj, 'test'));

Prints

{ value: 2, writable: true, enumerable: true, configurable: true }
{ value: 1, writable: true, enumerable: false, configurable: false }

So after the restore() call, we have a new property on obj with the descriptor of proto.test.

[fatso83]

Simple fix with a discussion of doing some (separate) deeper changes in #2237

[mroderick]

The fix has been published to npm as [email protected]