sandbox.restore() does not work on static members of a function or class

[ghdoergeloh]

Describe the bug
The documentation says, that we can stub all methods of a object by using sinon.stub(obj)
https://sinonjs.org/releases/latest/stubs/#var-stub--sinonstubobj

This works great for objects and also for classes.

It also works for sandbox.stub(obj)

What doesn't work, is to restore the methods of a class by using sandbox.restore()

To Reproduce

This block works:

const sinon = require('sinon');
const sandbox = sinon.createSandbox();

console.log('stub(obj)')
let i = 1;

const myTest = {
  test: (value) => {
    return value;
  }
}

console.log(myTest.test(i++));
sandbox.stub(myTest);
console.log(myTest.test(i++));
sandbox.restore();
console.log(myTest.test(i++));
sandbox.stub(myTest);
console.log(myTest.test(i++));
sandbox.restore();
console.log(myTest.test(i++));

The output is:

stub(obj)
1
undefined
3
undefined
5

This one doesn't:

const sinon = require('sinon');
const sandbox = sinon.createSandbox();

console.log('stub(class)')
let i = 1;

class MyTest {
  static test(value) {
    return value;
  }
}

console.log(MyTest.test(i++));
sandbox.stub(MyTest);
console.log(MyTest.test(i++));
sandbox.restore();
console.log(MyTest.test(i++));
sandbox.stub(MyTest);
console.log(MyTest.test(i++));
sandbox.restore();
console.log(MyTest.test(i++));

The output is:

stub(class)
1
undefined
undefined
./node_modules/sinon/lib/sinon/util/core/wrap-method.js:82
            throw error;
            ^

TypeError: Attempted to wrap test which is already wrapped
...

Expected behavior
It would be great if sandbox.restore() would also restore all methods of a class and not only of a object.

Context (please complete the following information):

• Library version: 11.1.1
• Environment: Linux - node v14.16.0

[ghdoergeloh]

By the way: it is working with MyTest.prototype.

[fatso83]

A static function is per def not part of that class. It is just a utility function tacked onto the class.

class MyTest {}
MyTest.someStatic = function() { ... } 

this has nothing to do with the prototype nor its instances, so I am not really sure we are looking at a bug here.

A method, like a function, is a set of instructions that perform a task. The difference is that a method is associated with an object, while a function is not.

A static member of a class has no relationship with that class, apart from just being located there.

(I did just browse very quickly through this, so maybe I read it wrong)

---

EDIT: yes, I did read it wrong. I see I missed some details when skimming through it quickly. I somehow mixed this up a bit with createStubInstance.

[fatso83]

OK, I have verified this using the example you posted. This does indeed look like a bug. Not sure why we treat a class object differently than a normal Object, but might be related to us thinking it's a function and not doing all the right things.

[fatso83]

Function does not work either, as expected:

const sinon = require('sinon')
const sandbox = sinon.createSandbox()

function MyTest() {}
MyTest.test = function (value) {
  return value
}

sandbox.stub(MyTest)
sandbox.restore()
sandbox.stub(MyTest) // --> TypeError: Attempted to wrap test which is already wrapped

[fatso83]

this was not so straight forward after all ... we have different paths for objects and functions, so it got a bit hairy when I was looking at it. I'll continue another day when I have had some sleep :)

A quickfix is to use sinon.restoreObject() on your class:

//sandbox.restore();
sinon.restoreObject(MyTest);

[ghdoergeloh]

Thanks for your support, @fatso83 . For now I have a different workaround. The method you mentioned should not be used, as said in the docs:

Sinon.JS has a few utilities used internally in lib/sinon.js. Unless the method in question is documented here, it should not be considered part of the public API, and thus is subject to change.

And therefore it is also not supported by @types/sinon (which we use for typescript development).

My workaround is to create an object for the stubbed Class by myself:

let stubbedClass: SinonStubbedInstance<typeof MyClass>;

stubbedClass = {
  doSomething: sandbox.stub(MyClass, 'doSomething'),
  prototype: {},
};

when this issue is fixed i can replace it easily by sandbox.stub(MyClass)

[fatso83]

@ghdoergeloh That same page documents exactly that method (second from the top), and it is both part of the exposed public API of the sinon export and listed on the utilities page, so you are free to use it. The DT typings package is often out of date or is missing bits, and I have patched it in the past (it is not our package), and you could supply a patch too 😃

Your temporary fix is just as good, of course :-)

[ghdoergeloh]

Sorry, I totally misunderstood the sentence at the top of that page. (translation problems)

[fatso83]

I suspected as much after reading it myself, but feel free to improve it to make it less ambiguous: https://github.com/sinonjs/sinon/blob/master/docs/release-source/release/utils.md

[marceliwac]

I'm glad that there's some workarounds, thanks for providing them!

On a separate note, is this issue still considered a bug and therefore planned to be fixed at some point in the future?

[fatso83]

Hi, @marceliwac. Thanks for bumping this. I have forgotten all about it to be honest. When I looked at it now, I just saw the last part of the conversation, and thought it meant that whatever the issue was, it was fixed, but it is indeed a bug in the sandbox cleanup (as verified) and we should try and fix it.

I removed myself as an assignee as I am a bit too caught up in other projects at the moment and I see that I was unable to fix this quickly the last time.

