Sinon: Unable to stub properties attached to a function if they have a setter

[danShumway]

• Sinon version : 2.3.2 (I believe 1.7.x as well, but would need to retest to make sure)
• Environment : Any ES5 compatible browser or environment
• Example URL : https://jsfiddle.net/1ro5xe4h/8/
• Other libraries you are using: none
• May be related to: #1445

Stubbing a function on a function fails with the following error:

TypeError: Attempted to wrap undefined property entry as function
    at checkWrappedMethod (VM6617 sinon-2.3.2.js:3587)
    at wrapMethod (VM6617 sinon-2.3.2.js:3636)
    at Object.stub (VM6617 sinon-2.3.2.js:2820)
    at window.onload ((index):61)

How to reproduce

var target = function () {};
var _entry = function () { return false; };
Object.defineProperty(target, 'entry', {
    get : function () {
    return _entry;
    },
    set : function (value) {
        _entry = value;
    }
});

sinon.stub(target, 'entry').returns(true);

Stubbing with a setter fails, but succeeds if a normal property is given instead. This leads me to suspect that it is specifically related to how the property is defined.

I'm not familiar with Sinon's code, so disregard this if I'm off-base, but the problem seems to be related to an assumption that value will be attached to all property descriptors in wrap-method.js.

var methodDesc = (typeof method === "function") ? {value: method} : method;
var wrappedMethodDesc = getPropertyDescriptor(object, property);

//etc....

var types = Object.keys(methodDesc);
for (i = 0; i < types.length; i++) {
     wrappedMethod = wrappedMethodDesc[types[i]];
     checkWrappedMethod(wrappedMethod);
}

Properties that use getters and setters will fail here, since their property descriptors won't have a value for you to read. As a result, undefined will (incorrectly) get passed into checkWrappedMethod.

[lucasfcosta]

Hi, @danShumway, thanks for your issue!

I just tried to reproduce it with the testcase below on Sinon's latest version, but I was not able to get the same error as you did.

    it.only("should be able to stub properties with no value in their descriptor", function () {
        var target = function () {};
        var _entry = function () { return false; };
        Object.defineProperty(target, "entry", {
            get : function () {
                return _entry;
            },
            set : function (value) {
                _entry = value;
            }
        });

        createStub(target, "entry").returns(true);
});

Can you please confirm your problem has been solved in Sinon's latest version?

I think this won't happen anymore because of this check when creating the stub.

Because of that we won't call wrapMethod. We will just return the newly created stub.

Please let me know if I've made any misconceptions and if your problem persists.

[danShumway]

Hi @lucasfcosta, thanks so much for helping out.

I've retested with Sinon 2.3.4. I'm no longer seeing an exception, and I do get a stub back, but the property is unfortunately still not being actually mocked.

I'm sure I'm missing something in the code, but it doesn't look like stub.create ever attaches the mock to the object?

Almost definitely I'm just misreading the code though since normal mocking works correctly. Setting the property as configurable doesn't seem to do anything, adding writable allows it to get mocked correctly - but of course writable can't be set unless I get rid of the getter/setter :)

[lucasfcosta]

Hi @danShumway, I'm always happy to help.

I'm not sure I understand your problem completely. Actually, it's not stub.create which is responsible for attaching the stub to an object. That method is only responsible for creating a "dummy function" which has a bunch of properties responsible for keeping track of calls, arguments and this kind of stuff.

The function that is called when you use sinon.stub(obj, propName) is the stub function.

Also, your problem is that you are using the returns method instead of the value method. By using the returns method you are turning that property into a function, which means that you would have to invoke it in order to get true back.

If this does not solve your problem, can you elaborate what you mean by saying "it does not attach the mock to the object"? Just to make sure we're talking about the same thing.

Thank you very much for getting in touch and please let me know if there's anything I can do to help you or if you have any further problems.

[danShumway]

:) Seems like I was way off with how attachment happens. I can clarify more about the output I'm seeing though.

I've updated my example, and I'll give the short version here.

This is the behavior I currently see with Sinon 2.3.4, which is what I expected based on my understanding of the API:

var target = function () {};
var _entry = function () { return 'original'; };
target.entry = _entry;

