Skip to content

Commit

Permalink
Handling non-configurable object descriptors on the prototype (#2508)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
fatso83 authored Apr 20, 2023
1 parent 3b41aff commit e9042c4
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 1 deletion.
2 changes: 1 addition & 1 deletion lib/sinon/stub.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
);
Expand Down
1 change: 1 addition & 0 deletions lib/sinon/util/core/wrap-method.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
51 changes: 51 additions & 0 deletions test/issues/issues-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
});
});

0 comments on commit e9042c4

Please sign in to comment.