Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sandbox.stub fails to stub methods on the prototype #1512

Closed
RaceProUK opened this issue Aug 3, 2017 · 12 comments
Closed

sandbox.stub fails to stub methods on the prototype #1512

RaceProUK opened this issue Aug 3, 2017 · 12 comments

Comments

@RaceProUK
Copy link

RaceProUK commented Aug 3, 2017

  • 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
Copy link
Contributor

fatso83 commented Aug 3, 2017

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
Copy link
Author

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
Copy link

Minimal reproduction of the issue described:

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

@fatso83
Copy link
Contributor

fatso83 commented Aug 4, 2017

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
Copy link

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
Copy link
Contributor

fatso83 commented Aug 5, 2017

@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 fatso83 changed the title [3.0.0] Stub reports existent ES6 functions as non-existent properties Stub reports existent ES6 functions as non-existent properties Aug 7, 2017
@fatso83
Copy link
Contributor

fatso83 commented Aug 7, 2017

@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
Copy link
Contributor

fatso83 commented Aug 7, 2017

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 fatso83 changed the title Stub reports existent ES6 functions as non-existent properties sanddbox.stub reports methods on the prototype as non-existent properties Aug 7, 2017
@fatso83 fatso83 changed the title sanddbox.stub reports methods on the prototype as non-existent properties sandbox.stub reports methods on the prototype as non-existent properties Aug 7, 2017
@fatso83 fatso83 changed the title sandbox.stub reports methods on the prototype as non-existent properties sandbox.stub fails to stub methods on the protoytpe Aug 7, 2017
@fatso83 fatso83 changed the title sandbox.stub fails to stub methods on the protoytpe sandbox.stub fails to stub methods on the prototype Aug 7, 2017
@fatso83
Copy link
Contributor

fatso83 commented Aug 7, 2017

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
Copy link
Contributor

fatso83 commented Aug 8, 2017

Released as Sinon 3.1 on NPM.

@christopher-francisco
Copy link

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

@mroderick
Copy link
Member

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

@chris-fran are you unable to upgrade?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants