sandbox.restore() restores to the wrong value when sandbox.stub()ing twice.

[saifelse]

• Sinon version : 1.17.6, 2.0.0-pre.4
• Environment : node
• Other libraries you are using: none

How to reproduce
http://jsfiddle.net/totfnww7/1/

var sandbox = sinon.sandbox.create();
var obj = {a: 1};
console.log('Before stub:', obj.a, ', correct:', obj.a === 1); // Before stub: 1, correct; true
sandbox.stub(obj, 'a', 2);
console.log('After stub:', obj.a, ', correct:', obj.a === 2); // After stub: 2, correct: true
sandbox.restore();
console.log('After restore:', obj.a, ', correct:', obj.a === 1); // After restore: 1, correct: true

console.log('===');

var sandbox2 = sinon.sandbox.create();
var obj2 = {a: 1};
console.log('Before stub:', obj2.a, ', correct:', obj2.a === 1); // Before stub: 1, correct: true
sandbox.stub(obj2, 'a', 2);
console.log('After stub:', obj2.a, ', correct:', obj2.a === 2); // After stub: 2, correct: true
sandbox.stub(obj2, 'a', 3);
console.log('After 2nd stub:', obj2.a, ', correct:', obj2.a === 3); // After 2nd stub: 3, correct: true
sandbox.restore();
console.log('After restore:', obj2.a, ', correct:', obj2.a === 1); // After restore: 2, correct: false

What did you expect to happen?
Expected last output to be: "After restore: 1, correct: true" -- expecting sinon to restore to the original value

What actually happens
But it outputted "After restore: 2, correct: false" -- sinon restored to the //previous// stubbed value.

Actual cause

1. sinon/lib/sinon/collection.js keeps track of stubbed values's restores in the array collection.fakes.
2. NB: There is nothing that prevents you from stubbing a property value twice, unlike methods (this would require more record keeping from what I can tell)
3. The restore code iterates over the restore methods in the order in which they were added; BUT it should be iterating in reverse.

A fix that I think works is:

    function each(fakeCollection, method, reverse) {
        var fakes = getFakes(fakeCollection);
        if (reverse) {
            fakes = fakes.slice().reverse();
        }
        for (var i = 0, l = fakes.length; i < l; i += 1) {
            if (typeof fakes[i][method] === "function") {
                fakes[i][method]();
            }
        }
    }
...
    function makeApi(sinon) {
        var collection = {
          ...
            restore: function restore() {
                each(this, "restore", true /* reverse */);
                compact(this);
            },

I can submit a PR if this sounds correct.

[fatso83]

Hi, while it's great to have this fixed, I'm not sure we should support this use case. Multiple stubbings of the same property seems like an anti-pattern IMHO, as it leads to hard to read/complex tests.

So I'd rather have Sinon throw an explicit error on encountering this than trying to fix it, but ... there's more than one way to view things, so I'd be interested to hear what the others say before flat out dismissing this work to improve this.

[mroderick]

I would also prefer that sandbox throws an error when trying to stub the same property twice, just like we do in wrap-method.js when user tries to wrap the same method twice.

[saifelse]

From briefly looking through the code, it looks like this would be a somewhat involved refactor, since the existing wrap for properties just overwrites the property and has a closured method to restore the property; whereas for methods, sinon stores extra data on the function itself (which we are afforded by Functions being non-primitives)... however, the bookkeeping for properties would have to be in an auxiliary data structure... presumably in a map from object to set of stubbed property names. For being the most defensive, it would make sense for this data structure to be shared across all sandboxes, otherwise, you may re-define a property multiple times. Does this sound correct?

However, I would argue that even if the original suggestion can encourage an anti-pattern, it does seem more intuitive for the restore to happen in reverse, as you are effectively "rewinding" the changes that were made by sinon. In my mind, it makes sense that the the core logic is applying these changes to stub things, applying the changes in reverse to restore things, and then erroring is to discourage the anti-patterns of stubbing things twice.

I propose that suggested change should still be added (even easily added for the 1.x release), and then bookkeeping can be added on-top of it in a separate PR.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.