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

ES6 classes and stub #831

Closed
armandabric opened this issue Sep 9, 2015 · 16 comments
Closed

ES6 classes and stub #831

armandabric opened this issue Sep 9, 2015 · 16 comments
Labels
Documentation ES2015+ ES2015 and later revision

Comments

@armandabric
Copy link

I'm working on an ES6 project, and I'm trying to stub ES6 classes. How can I do this?

My class:

var sinon = require('sinon');

class Foo {
    constructor() {
        this.bar = 42;
    }

    getBar() {
        return this.bar;
    }
}

console.log(Foo);

var fooStub = sinon.stub(Foo); // error

And when I tried to test it:

$ mocha --compilers js:babel/register classes-mock.js
[Function: Foo]
/Users/spy-seth/workspace/web.client.vlr/node_modules/sinon/lib/sinon/util/core.js:115
                    throw error;
                    ^

TypeError: Attempted to wrap undefined property undefined as function
    at Object.wrapMethod (/Users/spy-seth/workspace/web.client.vlr/node_modules/sinon/lib/sinon/util/core.js:106:29)
    at Object.stub (/Users/spy-seth/workspace/web.client.vlr/node_modules/sinon/lib/sinon/stub.js:61:26)
    at Object.<anonymous> (/Users/spy-seth/workspace/web.client.vlr/classes-mock.js:15:21)
    at Module._compile (module.js:434:26)
    at normalLoader (/Users/spy-seth/workspace/web.client.vlr/node_modules/babel-core/lib/api/register/node.js:199:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/spy-seth/workspace/web.client.vlr/node_modules/babel-core/lib/api/register/node.js:216:7)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at /Users/spy-seth/.nvm/versions/node/v4.0.0/lib/node_modules/mocha/lib/mocha.js:216:27
    at Array.forEach (native)
    at Mocha.loadFiles (/Users/spy-seth/.nvm/versions/node/v4.0.0/lib/node_modules/mocha/lib/mocha.js:213:14)
    at Mocha.run (/Users/spy-seth/.nvm/versions/node/v4.0.0/lib/node_modules/mocha/lib/mocha.js:453:10)
    at Object.<anonymous> (/Users/spy-seth/.nvm/versions/node/v4.0.0/lib/node_modules/mocha/bin/_mocha:401:18)
    at Module._compile (module.js:434:26)
    at Object.Module._extensions..js (module.js:452:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:475:10)
    at startup (node.js:117:18)
    at node.js:951:3

Is there a way to stub ES6 classes?

@mroderick
Copy link
Member

We are trying to keep the GitHub issues list tidy and focused on bugs and feature discussions. This ticket looks like a usage question, please post it to the Sinon.JS mailinglist, so the bigger community can help answer your questions.

If you feel that your topic is an issue with Sinon.JS, please open a new ticket and follow the guidelines for reporting an issue.

@rstuven
Copy link

rstuven commented Oct 28, 2015

@Spy-Seth Try this:

var FooStub = sinon.spy(function() {
  return sinon.createStubInstance(Foo);
});

Then you could test it like this:

    var foo = new FooStub();
    expect(FooStub).to.have.been.calledWithNew;
    foo.getBar.returns('something');
    expect(foo.getBar()).to.equal('something');
    expect(foo.getBar).to.have.been.calledOnce;

(using chai and sinon-chai)

@armandabric
Copy link
Author

I was hoping that sinon could manage this correctly. It's conform me in the fact ES6 classes are not completely ready to be widely used: they can't be tested easily.

@shouze
Copy link

shouze commented Nov 4, 2015

Yes, this is weird... 🙀

@fatso83
Copy link
Contributor

fatso83 commented Nov 6, 2015

There is nothing weird or different about ES6 classes. ES classes are sugar coated ES5 classes and can be stubbed in the same way using sinon.createStubInstance

@shouze
Copy link

shouze commented Jan 30, 2016

ping @Spy-Seth we can close this one no? @fatso83 yes in fact with the right babel config & release it's ok for us.

@machineghost
Copy link

