Fix issue 2534: spies are not restored (#2535)

* Document the custom property descriptor type used in Sinon

* Fix issue #2534

* prettify

Author: fatso83

lib/sinon/sandbox.js

@@ -218,6 +218,7 @@ function Sandbox() {
                 delete object[property];
             }
         }
+
         restorer.object = object;
         restorer.property = property;
         return restorer;
@@ -355,8 +356,7 @@ function Sandbox() {
     };
 
     function commonPostInitSetup(args, spy) {
-        const object = args[0];
-        const property = args[1];
+        const [object, property, types] = args;
 
         const isSpyingOnEntireObject =
             typeof property === "undefined" && typeof object === "object";
@@ -369,6 +369,11 @@ function Sandbox() {
             });
 
             usePromiseLibrary(promiseLib, ownMethods);
+        } else if (Array.isArray(types)) {
+            for (const accessorType of types) {
+                addToCollection(spy[accessorType]);
+                usePromiseLibrary(promiseLib, spy[accessorType]);
+            }
         } else {
             addToCollection(spy);
             usePromiseLibrary(promiseLib, spy);

lib/sinon/util/core/get-property-descriptor.js

@@ -1,6 +1,28 @@
 "use strict";
 
-module.exports = function getPropertyDescriptor(object, property) {
+/* eslint-disable jsdoc/valid-types */
+/*
+ * The following type def is strictly an illegal JSDoc, but the expression forms a
+ * legal Typescript union type and is understood by Visual Studio and the IntelliJ
+ * family of editors. The "TS" flavor of JSDoc is becoming the de-facto standard these
+ * days for that reason (and the fact that JSDoc is essentially unmaintained)
+ */
+
+/**
+ * @typedef {{isOwn: boolean} & PropertyDescriptor} SinonPropertyDescriptor
+ * a slightly enriched property descriptor
+ * @property {boolean} isOwn true if the descriptor is owned by this object, false if it comes from the prototype
+ */
+/* eslint-enable jsdoc/valid-types */
+
+/**
+ * Returns a slightly modified property descriptor that one can tell is from the object or the prototype
+ *
+ * @param {*} object
+ * @param {string} property
+ * @returns {SinonPropertyDescriptor}
+ */
+function getPropertyDescriptor(object, property) {
     let proto = object;
     let descriptor;
     const isOwn = Boolean(
@@ -19,4 +41,6 @@ module.exports = function getPropertyDescriptor(object, property) {
     }
 
     return descriptor;
-};
+}
+
+module.exports = getPropertyDescriptor;

test/issues/issues-test.js

@@ -899,4 +899,19 @@ describe("issues", function () {
             this.sandbox.restore();
         });
     });
+
+    it("#2534 - spies on accessors are not being cleaned up", function () {
+        const object = {
+            get prop() {
+                return "bar";
+            },
+        };
+        const spy = sinon.spy(object, "prop", ["get"]);
+        /* eslint-disable no-unused-expressions */
+        object.prop;
+        assert.equals(spy.get.callCount, 1);
+        sinon.restore();
+        object.prop;
+        assert.equals(spy.get.callCount, 1); // should remain unchanged
+    });
 });

test/sandbox-test.js

@@ -2205,7 +2205,7 @@ describe("Sandbox", function () {
             assert.equals(object.prop, "blabla");
         });
 
-        it("allows restoring setters", function () {
+        it("allows putting setters on fields and subsequently restoring them", function () {
             const object = {
                 prop: "bar",
             };
@@ -2221,6 +2221,37 @@ describe("Sandbox", function () {
 
             assert.equals(object.prop, "bla");
         });
+
+        it("allows replacing setters on fields and subsequently restoring them", function () {
+            const object = {
+                get prop() {
+                    return "bar";
+                },
+            };
+
+            const sandbox = new Sandbox();
+            const getter = sandbox.spy(() => "foobar");
+            sandbox.stub(object, "prop").get(getter);
+            assert.equals(object.prop, "foobar");
+            assert.equals(getter.callCount, 1);
+
+            sandbox.restore();
+            assert.equals(object.prop, "bar");
+        });
+
+        it("allows spying on accessors and subsequently restoring them", function () {
+            const object = {
+                get prop() {
+                    return "bar";
+                },
+            };
+            const sandbox = new Sandbox();
+            const spy = sandbox.spy(object, "prop", ["get"]);
+            sandbox.restore();
+            const descriptor = Object.getOwnPropertyDescriptor(object, "prop");
+            const getter = descriptor.get;
+            refute.equals(getter, spy.get);
+        });
     });
 
     describe(".assert", function () {