sandbox.stub fails to stub methods on the prototype

[RaceProUK]

• Sinon version: 2.4.1 and 3.0.0
• Environment: Node 8 (also happens in earlier versions of Node)

What did you expect to happen?
I expected that calling sandbox.stub(object, 'property'), where property is a function on the prototype, would result in the function being successfully stubbed with a no-op stub.

What actually happens
We receive the exception TypeError: Cannot stub non-existent own property <property>

How to reproduce
(Test case added August 7 by @fatso83)

const sinon = require('sinon')

class TestObject {
    someFunction() {
        console.error('sinon should stub me!');
    }
}

const sandbox = sinon.sandbox.create();

const instance = new TestObject();
const stub = sandbox.stub(instance, 'someFunction'); // This fails on Sinon 3.0, it worked on Sinon 2.x

Diagnosis
It appears the issue is in the check on like 89 of collections.js, specifically the call Object.prototype.hasOwnProperty.call(object, property). It appears that hasOwnProperty() doesn't enumerate ES6 functions and properties.

Proposed fix
Instead of calling the above, use the test Object.getOwnPropertyNames(Object.getPrototypeOf(object)).indexOf(property) > -1. Our testing indicates that this method does enumerate ES6 functions and properties in addition to ES5 functions and properties.

[fatso83]

Thank you for your bug report, but I have no idea what "an es6 function" is. Do you mean an "arrow function" - a function whose context is the environment in which it was defined, or are you thinking about the proposed standard for class methods in esnext that are pre-bound to the instance? Or something else entirely? Not getting wiser by the prose, I am afraid.

A reproducible snippet of code speaks more than three paragraphs of text :-)

[RaceProUK]

My apologies 😊

By ES6 function, I mean defining a member function with the ES6 class syntax, as we do here:
https://github.com/SockDrawer/SockBot/blob/master/providers/nodebb/index.js#L157-L160

One of the tests we're seeing with this issue is here:
https://github.com/SockDrawer/SockBot/blob/master/test/providers/nodebb/indexTest.js#L265

As the first link shows, the function does exist, but it's not enumerated in the hasOwnProperty() check, which is causing these tests to fail:
https://travis-ci.org/SockDrawer/SockBot/jobs/260642010#L969

But it does seem that getting the prototype of the object and checking the members of that does find the member function, as well as any defined int he old ES5 way (directly manipulating the prototype).

[AccaliaDeElementia]

Minimal reproduction of the issue described:

https://gist.github.com/AccaliaDeElementia/c68ddbc0b49d12e1eec9079a28370c68

[fatso83]

Hmm ... thanks for the updates and the reproducible snippet. Having the snippet means we can automate the process of finding the commit that introduced the regression (using git bisect).

It is true that the property is not the object's own property, rather it's further up the prototype, but since this was working before we should fix it. I suspect it has something to do with the getters and setters functionality, which would require this check.

The meantime fix is simple. Just create a stub on the object like thisobject.myMethod = sinon.stub().

[joseSantacruz]

When you are working with typescript that fix doesn't work because when I tried to put object.myMethod = sinon.stub I get a typescript error Cannot assign to 'myMethod' because it is a constant or a read-only property. this happens because all the methods inside an import variable are considered as read-only

[fatso83]

@joseSantacruz That's a limitation of Typescript. When I worked with TS I still wrote all the tests in js to make life simpler :-) You can still "legally" stub and spy on the prototype's method in the meantime. Or just use the last stable 2.x release 😬

If you have time you can help us track down the regression using the method mentioned above.

[fatso83]

@mroderick I used @AccaliaDeElementia's test case and found the commit that introduced the regression:

The test case

const sinon = require('sinon')

class TestObject {
    someFunction() {
        console.error('sinon should stub me!');
    }
}

const sandbox = sinon.sandbox.create();

const instance = new TestObject();
const stub = sandbox.stub(instance, 'someFunction'); // This fails on Sinon 3.0, it worked on Sinon 2.x

running node test.js
74263dd9c5154587e1436d1b742af6d0137bd399 is the first bad commit
commit 74263dd9c5154587e1436d1b742af6d0137bd399
Author: Morgan Roderick <[email protected]>
Date:   Tue Jul 11 21:00:03 2017 +0200

    Remove deprecated
    
    * sandbox.stub(obj, 'meth', val)
    * sinon.stub(obj, 'meth', fn)
    * sinon/util/core

It seems something happened in that commit that did more than just remove deprecated functionality. It also means this is affecting 2.4.1, not just 3.0.

[fatso83]

Ah, just found out that this is purely affecting the sandbox stubbing functionality. The original report talks about sinon.stub(), but that functionality works fine. This is exactly why we ask for a reproducible snippet of code, both the test case and the linked failing test suite deals strictly with sandboxes. I will update the original issue report to reflect this (replacing sinon.stub with sandbox.stub).

[fatso83]

I have a fix for this coming up. The proposed fix Object.getOwnPropertyNames(Object.getPrototypeOf(object)).indexOf(property) > -1 does not work on the general case, as it will only list properties on the prototype, not on the object itself. It also breaks other tests. All we care about is if the property exists somewhere on the prototype chain - either on the prototype or on the object itself. Thus a simple property in object should be sufficient.

[fatso83]

Released as Sinon 3.1 on NPM.

[christopher-francisco]

please tell me this was fixed for sinon 2.4.* 😭

[mroderick]

please tell me this was fixed for sinon 2.4.* 😭

@chris-fran are you unable to upgrade?