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

Stubbing, and restoring do not work with es6 class instances #878

Closed
erin-noe-payne opened this issue Oct 14, 2015 · 31 comments
Closed

Stubbing, and restoring do not work with es6 class instances #878

erin-noe-payne opened this issue Oct 14, 2015 · 31 comments

Comments

@erin-noe-payne
Copy link

Environment:

  • Node 4.2.1
  • Sinon 1.15.4
  • Writing natively supported es6 syntax, no transpilers.

Test Case:

'use strict';
const sinon = require('sinon');

class C {
  foo(){ }
}

const c = new C();

const sandbox = sinon.sandbox.create();

sandbox.stub(c);
sandbox.restore();
sandbox.stub(c);
sandbox.restore();

Expected result:

No errors, code executes and exists cleanly.

Actual results:

TypeError: Attempted to wrap foo which is already wrapped
    at checkWrappedMethod (/Users/erin.noe-payne/local/lr-zmq/node_modules/sinon/lib/sinon/util/core.js:81:29)
    at Object.wrapMethod (/Users/erin.noe-payne/local/lr-zmq/node_modules/sinon/lib/sinon/util/core.js:121:21)
    at stub (/Users/erin.noe-payne/local/lr-zmq/node_modules/sinon/lib/sinon/stub.js:67:26)
    at /Users/erin.noe-payne/local/lr-zmq/node_modules/sinon/lib/sinon/stub.js:60:25
    at /Users/erin.noe-payne/local/lr-zmq/node_modules/sinon/lib/sinon/walk.js:26:26
    at Array.forEach (native)
    at walkInternal (/Users/erin.noe-payne/local/lr-zmq/node_modules/sinon/lib/sinon/walk.js:23:45)
    at Object.walk (/Users/erin.noe-payne/local/lr-zmq/node_modules/sinon/lib/sinon/walk.js:46:20)
    at Object.stub (/Users/erin.noe-payne/local/lr-zmq/node_modules/sinon/lib/sinon/stub.js:52:23)
    at Object.stub (/Users/erin.noe-payne/local/lr-zmq/node_modules/sinon/lib/sinon/collection.js:106:49)

More info:

  • Manually stubbing and restoring each function does work: sinon.stub(c, 'foo'); c.foo.restore();
  • Using sinon.stub and sinon.restore instead of sandboxes does not work - it fails with the same issue, I assume with the same root cause
@mantoni
Copy link
Member

mantoni commented Oct 14, 2015

Please upgrade to the latest Sinon 1.17 where this should work.

@mantoni mantoni closed this as completed Oct 14, 2015
@erin-noe-payne
Copy link
Author

It does not work in 1.17. I'm actually using 1.17.1 - I misread my package.json.

You can run the provided code in the latest sinon and observe the issue.

@mantoni mantoni reopened this Oct 14, 2015
@mantoni
Copy link
Member

mantoni commented Oct 16, 2015

I noticed that #870 did not make it into a release yet. We need to cut 1.17.2 to ship this fix. @mroderick Can I ask you to do that release? I still didn't look into how the release process works now 😬

@mroderick
Copy link
Member

Sure, I can look into that this weekend

@mroderick
Copy link
Member

[email protected] is now published to npm

@autoric would you mind testing with that version?

@erin-noe-payne
Copy link
Author

Will integrate and test today. Thanks a lot for the support!

@erin-noe-payne
Copy link
Author

@mroderick I see no change with the issue in my production code.

Further, running the simple test case provided for this issue continues to throw the same error. Did you see it run successfully?

@nomadtechie
Copy link

Same here....same issue for me as well with es6.

@glortho
Copy link

glortho commented Dec 1, 2015

+1

@fatso83
Copy link
Contributor

fatso83 commented Dec 2, 2015

For everyone experiencing this you should help us out and try our pre-release of Sinon 2. We regard it as quite ready, but there might be things we haven't been able to see yet that's lurking beneath the surface. Try it out like this:

npm install sinon@next

... and use the issue tracker for any issues you come across. Should fix this and quite a few other issues.

@glortho
Copy link

glortho commented Dec 2, 2015

Great will do @fatso83 . I take it existing API will mostly apply in next. If not do you have v2 docs somewhere?

@fatso83
Copy link
Contributor

fatso83 commented Dec 2, 2015

