Using createStubInstance plus method.restore returns undefined

[ej-mark]

The change to createStubInstance added in 15.0.2 (#2499) causes issues when restoring the individual methods. A method does not restore properly and causes undefined to be returned. An example is provided below.

import sinon from 'sinon';
class Tester {
    a() {
      return 'str';
    }
  }
  describe('test', () => {
    const dummy = sinon.createStubInstance(Tester);
    dummy.a.restore();
    it('should be equal', () => {
      assert.equal('str', dummy.a());
    });
  });

[fatso83]

Please fill out the template. It's not certain what your issue is. A stub instance is not guaranteed in the docs to be equal to an unstubbed instance, if so, please point me to the docs.

[ej-mark]

Describe the bug A clear and concise description of what the bug is.
In 15.0.1 and previously, when using
const stubInstance = sinon.createStubInstance(MyConstructor)
then using
stubInstance.method.restore(),
the functions previous functionality would be returned. Now, after calling restore(), undefined is returned.

To Reproduce Steps to reproduce the behavior:

import sinon from 'sinon';
class Tester {
    a() {
      return 'str';
    }
  }
  describe('test', () => {
    const dummy = sinon.createStubInstance(Tester);
    dummy.a.restore();
    it('should be equal', () => {
      assert.equal('str', dummy.a());
    });
  });

Expected behavior A clear and concise description of what you expected to happen.
The original methods functionality to be restored.
In the example, 'str' should be returned.
If my understanding is incorrect, please let me know.

Context (please complete the following information):

Library version: 15.0.4
Additional context Add any other context about the problem here.
This example is from the doc if it helps
https://sinonjs.org/releases/v15/stubs/
"If you want to create a stub object of MyConstructor, but don’t want the constructor to be invoked, use this utility function.

var stub = sinon.createStubInstance(MyConstructor, overrides);
"

[fatso83]

OK, so the original description was a bit misleading. I'll update it as it is very confusing and points to #2491 which has nothing to do with this. Rather, the comment you copied is about the change introduced in #2499.

The real issue here is that the behavior you rely on is undocumented. It is not clear how a stubbed instance should behave. That is also why we could change it without introducing a new major version. It seems as if you rely on using that object after restoring it, which is a bit peculiar (not sure what the use case is).

There are two approaches here:

1. Revert the feature introduced in Use no-op for every function when restoring instances #2499 . This will ensure the behavior of the restored stubs is in line with that of doing sinon.stub(someObject), where doing a .restore() on each method of someObject would restore the original methods. Whether or not it should be expected that this is the way createStubInstance should work to begin with is up for debate. In any case, we would then need to figure out an alternative way of doing Use no-op for every function when restoring instances #2499.
2. Just update the docs of createStubInstance to say that if restoring the stubs the result is an object with methods that simply return undefined. This is to avoid the problem of Using restore after createStubInstance creates frankenstein objects #2477: "Fankenstein objects that have the original prototype-chain of the object passed in without the object having been properly created through the constructor"

I am leaning towards the second.

Thoughts @mroderick @fearphage @mantoni ?

[mantoni]

If I had to choose between option 1 and option 2, I'd opt for option 2. However, I can think of a third option:

#2499 was implemented to fix #2477, to avoid that a stub instance becomes an object that actually executes functions from the prototype chain.

Instead of replacing the functions with a no-op, we could set the properties to a function that throws a meaningful error. This would make the object unusable when attempting to invoke a function on a stub instance after restore().

Not sure this option is desirable, but at least it would make it more obvious what happened.

[fatso83]

@mantoni This was added for the cases when you are finished with the object, but the test framework or whatever still traverses the created objects, triggering some method calls on the stubbed instance. See #2477 for details.

Since the problem was that the default would end up throwing, this was more of a silencing feature. Maybe it should be an optional flag for the few that needs it? This feature still is not documented, so could still change it ...

[fearphage]

I'm leaning towards the second solution, but perhaps we should go further? I think calling restore on methods of a stubbed instance should throw. What you're attempting to do seems illogical and the behavior is unpredictable. We should prevent this behavior from "failing" silently.

[fatso83]

@fearphage People often have a sandbox.restore() in a afterEach() in many test suites to automatically cleanup. This will call each stub's restore method. I don't think we should break that common usecase, even if re-using a stub instance makes very little sense.

[fearphage]

This will call each stub's restore method.

Are you sure that sandbox.restore() is calling restore on each stubbed method on a stub instance? That seems like a bug in and of itself. That seems like wasted cycles/effort.

[fatso83]

When you create a stub it is added to an internal collection in the current sandbox. When you call sandbox.restore() it will iterate through that list and call the restore() method on each stub.

But yeah, maybe that is another take at the problem I originally tried addressing in #2477 : simply avoid doing doing the restore altogether on stub instances. That would avoid the issues @dpraul encountered in his Angular test suite rig by simply not touching them. We could alternatively release a new major version that will throw on trying to manually restore the stub instances, since it has little meaning.

After looking at the code (related to this) I do suspect it will not turn out very clean. The current approach is simple to reason about at least. If stub, it will be restored with the sandbox restoration. I do not regard wasted cycles as a bug, especially in such uncommonly used bits of the API.

[dpraul]

Chiming in since I was pinged: looking at this in terms of "how I expect this to work" (regardless of what the docs say):

• I expect a stubbed instance to always have methods that do nothing, because by-design the constructor has never been called. That's why I filed my original issue - it seemed like a mistake that it would call through to the implementation after being restored, and if I were writing code that depended on that behavior I would view it as a code smell.
• Since I expect the stubbed instance to never do anything, doing nothing to a stub when a sandbox is restored could make sense from an optimizations perspective.
• That said, when I call sandbox.restore(), I do expect everything in the sandbox to be restored. So if restoring the stub does do something, I would expect it to occur when I restore the sandbox.

---

All that said, @ej-mark looking at your example, perhaps what you're looking for is callThrough?

import sinon from 'sinon';
class Tester {
    a() {
      return 'str';
    }
  }
  describe('test', () => {
    const dummy = sinon.createStubInstance(Tester);
    dummy.a.callThrough();
    it('should be equal', () => {
      assert.equal('str', dummy.a());
    });
  });

[ej-mark]

@dpraul Thank you for providing a workaround for us.

The tests that have been implemented using createStubInstance more than likely need to be rewritten since they really do the opposite of how to test. I can say that the reason they were implemented using createStubInstance was due to being legacy, complex classes with complex constructors. "Restoring" only the necessary functions was a way to begin creating a test suite when new features were added.

[fatso83]

Closing as working as intended.