From e5b43de59fcad8f32f3a64930fb63bb05f2c5482 Mon Sep 17 00:00:00 2001 From: Maximilian Antoni Date: Sun, 6 May 2018 15:18:58 +0200 Subject: [PATCH] Refuse to replace already replaced values When values where replaced multiple times, `restore` didn't restore the original value. This change causes `sandbox.replace` to throw if the given object / property combination was already used. Fixes #1779 --- docs/release-source/release/sandbox.md | 6 +-- lib/sinon/sandbox.js | 21 +++++++++- lib/sinon/stub.js | 2 +- test/sandbox-test.js | 57 ++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 6 deletions(-) diff --git a/docs/release-source/release/sandbox.md b/docs/release-source/release/sandbox.md index 4a4343d66..dff81b89a 100644 --- a/docs/release-source/release/sandbox.md +++ b/docs/release-source/release/sandbox.md @@ -130,7 +130,7 @@ A convenience reference for [`sinon.assert`](./assertions) #### `sandbox.replace(object, property, replacement);` -Replaces `property` on `object` with `replacement` argument. +Replaces `property` on `object` with `replacement` argument. Attempts to replace an already replaced value cause an exception. `replacement` can be any value, including `spies`, `stubs` and `fakes`. @@ -154,7 +154,7 @@ console.log(myObject.myMethod()); #### `sandbox.replaceGetter();` -Replaces getter for `property` on `object` with `replacement` argument. +Replaces getter for `property` on `object` with `replacement` argument. Attempts to replace an already replaced getter cause an exception. `replacement` must be a `Function`, and can be instances of `spies`, `stubs` and `fakes`. @@ -175,7 +175,7 @@ console.log(myObject.myProperty); #### `sandbox.replaceSetter();` -Replaces setter for `property` on `object` with `replacement` argument. +Replaces setter for `property` on `object` with `replacement` argument. Attempts to replace an already replaced setter cause an exception. `replacement` must be a `Function`, and can be instances of `spies`, `stubs` and `fakes`. diff --git a/lib/sinon/sandbox.js b/lib/sinon/sandbox.js index fce6468f2..9d63c084d 100644 --- a/lib/sinon/sandbox.js +++ b/lib/sinon/sandbox.js @@ -161,9 +161,20 @@ function Sandbox() { function getFakeRestorer(object, property) { var descriptor = getPropertyDescriptor(object, property); - return function restorer() { + function restorer() { Object.defineProperty(object, property, descriptor); - }; + } + restorer.object = object; + restorer.property = property; + return restorer; + } + + function verifyNotReplaced(object, property) { + fakeRestorers.forEach(function (fakeRestorer) { + if (fakeRestorer.object === object && fakeRestorer.property === property) { + throw new TypeError("Attempted to replace " + property + " which is already replaced"); + } + }); } sandbox.replace = function replace(object, property, replacement) { @@ -189,6 +200,8 @@ function Sandbox() { throw new TypeError("Cannot replace " + typeof object[property] + " with " + typeof replacement); } + verifyNotReplaced(object, property); + // store a function for restoring the replaced property push.call(fakeRestorers, getFakeRestorer(object, property)); @@ -212,6 +225,8 @@ function Sandbox() { throw new Error("`object.property` is not a getter"); } + verifyNotReplaced(object, property); + // store a function for restoring the replaced property push.call(fakeRestorers, getFakeRestorer(object, property)); @@ -238,6 +253,8 @@ function Sandbox() { throw new Error("`object.property` is not a setter"); } + verifyNotReplaced(object, property); + // store a function for restoring the replaced property push.call(fakeRestorers, getFakeRestorer(object, property)); diff --git a/lib/sinon/stub.js b/lib/sinon/stub.js index 9b26350ff..997e34fd1 100644 --- a/lib/sinon/stub.js +++ b/lib/sinon/stub.js @@ -104,8 +104,8 @@ var proto = { return getCurrentBehavior(fnStub).invoke(this, arguments); }; - functionStub.id = "stub#" + uuid++; var orig = functionStub; + functionStub.id = "stub#" + uuid++; functionStub = spy.create(functionStub, stubLength); functionStub.func = orig; diff --git a/test/sandbox-test.js b/test/sandbox-test.js index 2cf731dd0..cd07744a1 100644 --- a/test/sandbox-test.js +++ b/test/sandbox-test.js @@ -506,6 +506,34 @@ describe("Sandbox", function () { }, {message: "Cannot replace function with string"}); }); + it("should refuse to replace a fake twice", function () { + var sandbox = this.sandbox; + var object = { + property: function () { + return "apple pie"; + } + }; + + sandbox.replace(object, "property", fake()); + + assert.exception(function () { + sandbox.replace(object, "property", fake()); + }, {message: "Attempted to replace property which is already replaced"}); + }); + + it("should refuse to replace a string twice", function () { + var sandbox = this.sandbox; + var object = { + property: "original" + }; + + sandbox.replace(object, "property", "first"); + + assert.exception(function () { + sandbox.replace(object, "property", "second"); + }, {message: "Attempted to replace property which is already replaced"}); + }); + it("should return the replacement argument", function () { var replacement = "replacement"; var existing = "existing"; @@ -630,6 +658,21 @@ describe("Sandbox", function () { assert.equals(object.foo, "bar"); }); + + it("should refuse to replace a getter twice", function () { + var sandbox = this.sandbox; + var object = { + get foo() { + return "bar"; + } + }; + + sandbox.replaceGetter(object, "foo", fake.returns("one")); + + assert.exception(function () { + sandbox.replaceGetter(object, "foo", fake.returns("two")); + }, {message: "Attempted to replace foo which is already replaced"}); + }); }); describe(".replaceSetter", function () { @@ -728,6 +771,20 @@ describe("Sandbox", function () { assert.equals(object.prop, "bla"); }); + + it("should refuse to replace a setter twice", function () { + var sandbox = this.sandbox; + // eslint-disable-next-line accessor-pairs + var object = { + set foo(value) {} + }; + + sandbox.replaceSetter(object, "foo", fake()); + + assert.exception(function () { + sandbox.replaceSetter(object, "foo", fake.returns("two")); + }, {message: "Attempted to replace foo which is already replaced"}); + }); }); describe(".reset", function () {