All current development on master is for version 2. Version 1 is in maintenance mode. The API will be mostly kept, except for a few breaking changes documented in the issue tracker (don't remember the issue on the top of my head).

@fatso83
Copy link
Contributor

fatso83 commented Feb 10, 2016

@mantoni We have no fix for this in the master and v1.17 branches: the bug is still there, so this must be something else than #867/#868. I pasted the test script above into 878-test.js and ran node 878-test.js. Got the exact same error as in the case description.

@symposion
Copy link

Hey, Just ran into this issue. Could perhaps be related to the fact that ES6 class methods are not enumerable and thus don't show up in for...in etc?

@rafa-munoz
Copy link

For version 2.0.0 it's possible to stub ES2016 classes. Here there is a post about how to make it:
https://eirikardal.no/stubbing-es2015-classes-with-sinon/

@fatso83
Copy link
Contributor

fatso83 commented Jun 2, 2016

@Menda You are mistaking this bug for something else :-) This post is about stubbing instances of a class, not the class itself. Stubbing ES6 classes has been covered extensively before (see #831 and #1035). So for latecomers wanting to know about that, please read and understand this thread on a similar issue. It might shed some light on what you are trying to do.

The thread basically says the same as the article, which by the way has nothing to do with Sinon 2.0. Spying and stubbing ES6 classes is equally possible (and impossible - depending on what you want to do) with Sinon 1.x. Read my comment if you want a concise summary of how to do various things in ES6, including examples with code.

@rafa-munoz
Copy link

rafa-munoz commented Jun 2, 2016

Thanks @fatso83. I totally mixed things up, my bad!

@franvarney
Copy link

I just want to confirm v2/next solves the "Attempted to wrap.." error for me!

@davidtzoor
Copy link

davidtzoor commented Sep 28, 2016

@fatso83 I didn't understand if there is a way to stub an ES6 class method, and still having the same issue

@fatso83
Copy link
Contributor

fatso83 commented Sep 28, 2016

@davidtzoor There is nothing special about an ES6 class compared to an ES5 class. It's just syntactic sugar. You still have the prototype object. Stub instance methods any way you would with an ES5 class.

class FooClass{
  static classMethod(){}
  instanceMethod(){}
}

is nothing but this

function FooClass(){}
FooClass.classMethod = function(){}
FooClass.prototype.instanceMethod = function(){}

So if you want to stub class methods just stub methods on the class object and not on the instances:

const stub = sinon.stub(FooClass, 'classMethod').returns('mystub');

Regarding Sinon 2 this is not officially out, but is usable as is. If you had read this thread you would have seen me comment on this and how to get the pre-release 😃

@JoeChapman
Copy link

I couldn't get that to work

@fatso83 fatso83 closed this as completed Oct 6, 2016
@fatso83
Copy link
Contributor

fatso83 commented Oct 6, 2016

Closed by mistake, sorry.

@fatso83 fatso83 reopened this Oct 6, 2016
@lucasfcosta
Copy link
Member

lucasfcosta commented Oct 6, 2016

I just tested this at the master branch with the exact same example described on the first post and the test passed. I'm not sure if I'm doing this on the wrong branch or anything like that, but this is the code I ran:

it.only("should work with ES6 classes", function () {
    class C {
      foo(){ }
    }

    const c = new C();

    const sandbox = sinonSandbox.create();

    sandbox.stub(c);
    sandbox.restore();
    sandbox.stub(c);
    sandbox.restore();
});

And it seems to be running without problems.

@fatso83
Copy link
Contributor

fatso83 commented Oct 6, 2016

@lucasfcosta, thank you for taking the time to check that out! Closing this as fixed.

@fatso83 fatso83 closed this as completed Oct 6, 2016
@fatso83
Copy link
Contributor

fatso83 commented Oct 6, 2016

@JoeChapman, try the mailing list or the Gitter chat room.

@davidtzoor
Copy link

@lucasfcosta did you also test stubbing a method of an existing class instance?

@fatso83
Copy link
Contributor

fatso83 commented Oct 6, 2016

@davidtzoor I know you asked how to do that earlier, so are the steps outlined failing to work somehow?

@davidtzoor
Copy link

davidtzoor commented Oct 6, 2016

I am actually on a vacation, so can't check right now 😃 was wondering if
you already checked that. Will do next week, thanks!

@fatso83
Copy link
Contributor

fatso83 commented Oct 7, 2016

It's working. Stubbing a function property is the most basic use case for Sinon. Calling it a static class method does not change anything about the mechanics :-)

Edit: too damn easy to post comments by mistake on my phone..

@fatso83 fatso83 reopened this Oct 7, 2016
@coolnalu
Copy link

Encountered this issue today. The exact code works for all tests except in one file. driving me nuts. lol. will try to create a test case to reproduce.

@fatso83 fatso83 closed this as completed Oct 21, 2016
@fatso83
Copy link
Contributor

fatso83 commented Oct 21, 2016

Think I mistakenly reopened this without noticing when commenting a few weeks ago.

@sinonjs sinonjs locked and limited conversation to collaborators Oct 25, 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.
Projects
None yet
Development

No branches or pull requests