Refuse to replace already replaced values #1785
Conversation
lib/sinon/stub.js
Outdated
| var orig = functionStub; | ||
| functionStub = spy.create(functionStub, stubLength); | ||
| functionStub.func = orig; | ||
| functionStub.id = "stub#" + uuid++; |
There was a problem hiding this comment.
Uh, sorry. Commited this by accident. I noticed that the generated stub#${uuid} id property is replaced by spy.create with spy#{uuid}. This change fixes it, but it's not really part of this PR. Shall I revert or just leave it?
There was a problem hiding this comment.
Maybe just put it into it's own commit?
lib/sinon/sandbox.js
Outdated
| } | ||
|
|
||
| function verifyNotReplaced(object, property) { | ||
| for (var i = 0; i < fakeRestorers.length; i++) { |
There was a problem hiding this comment.
Since we're targeting ES5.1, it's fine to use the ES5 Array Extras... it is much harder to create off-by-one errors, when you don't need to maintain an index 😉
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");
}
});
}There was a problem hiding this comment.
Yeah, you're right. And even forEach would give us an index as the second argument, if needed.
lib/sinon/stub.js
Outdated
| var orig = functionStub; | ||
| functionStub = spy.create(functionStub, stubLength); | ||
| functionStub.func = orig; | ||
| functionStub.id = "stub#" + uuid++; |
There was a problem hiding this comment.
Maybe just put it into it's own commit?
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
mantoni
force-pushed
the
sandbox-replace-only-once
branch
from
ebabc59
to
00432ae
Compare
|
This has been published as |
Fix for issue #1779
When values where replaced multiple times,
restoredidn't restore the original value. This change causessandbox.replaceto throw if the given object / property combination was already used.How to verify - mandatory
npm installnpm testor
Versioning
While this fixes an issue where
sandbox.restore()fails to restore values that where replaced multiple times, this might also cause existing tests to fail. However, I would argue that we're fixing a broken behavior in the newsandbox.replaceAPI to bring it in line withsandbox.stubwhich also throws in this case. Therefore I'd vote for semver patch.Checklist for author
npm run lintpasses