From b4bf432a6876214bd6e8773daf2ae1fe667b618c Mon Sep 17 00:00:00 2001 From: Carl-Erik Kopseng Date: Thu, 20 Apr 2023 09:35:07 +0200 Subject: [PATCH 1/6] regression test for #2491 --- test/issues/issues-test.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/issues/issues-test.js b/test/issues/issues-test.js index 662b15444..a5dc218eb 100644 --- a/test/issues/issues-test.js +++ b/test/issues/issues-test.js @@ -822,4 +822,25 @@ describe("issues", function () { assert.isTrue(fooStubInstance.wasCalled); }); }); + + describe("#2491 - unable to restore stubs on an instance where the prototype has an unconfigurable property descriptor", function () { + it("should ensure object descriptors are always configurable", function () { + class BaseClass {} + Object.defineProperty(BaseClass.prototype, "aMethod", { + value: function () { + return 42; + }, + }); + + // anchor + const instance = new BaseClass(); + assert.isFunction(instance.aMethod); + assert.equals(instance.aMethod(), 42); + this.sandbox.spy(instance, "aMethod") + + refute.exception(() => { + this.sandbox.restore(); // #2491: this throws "TypeError: Cannot assign to read only property 'myMethod' of object '#'" + }); + }); + }); }); From 84319011c8e2bf54e5470b788f4fe93f7d9ab58e Mon Sep 17 00:00:00 2001 From: Carl-Erik Kopseng Date: Thu, 20 Apr 2023 09:24:00 +0200 Subject: [PATCH 2/6] fix: ensure we can always restore our own spies --- lib/sinon/util/core/wrap-method.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/sinon/util/core/wrap-method.js b/lib/sinon/util/core/wrap-method.js index 64127721c..8acc7eeb3 100644 --- a/lib/sinon/util/core/wrap-method.js +++ b/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 From 24e28772bf4e866dfc968de5fbfe297b0c9bdccd Mon Sep 17 00:00:00 2001 From: Carl-Erik Kopseng Date: Thu, 20 Apr 2023 10:21:15 +0200 Subject: [PATCH 3/6] Add tests for mocks, fakes and stubs This shows an incoherent appraoch to how we deal with object descriptors across different code paths. --- test/issues/issues-test.js | 58 +++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/test/issues/issues-test.js b/test/issues/issues-test.js index a5dc218eb..64db5eb8f 100644 --- a/test/issues/issues-test.js +++ b/test/issues/issues-test.js @@ -823,9 +823,11 @@ describe("issues", function () { }); }); - describe("#2491 - unable to restore stubs on an instance where the prototype has an unconfigurable property descriptor", function () { - it("should ensure object descriptors are always configurable", function () { - class BaseClass {} + describe("#2491 - unable to restore spies on an instance where the prototype has an unconfigurable property descriptor", function () { + function createInstanceFromClassWithReadOnlyPropertyDescriptor() { + class BaseClass { + instanceProperty = 1; + } Object.defineProperty(BaseClass.prototype, "aMethod", { value: function () { return 42; @@ -834,13 +836,55 @@ describe("issues", function () { // anchor const instance = new BaseClass(); - assert.isFunction(instance.aMethod); assert.equals(instance.aMethod(), 42); - this.sandbox.spy(instance, "aMethod") + + 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 '#'" - }); + this.sandbox.restore(); // #2491: this throws TypeError: Cannot assign to read only property 'myMethod' of object '#' + }); + }); + + 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(); + }); + }); + + it("should not throw if the unconfigurable object descriptor to be used for a Fake is on the prototype", function () { + const instance = + createInstanceFromClassWithReadOnlyPropertyDescriptor(); + + // per #2491 this throws "TypeError: Cannot assign to read only property 'aMethod' of object '#'" + // 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.replace(instance, "aMethod", sinon.fake()); + }); }); }); }); From 0a8b32df204ce600375f955d984938d8738e0b28 Mon Sep 17 00:00:00 2001 From: Carl-Erik Kopseng Date: Thu, 20 Apr 2023 13:00:18 +0200 Subject: [PATCH 4/6] fix: only bother with unconfigurable descriptors if they are our own --- lib/sinon/stub.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/sinon/stub.js b/lib/sinon/stub.js index 309be1abb..beec9d924 100644 --- a/lib/sinon/stub.js +++ b/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` ); From aa9662b1ee5504f745a20add16afe354ea506823 Mon Sep 17 00:00:00 2001 From: Carl-Erik Kopseng Date: Thu, 20 Apr 2023 13:38:12 +0200 Subject: [PATCH 5/6] remove test for sandbox.replace never supported undefined or protypal props in the first place. See #2195 for backing discussion on creating sinon.define() --- test/issues/issues-test.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/test/issues/issues-test.js b/test/issues/issues-test.js index 64db5eb8f..8d675af29 100644 --- a/test/issues/issues-test.js +++ b/test/issues/issues-test.js @@ -874,17 +874,5 @@ describe("issues", function () { mock.expects("aMethod").once(); }); }); - - it("should not throw if the unconfigurable object descriptor to be used for a Fake is on the prototype", function () { - const instance = - createInstanceFromClassWithReadOnlyPropertyDescriptor(); - - // per #2491 this throws "TypeError: Cannot assign to read only property 'aMethod' of object '#'" - // 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.replace(instance, "aMethod", sinon.fake()); - }); - }); }); }); From 040b144490e159a19ae988abadf1f3cb0123b93b Mon Sep 17 00:00:00 2001 From: Carl-Erik Kopseng Date: Thu, 20 Apr 2023 13:49:17 +0200 Subject: [PATCH 6/6] fix linting --- test/issues/issues-test.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/issues/issues-test.js b/test/issues/issues-test.js index 8d675af29..e616e03a8 100644 --- a/test/issues/issues-test.js +++ b/test/issues/issues-test.js @@ -825,9 +825,7 @@ describe("issues", function () { describe("#2491 - unable to restore spies on an instance where the prototype has an unconfigurable property descriptor", function () { function createInstanceFromClassWithReadOnlyPropertyDescriptor() { - class BaseClass { - instanceProperty = 1; - } + class BaseClass {} Object.defineProperty(BaseClass.prototype, "aMethod", { value: function () { return 42;