sinon.stub(target, 'entry').returns('mocked');
target.entry(); //returns 'mocked'
(target.entry === _entry); //false: target.entry has been replaced by the mock

However, here's the behavior I see if I'm using a setter.

var target = function () {};
var _entry = function () { return 'original'; }
Object.defineProperty(target, 'entry', {
    get : function () { return _entry; },
    set : function (value) { _entry = value; }
});

sinon.stub(target, 'entry').returns('mocked');
target.entry(); //returns 'original', not like the previous example
(target.entry === _entry); //true: target.entry has been left untouched.

In both of these examples, I'm calling the same code to stub the attached method. If entry is attached as a standard writable property, then calling sinon.stub replaces the method. If entry is attached with a setter, it does not get replaced.

sinon.stub still returns a stub, and I can manipulate it and even call restore on it. But the stub is not actually mocking anything - it exists in complete isolation from the object that I called it on. Calling the method that I stubbed (entry) still calls the original method.

[danShumway]

Some light debugging:

I can confirm that the setter is never called at all:

Object.defineProperty(target, 'entry', {
    get : function () { return _entry; },
    set : function (value) {
        throw 'the setter was at least called'; //No exceptions are thrown
    }
});

I can confirm that the property does not get mocked even if it is configurable (Sinon isn't attempting to redefine the entire property)

Object.defineProperty(target, 'entry', {
    get : function () { return _entry; },
    set : function (value) { _entry = value; },
    configurable : true // I would prefer not needing to set this to true, but if it was necessary I could deal.
});

I can confirm that the property does not get mocked even if it is attached to an object literal rather than a function (It seems to be a problem specifically with the setter)

var _entry = function () { return 'original'; };
var target = {
    get entry () { return _entry; },
    set entry (value) { _entry = value; }
};

sinon.stub(target, 'entry').returns('mocked');
target.entry(); //returns 'original'
(target.entry === _entry); //true: target.entry has been left untouched.

[danShumway]

Oh, crud, I can't believe I missed that! It's line 24. Sorry for not seeing that sooner, I threw you off track linking to the stub.create.

var isStubbingNonFuncProperty = (typeof object === "object" || typeof object === "function")
                                    && typeof property !== "undefined"
                                    && (typeof actualDescriptor === "undefined"
                                    || typeof actualDescriptor.value !== "function")
                                    && typeof descriptor === "undefined";

For a property with a setter, the type of descriptor.value will always be undefined. That means that it will always get listed as non-function property, which means it will never actually get wrapped.

return isStubbingNonFuncProperty ? s : wrapMethod(object, property, s);

You'll probably also want to update Line 27 as well to match the original fix that was made: (typeof object === "object" || typeof object === "function"). It's possible that the original wrap-method issue may cause problems once it starts getting called? I didn't run all the way through the code to make sure.

Not sure if I'll have time this weekend, but if I do I'll try to clone if I can and see if that actually fixes.

[danShumway]

Taking a look at this, but I'll need some clarification of how you want to handle it.

stub-test.js defines some examples of how Sinon should handle restoring objects with getters.

What's the spirit of this test?

it("does not call getter during restore", function () {
    var obj = {
        get prop() {
            fail("should not call getter");
        }
    };
    var stub = createStub(obj, "prop", {get: function () {
        return 43;
    }});

    assert.equals(obj.prop, 43);

    stub.restore();
});

If the intent is for Sinon to never call a getter while stubbing a property, it's going to be very difficult to do anything with a setter.

In order to figure out whether you're stubbing a property or a method, you need some way to get the value. If you can't do that via a getter, effectively Sinon can't mock anything with a getter - or at least, it needs to assume that anything with a getter is not a method.

I think that's a major loss of functionality from previous versions, but it's not up to me what the spec is, so I'll defer to others' perspectives.

[danShumway]

I can do the getter as a last-resort. So if there's ANY way at all to avoid calling it, I'll do so, BUT if you have a descriptor with a getter and it doesn't fall under any other category...

That gets rid of most getter related errors, but not all of them. You're still going to run into a few tests like this:

it("replaces old getters", function () {
    var myObj = {
        get prop() {
            fail("should not call the old getter");
        }
    };

    createStub(myObj, "prop").get(function getterFn() {
        return "bar";
    });

    assert.equals(myObj.prop, "bar");
});

I can't think of any way to get around something like that without either calling the getter or making a library-wide assumption that anything with a getter should be treated like a non-function property. At this point this really just becomes a question of "what do you want your API to be?"

Maybe it's just late and I'm not thinking clearly anymore.

[lucasfcosta]

Hi @danShumway, thanks for all this awesome investigation! Sorry for the delay on answering you.

Every descriptor has a value property which indicates the real value of a property. In order to avoid calling getters we consider that the value of a property is what really represents its type.

So, in order to avoid that you could use Object.getOwnPropertyDescriptor and check every object's prototype chains recursively in order to find the property you want and them get its descriptor. After that just do descriptor.value. Also, we already have a method which gets any property descriptors for any object (including checking its prototypes) as you can see here.

If that is enough to solve your problem, great! Otherwise just let me know if you need anything and I'll be more than happy to help.

Thanks for working on this, btw.

[danShumway]

Sorry, I know I can be a little long-winded sometimes - just to be clear, I'm specifically talking about getters and setters. Accessor descriptors don't have a value, they only have a get method, and the only way to figure out what it returns is to call it and see.

I can fix Sinon to act like I originally expected it to above, but I can't fix it in a way that perfectly preserves existing behavior. When Sinon encounters a getter it has to make a decision. It can do one of two things:

• Treat any property with an accessor as a non-method, regardless of whether or not it returns a function.
• Call the getter attached to an Acessor descriptors once to determine whether or not the property being stubbed is a function.

That's not a decision I can make though, it really comes down to what you want the API to be for Sinon. I'm OK with either behavior as long as it's explicit, and if you want I'd also be happy to put together a pull request for whichever one you'd like. :smile:

[lucasfcosta]

Hi @danShumway, yes, you're right. I haven't noticed that. Thanks for all this info and please don't feel sorry for all these awesome comments. I'd rather have to read long and well made comments than having to spend a lot more time figuring out things unecessarily. You're doing a great job.

So, let me explore a few other facts that might help you into finding an optimal solution for this.

In this example:

var _entry = function () { return 'original'; };
var target = {
    get entry () { return _entry; },
    set entry (value) { _entry = value; }
};

sinon.stub(target, 'entry').returns('mocked');
target.entry(); //returns 'original'
(target.entry === _entry); //true: target.entry has been left untouched.

The stubbing works, the problem is it is not being added to the object.

This line right here:

return isStubbingNonFuncProperty ? s : wrapMethod(object, property, s);

Returns a stub that has been previously created, but it does not add that to the object in order to replace the old one.

Things like the one below work:

it("allows stubbing property descriptor values", function () {
    var myObj = {
        prop: "rawString"
    };

    createStub(myObj, "prop").value("newString");
    assert.equals(myObj.prop, "newString");
});

But they work not because createStub changed that property in myObj to become the newly created stub, they work because we call Object.defineProperty in the wrapped object when calling value.

So, in order to solve this, we could simply treat properties with undefined values as "non-function" properties and start replacing the old properties in an object with them after creating the stub and the restore method.

This might be enough to solve this problem and preserve all the old behavior.

Please correct me if I'm wrong or if I've made any incorrect assumptions.

Also, if you'd like to do some pair programming or if you need to chat in more detail so that we can figure things out as they appear, please get in touch.

(awesome job dealing with this problem, btw, thank you very much for your time and attention)

[danShumway]

My pleasure, being able to contribute to a library I use is a privilege, that's what makes Open Source so great.

I'd be fine with the solution you mention - in which case there's actually not a lot of coding that needs to be done since it's already happening. The only thing I'd suggest is it might be beneficial to make that behavior more explicit so it's more obvious that it's intended, and it might be a good idea to update the docs.

Let me try to put together a pull request for those two things, if I run into code I don't understand I might take you up on your offer.

[fatso83]

Just one minor thing ... Shouldn't that getter be configured as writeable if you should be able to overwrite it? The default value is false, AFAIK.

[stale[bot]]

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.