I'm not sure if I should file this as a separate issue (since it's a different error than what @Spy-Seth reported), or if it should be part of this issue (since it's another problem with stubbing ES6 class constructors ... so I guess I'll post it here.

When I stub a class's constructor with Sinon 1.17.3 it works ... but the stub never gets called. In other words if you do something like ...

class Foo {
    constructor() {console.log('this shouldn\'t have been called')} // gets called
}
sinon.stub(Foo.prototype, 'constructor');
new Foo();
console.log(Foo.prototype.constructor.called); // false

you will see that the constructor stub was not called ... despite the new Foo(). However "this shouldn't have been called" did get called, implying that the Foo class constructor was never actually replaced (despite the stub call going off without an error).

Based on this quote from http://www.2ality.com/2015/02/es6-classes-final.html:

Foo.prototype.constructor is non-writeable, non-enumerable, non-configurable.
Prototype methods Foo.prototype.* are writable and configurable, but not enumerable.

It appears that ES6 class constructors cannot be changed after construction, and so calling spy/stub/whatever on them is a nonsense operation. As such, I believe Sinon should throw an error when you try to stub a constructor, rather than letting the user believe that the stub worked.

P.S. All of this relies on a "real" ES6 environment (eg. nodejs run with the --harmony flag), not Babel ES6=>ES5 code.

@fatso83
Copy link
Contributor

fatso83 commented Mar 16, 2016

@machineghost: user error. That's not how javascript works. Look into what MDN has to say on the constructor property. As stated above: nothing magic about ES6 classes. Just sugar coated syntax for ES5. Test them as such. If you want to see if a function gets called, just supply a stub instead. You want to check on Foo - not Foo.constructor.

@machineghost
Copy link

EDIT

@fatso83 Did you try the example code I provided? Here's a simplified version of my example that doesn't use Sinon, which you can copy/paste directly in to the Chrome debugger to see in action:

class Foo {
    constructor() {console.log('this shouldn\'t have been called')} // gets called
}
Foo.prototype.constructor = function() {};
new Foo();
// "this shouldn't have been called" gets logged

You want to check on Foo - not Foo.constructor.

I don't quite follow; if I change the last line of the previous example code from:

console.log(Foo.prototype.constructor.called); // false

to:

console.log(Foo.called); // undefined

then Foo.called is (unsurprisingly) undefined. And I'm not aware of a way to stub the entire class (ie. sinon.stub(Foo)). Could you please clarify?

@fatso83
Copy link
Contributor

fatso83 commented Mar 16, 2016

That was perhaps a bit too brief (typed on a mobile), so I understand the confusion. I'll have another go:

Summed up:

  • This has nothing to do with ES6 classes per se, and everything to do with understanding how javascript prototypes work (since forever). It is just an unfortunate overlap in terms that confuses (here: the constructor ES6 keyword which refers to something else than the Object.prototype.constructor property). Try pasting your code into the Babel transpiler and see what it produces.
  • The constructor property references what has created an object. Setting it on Foo.prototype will not have any effect on the creation of any objects created by Foo. See this very nice article on the matter
  • Without having an idea what you are trying to test it it is hard to give general advice on how to do your testing.
  • I think what you want is to investigate the sinon.createStubInstance method. See this recent discussion.

@machineghost
Copy link

Thanks a bunch @fatso83 for that explanation, it really helped. It sounds like what I'd like to do is impossible, but for reasons that have nothing to do with ES6.

What I'm trying to accomplish is the following

 // A.js
module.exports = class A {};

// B.js
module.exports = class B {
    constructor() {
        this.a = new A();
    }
};

// B-test.js
// imagine some test code wrapping this next bit
sinon.stub(A.prototype, 'constructor');
new B();
assert(A.prototype.constructor.called);

While that sort of pattern works great with say a Backbone class (which has a separate, non-constructor initialize method), it doesn't seem like it's possible with straight JS classes.

Of course, I could get all dependency injection-y and make B's constructor take A as an argument so that I can test it ... but I think instead I'm just going to give up on doing this sort of assertion. But again, thanks a bunch for taking the time to provide that explanation.

@fatso83
Copy link
Contributor

fatso83 commented Mar 17, 2016

It is possible to achieve what you want, at least in Node, using heavier machinery like proxiquire, but I find that unnecessary. Just opening up a little bit for dependency injection is a pragmatic middle-way that does less magic IMHO, at the expense of a few lines. Doing it like you just said is something I sometimes do, as in this codebit I had lying around from a project a few years ago:

/**
 * Request proxy to intercept and cache outgoing http requests
 *
 * @param {Number} opts.maxAgeInSeconds how long a cached response should be valid before being refreshed
 * @param {Number} opts.maxStaleInSeconds how long we are willing to use a stale cache in case of failing service requests
 * @param {boolean} opts.useInMemCache default is false
 * @param {Object} opts.stubs for dependency injection in unit tests
 * @constructor
 */
function RequestCacher (opts) {
    opts = opts || {};
    this.maxAge = opts.maxAgeInSeconds || 60 * 60;
    this.maxStale = opts.maxStaleInSeconds || 0;
    this.useInMemCache = !!opts.useInMemCache;
    this.useBasicToken = !!opts.useBasicToken;
    this.useBearerToken = !!opts.useBearerToken;

    if (!opts.stubs) {
        opts.stubs = {};
    }

    this._redisCache = opts.stubs.redisCache || require('./redis-cache');
    this._externalRequest = opts.stubs.externalRequest || require('../request-helpers/external-request-handler');
    this._memCache = opts.stubs.memCache || SimpleMemCache.getSharedInstance();
}

RequestCacher.prototype = { /* methods ... */ }
module.exports = RequestCacher

By using an options argument, instead of relying on the positional arguments, it keeps flexible, but often I just export some factory methods to produce objects in various ways instead of, or (for convenience) in addition to, directly exposing the constructor. For instance, taking your own example of A and B:

module.exports = class B {
    constructor() {
        this.a = new A();
    }

    doSomething() { return a.foo(); }
};

// alternative factory method
B.createUsing = (injectedDependency) => { 
    const b = Object.create(B.prototype); // will not invoke the constructor!
    b.a = injectedDependency; 
}

// test that B is using A correctly
const aStub = { foo : sinon.stub() };
const b = B.createUsing(aStub);

b.doSomething();
assert.true(a.foo.calledOnce);

No extra magic, but a few lines extra. Granted, it still doesn't test the actual constructor, but I usually try to leave testing to code lines with a higher ROI 😏

@machineghost
Copy link

Hmmm ... well your example makes a very good case for (minimal) dependency injection, and I'm sure there's something to the idea since the Angular team embraces it so heavily. But at the same time, I have a very strong (I'd argue "healthy") aversion to changing production code purely to support test code.

So, I guess I just have to weigh my distaste for, on the one hand, making test-only changes to non-test code vs. my desire to test whether new Foo() was called, on the other. Not sure quite which side I'm going to fall on, but either way I understand things MUCH better now, so thanks again for taking the time to break it all down 👍

@markcellus
Copy link

Thanks, so I came here in search of a solution to the OP's problem, which seems to be similar to the issue I'm having. I guess this won't be a huge issue until more people start to use ES6 native classes. Since this issue is closed, I'm assuming we have to seek an alternative? Any new thoughts on this @machineghost?

@markcellus
Copy link

It is possible to achieve what you want, at least in Node, using heavier machinery like proxiquire,

@fatso83, yes I do believe you are correct. However, I've used proxyquire, and it does not let you stub out multiple instances of a class.

So for instance if I have this

// carousel.js
import Panel from 'panel';
class Carousel {
     /**
     * When the carousel is instantiated.
     * @param {HTMLCollection|NodeList} panels - The elements that will represent the panels
     */
    constructor (panels) {
       this._panelInstances = {};
       for (var i=0; i < panels.length; i++) {
          this._panelInstances['panel' + i] = new Panel(panels[i]);
       }
  }
}

Even with proxyquire, there is no way to stub the Panel's constructor to test that new Panel() was called with appropriate arguments when testing Carousel class (at least not with their latest code build), unless I'm missing something.

Ideally, I would want to do something like this...

// carousel-test.js
import Panel from 'panel';
import Carousel from 'carousel';
var firstPanelInstance = sinon.createStubInstance(Panel);
var carouselPanelConstructor = sinon.stub(Carousel.prototype, 'constructor');
var panelEl = document.getElementById("panel1");
var firstPanelInstance = carouselPanelConstructor.withArgs(panelEl).returns(firstPanelInstance);
var carousel = new Carousel([panelEl]);
assert.equal(firstPanelInstance.args[0][0], panelEl, 'first panel instance was created correctly');
// ...

@fatso83
Copy link
Contributor

fatso83 commented Apr 13, 2016

@mkay581 You are missing something. proxyquire is perfectly capable of doing what you mentioned. See below. The second issue is that your code example demonstrates a misunderstanding of what the constructor keyword in ES6 actually refers to. See the discussion above where I elaborate on this point.

I have now made this example to show an example of

  • using proxyquire
  • show how to stub out collaborating parties in the constructor
  • verify that each collaborator indeed is called with the right parameters

I am going to lock the discussion on this one from now on. For latecomers these are the main points to take to heart:

  • if you are having an issue with stubbing ES6 constructor classes, please try to write the same code using plain ES5 to understand what is going on
  • the constructor keyword in ES6 classes is (almost) just syntactic sugar (it now has a built-in guard against forgetting to call with new). You still need to actually test for invocations of the function creating the class, not its constructor property.
  • this is not some ES2015/ES6 specific thing that is missing in sinon.
  • you need some way of controlling how your collaborating classes are instantiated. This happens either using constructor injection, injection methods or proxyquire. Two out of three are demonstrated in this thread (if you count the link to my gist).

@sinonjs sinonjs locked and limited conversation to collaborators Apr 13, 2016
@fatso83 fatso83 added the ES2015+ ES2015 and later revision label Sep 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation ES2015+ ES2015 and later revision
Projects
None yet
Development

No branches or pull requests

7 participants