Merge pull request #2237 from fatso83/issue-2226

Fix 2226: restore props defined on prototype chain by deleting
sinonjs · Mar 9, 2020 · 775e53b · 775e53b
2 parents 5436466 + 92dc087
commit 775e53b
Show file tree

Hide file tree

Showing 8 changed files with 76 additions and 14 deletions.
diff --git a/lib/sinon/default-behaviors.js b/lib/sinon/default-behaviors.js
@@ -258,7 +258,7 @@ var defaultBehaviors = {
         Object.defineProperty(rootStub.rootObj, rootStub.propName, {
             value: newVal,
             enumerable: true,
-            configurable: isPropertyConfigurable(rootStub.rootObj, rootStub.propName)
+            configurable: rootStub.shadowsPropOnPrototype || isPropertyConfigurable(rootStub.rootObj, rootStub.propName)
         });
 
         return fake;

diff --git a/lib/sinon/sandbox.js b/lib/sinon/sandbox.js
@@ -191,7 +191,11 @@ function Sandbox() {
         var descriptor = getPropertyDescriptor(object, property);
 
         function restorer() {
-            Object.defineProperty(object, property, descriptor);
+            if (descriptor.isOwn) {
+                Object.defineProperty(object, property, descriptor);
+            } else {
+                delete object[property];
+            }
         }
         restorer.object = object;
         restorer.property = property;

diff --git a/lib/sinon/stub.js b/lib/sinon/stub.js
@@ -6,7 +6,7 @@ var behaviors = require("./default-behaviors");
 var createProxy = require("./proxy");
 var functionName = require("@sinonjs/commons").functionName;
 var hasOwnProperty = require("@sinonjs/commons").prototypes.object.hasOwnProperty;
-var isNonExistentOwnProperty = require("./util/core/is-non-existent-own-property");
+var isNonExistentProperty = require("./util/core/is-non-existent-property");
 var spy = require("./spy");
 var extend = require("./util/core/extend");
 var getPropertyDescriptor = require("./util/core/get-property-descriptor");
@@ -69,8 +69,8 @@ function stub(object, property) {
 
     throwOnFalsyObject.apply(null, arguments);
 
-    if (isNonExistentOwnProperty(object, property)) {
-        throw new TypeError("Cannot stub non-existent own property " + valueToString(property));
+    if (isNonExistentProperty(object, property)) {
+        throw new TypeError("Cannot stub non-existent property " + valueToString(property));
     }
 
     var actualDescriptor = getPropertyDescriptor(object, property);
@@ -97,8 +97,9 @@ function stub(object, property) {
     extend.nonEnum(s, {
         rootObj: object,
         propName: property,
+        shadowsPropOnPrototype: !actualDescriptor.isOwn,
         restore: function restore() {
-            if (actualDescriptor !== undefined) {
+            if (actualDescriptor !== undefined && actualDescriptor.isOwn) {
                 Object.defineProperty(object, property, actualDescriptor);
                 return;
             }

diff --git a/lib/sinon/util/core/get-property-descriptor.js b/lib/sinon/util/core/get-property-descriptor.js
@@ -3,9 +3,15 @@
 module.exports = function getPropertyDescriptor(object, property) {
     var proto = object;
     var descriptor;
+    var isOwn = Boolean(object && Object.getOwnPropertyDescriptor(object, property));
 
     while (proto && !(descriptor = Object.getOwnPropertyDescriptor(proto, property))) {
         proto = Object.getPrototypeOf(proto);
     }
+
+    if (descriptor) {
+        descriptor.isOwn = isOwn;
+    }
+
     return descriptor;
 };
diff --git a/lib/sinon/util/core/is-non-existent-own-property.js b/lib/sinon/util/core/is-non-existent-own-property.js
diff --git a/lib/sinon/util/core/is-non-existent-property.js b/lib/sinon/util/core/is-non-existent-property.js
@@ -0,0 +1,12 @@
+"use strict";
+
+/**
+ * @param {*} object
+ * @param {String} property
+ * @returns whether a prop exists in the prototype chain
+ */
+function isNonExistentProperty(object, property) {
+    return object && typeof property !== "undefined" && !(property in object);
+}
+
+module.exports = isNonExistentProperty;
diff --git a/test/issues/issues-test.js b/test/issues/issues-test.js
@@ -580,9 +580,11 @@ describe("issues", function() {
             function ClassWithoutProps() {
                 return;
             }
+
             function AnotherClassWithoutProps() {
                 return;
             }
+
             ClassWithoutProps.prototype.constructor = ClassWithoutProps;
             AnotherClassWithoutProps.prototype.constructor = AnotherClassWithoutProps;
             var arg1 = new ClassWithoutProps(); //arg1.constructor.name === ClassWithoutProps
@@ -640,6 +642,7 @@ describe("issues", function() {
         function Foo() {
             return;
         }
+
         // eslint-disable-next-line mocha/no-setup-in-describe
         Foo.prototype.testMethod = function() {
             return;
@@ -679,6 +682,7 @@ describe("issues", function() {
         function Foo() {
             return;
         }
+
         // eslint-disable-next-line mocha/no-setup-in-describe
         Foo.prototype.testMethod = function() {
             return 1;
@@ -709,4 +713,46 @@ describe("issues", function() {
             refute.equals(fake.lastArg, 3);
         });
     });
+
+    describe("#2226 - props on prototype are not restored correctly", function() {
+        function createObjectWithPropFromPrototype() {
+            var proto = {};
+            var obj = {};
+
+            Object.setPrototypeOf(obj, proto);
+            Object.defineProperty(proto, "test", { writable: true, value: 1 });
+            return obj;
+        }
+
+        it("should restore fakes shadowing prototype props correctly", function() {
+            var obj = createObjectWithPropFromPrototype();
+
+            var originalPropertyDescriptor = Object.getOwnPropertyDescriptor(obj, "test");
+
+            sinon.replace(obj, "test", 2);
+            var replacedPropertyDescriptor = Object.getOwnPropertyDescriptor(obj, "test");
+
+            sinon.restore();
+            var restoredPropertyDescriptor = Object.getOwnPropertyDescriptor(obj, "test");
+
+            assert.isUndefined(originalPropertyDescriptor);
+            refute.isUndefined(replacedPropertyDescriptor);
+            assert.isUndefined(restoredPropertyDescriptor);
+        });
+
+        it("should restore stubs shadowing prototype props correctly", function() {
+            var obj = createObjectWithPropFromPrototype();
+            var originalPropertyDescriptor = Object.getOwnPropertyDescriptor(obj, "test");
+
+            sinon.stub(obj, "test").value(2);
+            var replacedPropertyDescriptor = Object.getOwnPropertyDescriptor(obj, "test");
+
+            sinon.restore();
+            var restoredPropertyDescriptor = Object.getOwnPropertyDescriptor(obj, "test");
+
+            assert.isUndefined(originalPropertyDescriptor);
+            refute.isUndefined(replacedPropertyDescriptor);
+            assert.isUndefined(restoredPropertyDescriptor);
+        });
+    });
 });
diff --git a/test/sandbox-test.js b/test/sandbox-test.js
@@ -562,7 +562,7 @@ describe("Sandbox", function() {
                     function() {
                         sandbox.stub(object, Symbol());
                     },
-                    { message: "Cannot stub non-existent own property Symbol()" },
+                    { message: "Cannot stub non-existent property Symbol()" },
                     TypeError
                 );