Handling non-configurable object descriptors on the prototype (#2508)

* regression test for #2491

* fix: ensure we can always restore our own spies

* Add tests for mocks, fakes and stubs

This shows an incoherent appraoch to how we deal with object
descriptors across different code paths.

* fix: only bother with unconfigurable descriptors if they are our own

* remove test for sandbox.replace

never supported undefined or protypal props in the first place.
See #2195 for backing discussion on creating sinon.define()

* fix linting

Author: fatso83

lib/sinon/stub.js

@@ -134,7 +134,7 @@ function assertValidPropertyDescriptor(descriptor, property) {
     if (!descriptor || !property) {
         return;
     }
-    if (!descriptor.configurable && !descriptor.writable) {
+    if (descriptor.isOwn && !descriptor.configurable && !descriptor.writable) {
         throw new TypeError(
             `Descriptor for property ${property} is non-configurable and non-writable`
         );

lib/sinon/util/core/wrap-method.js

@@ -137,6 +137,7 @@ module.exports = function wrapMethod(object, property, method) {
         for (i = 0; i < types.length; i++) {
             mirrorProperties(methodDesc[types[i]], wrappedMethodDesc[types[i]]);
         }
+        methodDesc.configurable = true;
         Object.defineProperty(object, property, methodDesc);
 
         // catch failing assignment

test/issues/issues-test.js

@@ -822,4 +822,55 @@ describe("issues", function () {
             assert.isTrue(fooStubInstance.wasCalled);
         });
     });
+
+    describe("#2491 - unable to restore spies on an instance where the prototype has an unconfigurable property descriptor", function () {
+        function createInstanceFromClassWithReadOnlyPropertyDescriptor() {
+            class BaseClass {}
+            Object.defineProperty(BaseClass.prototype, "aMethod", {
+                value: function () {
+                    return 42;
+                },
+            });
+
+            // anchor
+            const instance = new BaseClass();
+            assert.equals(instance.aMethod(), 42);
+
+            return instance;
+        }
+
+        it("should ensure copied object descriptors are always configurable for spies", function () {
+            const instance =
+                createInstanceFromClassWithReadOnlyPropertyDescriptor();
+            this.sandbox.spy(instance, "aMethod");
+
+            refute.exception(() => {
+                this.sandbox.restore(); // #2491: this throws TypeError: Cannot assign to read only property 'myMethod' of object '#<BaseClass>'
+            });
+        });
+
+        it("should not throw if the unconfigurable object descriptor to be used for a Stub is on the prototype", function () {
+            const instance =
+                createInstanceFromClassWithReadOnlyPropertyDescriptor();
+
+            // per #2491 this throws 'TypeError: Descriptor for property aMethod is non-configurable and non-writable'
+            // that makes sense for descriptors taken from the object, but not its prototype, as we are free to change
+            // the latter when setting it
+            refute.exception(() => {
+                this.sandbox.stub(instance, "aMethod").returns("a stub");
+            });
+        });
+
+        it("should not throw if the unconfigurable object descriptor to be used for a Mock is on the prototype", function () {
+            const instance =
+                createInstanceFromClassWithReadOnlyPropertyDescriptor();
+
+            const mock = this.sandbox.mock(instance);
+
+            // per #2491 this throws 'TypeError: Attempted to wrap undefined property myMethod as function
+            refute.exception(() => {
+                mock.expects("aMethod").once();
+            });
+        });
+    });
 });