cc @mroderick

[marceliwac]

A static function is per def not part of that class. It is just a utility function tacked onto the class.

class MyTest {}
MyTest.someStatic = function() { ... } 

this has nothing to do with the prototype nor its instances, so I am not really sure we are looking at a bug here.

A method, like a function, is a set of instructions that perform a task. The difference is that a method is associated with an object, while a function is not.

A static member of a class has no relationship with that class, apart from just being located there.

(I did just browse very quickly through this, so maybe I read it wrong)

EDIT: yes, I did read it wrong. I see I missed some details when skimming through it quickly. I somehow mixed this up a bit with createStubInstance.

One more quick question regarding the above (might be separate issue but this conversation seems relevant). I'm having trouble spying on static properties of a class using the following:

(Node 14 LTS)

const sinon = require('sinon');
const assert = require('assert');

class C {
  static f(x) {
    return x+1;
  }
}

const sandbox = sinon.createSandbox();
sandbox.spy(C);
C.f(1);
assert(C.f.calledWith(1));

The above code results in the following:

assert(C.f.calledWith(1));
           ^

TypeError: C.f.calledWith is not a function
    at Object.<anonymous> (/Users/marceli/Desktop/temp/index.js:13:12)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)
    at internal/main/run_main_module.js:17:47

Does this have to do with the fact that static properties are somehow not bound to the class object or is this the problem we're discussing here as well (or a separate issue)?

For reference, the following works fine:

const sinon = require('sinon');
const assert = require('assert');

class C {
  static f(x) {
    return x+1;
  }
}

const sandbox = sinon.createSandbox();
sandbox.spy(C, 'f');
C.f(1);
assert(C.f.calledWith(1));

[fatso83]

@marceliwac There are two sides to your issue

1. Why the error happens
2. If this should be handled or not

The why

When one does sinon.spy(myFunc) this is creating a new function that wraps myFunc. Sinon's code is equivalent to:

/// spy.js
function createSpy(func){
  function calledWith(...args){
    for(let i = 0; i < this.calls; i++) {
      if( deepEquals(this.arguments[i], args) ) return true;
    }
    return false;
  }
  return function spy(...args) { 
    spy.calls = spy.called? spy.calls + 1 : 1; 
    spy.arguments = spy.called? spy.arguments.push(args) : [args];
    spy.called = true;
    spy.calledWith = 
    return func(...args); 
  }
}
/// end spy.js

(I just wrote this "blindfolded" now for pedagogical reasons, so it might not run without errors 😅)

That means that if one did this:

function myFunction(){ 
}
myFunction.staticMember = function(){}

const stubbedMyFunction = createSpy(myFunction); 

You will now have created a wrapper stubbedMyFunction around myFunction, but you will not have copied any fields on that function. That means

assert.isUndefined(stubbedMyFunction.staticMember); // true

Relationship to classes

Since the class syntax keyword is mostly syntactic sugaring around the old way of specifying prototypal inheritance using normal function definitions it is subject to the same restrictions, which is easier to see if we wrote the C class using ES5 syntax:

function C {}
C.f = function(x) {
    return x+1;
}

It's just like the plain jane function above, so any field member will not be copied.

Bug or feature?

This logic has been like this since Sinon's beginnings. One could argue that static class members are not related to the class at all, but merely a static utility function that should be handled standalone. On the other hand, it might break the expectations that tings should be auto-nice, just handling this for you. That would be a breaking change, but maybe one we want?

That being said, it might not be super-easy to get right, seeing that fields come in all kinds of flavours, but should be doable. We would essentially need to loop over all the property descriptors (which we do already for objects, if my memory serves me right).

If anyone would have a go at this, it's not hard, just a bit hairy with all the different paths 😄 I would suggest starting out with a test of what you want to end up with and then modify the library code until it passes.

#help-wanted

[marceliwac]

@fatso83 Thanks for this comprehensive reply! The logic in your answer is sound and my problem comes down to my lack of understanding of ES6 Classes, which I thought would treated as objects and not functions i.e.:

class C {
  static f(x){
    return x + 1;
  }
}

typeof C === 'function'  // true

The reason why this was unexpected to me is this example from the documentation which operates on a normal object (could be instance of a class), which - as you said - does in fact loop over the property descriptors of the supplied object. In my mind, the ES6 class with its static fields would work just as well.

I think it supporting the ES6 classes and their static properties in the same way objects are handled could be a useful feature to have and would be fairly achievable if there was an easy way to check whether an object is a class.

Regardless of that, though, would you say it makes sense to clarify this distinction in documentation or is this generally obvious?

[fatso83]

I don't see why we should restrict ourselves to classes, per se. I think such a feature should work on any function as well, not just the subset that are constructor functions in ES2015+. Doing that, the need to check for serialized representations disappear and it will work just as well with code that has been transpiled down to ES5.1 (which does not have the class keyword).

The documentation could clearly need some clarification, so if you would like to fill in some details or a new section on stubbing constructors, feel free to do so. It's just markdown files in the same repo under /docs

[marceliwac]

Sounds great! I'll get on it at some point soon!

[alejandro-venegas]

I think this is releated, but sinon.reset() isn't working on statics either.

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.