Using restore after createStubInstance creates frankenstein objects

[dpraul]

Describe the bug
Restoring stubs created by createStubInstance creates frankenstein objects that have the original prototype-chain of the object passed in without the object having been properly created through the constructor.

To Reproduce
Here is a jsfiddle demonstrating the following code

class TestObject {
  constructor() {
    this.constructorDidRun = true;
  }
  
  doSomething() {
    if (!this.constructorDidRun) {
      throw new Error("I am Frankenstein's monster!!")
    }
  }
}

const sandbox = sinon.createSandbox();
const stubInstance = sandbox.createStubInstance(TestObject);

stubInstance.doSomething(); // does nothing, as expected

sandbox.restore();

stubInstance.doSomething(); // uh-oh!

Expected behavior
I'm not totally certain, but perhaps nothing? For us, the use-case of createStubInstance is to create drop-in objects that simulate the provided object's interface, but without ever touching its implementation. We use them in unit-testing to validate interactions. I'm not sure the use-case where you'd want a partial object that has not had its constructor run, so I cannot imagine this is intended behavior (and even in that case, I would expect to expose that behavior using stub.callThrough(), rather than by restoring the sandbox).

Context

• Library version: 14.0.2
• Environment: Node/browser
• Example URL: https://jsfiddle.net/dpraul/4atk8jdv/2

Additional Context
This behavior came to our attention as we're updating our application from Angular 13 to Angular 14. Angular 14 updates the default TestBed behavior to tear down the test module after each test. While this is arguably a good addition as it provides a cleaner testing environment for each test, it has brought these frankenstein objects to our attention.

Since a sandox is created and restored inside of a test-suite, but the TestBed teardown occurs after the suite, the TestBed still holds references to these partial objects. The TestBed cleanup triggers any OnDestroy methods on the objects, which may now have implementations associated with them.

NOTE: This particular issue may be better-solved a multitude of ways (perhaps by introducing the TestBed teardown into the afterEach of the suite and prior to restoring the sandbox). Nonetheless, it seems like an issue that the implementations of these stubbed objects are being reintroduced back to the test suite, even after the suite has completed.

Here is an example of this issue in our application

import * as sinon from "sinon";
import { of } from "rxjs";
import { Injectable, OnDestroy } from "@angular/core";
import { TestBed } from "@angular/core/testing";

@Injectable({ providedIn: "root" })
class Dependency implements OnDestroy {
  private subscription = of(null).subscribe();

  ngOnDestroy() {
    // It's common to use the OnDestroy lifecycle hook to clean-up any RxJS subscriptions.
    // The sandbox is restored in afterEach, which re-introduces this implementation,
    // but since the constructor has never been called this.subscription will be undefined, so a TypeError is thrown
    this.subscription.unsubscribe();
  }
}

@Injectable({ providedIn: "root" })
class ServiceUnderTest {
  constructor(private dependency: Dependency) {}

  doSomething(): boolean {
    return true;
  }
}

describe("Test Suite", () => {
  let service: ServiceUnderTest;
  let sandbox: sinon.SinonSandbox;
  let mockDependency: sinon.SinonStubbedInstance<Dependency>;

  beforeEach(() => {
    sandbox = sinon.createSandbox();
    mockDependency = sandbox.createStubInstance(Dependency);

    TestBed.configureTestingModule({
      providers: [
        ServiceUnderTest,
        { provide: Dependency, useValue: mockDependency }
      ]
    });
    service = TestBed.inject(ServiceUnderTest);
  });

  afterEach(() => {
    sandbox.restore();
  });

  it("should do something", () => {
    expect(service.doSomething()).toBe(true);
  });
});

[fatso83]

Sorry for not having looked at this before, but after reading through this, I just want to note that this is an exemplary bug report. I think I understand the issue, but I need to have a look at the createStubInstance() implementation to be more certain.

Props on the test suite btw; I haven't seen testing code for Angular before and I could totally understand it from the context, so very clean and understandable.

[fatso83]

OK, I think I have something that should have this work as you would expect:

 
-    var stubbedObject = stub(Object.create(constructor.prototype));
+    // eslint-disable-next-line no-empty-function
+    const noop = () => {};
+    const defaultNoOpInstance = Object.create(constructor.prototype);
+    walkObject((obj, prop) => (obj[prop] = noop), defaultNoOpInstance);
+
+    const stubbedObject = stub(defaultNoOpInstance);

This creates default no-op implementations of all the functions on the prototype, which will then be stubbed and subsequently restored. This should be the least surprising behavior, IMHO and according to your expectations?

Given the following:

$ cat test/test-stub-instance.js 
"use strict";
const sinon = require("sinon");

class SystemUnderTest {
    constructor() {
        this.privateGetter = () => 42;
    }
    getValue() {
        return this.privateGetter();
    }
}

it("behavior of restored instance", function () {
    const instance = sinon.createStubInstance(SystemUnderTest, {
        getValue: "ooh, override of getValue",
    });

    console.log(instance.getValue());
    sinon.restore();
    console.log(instance.getValue()); // Existing behavior: TypeError: this.privateGetter is not a function
});

it will now run without failing, whereas previously it would throw after restoring.

Before

$ npx mocha test/test-stub-instance.js 


ooh, override of getValue
  1) behavior of restored instance

  0 passing (4ms)
  1 failing

  1) behavior of restored instance:
     TypeError: this.privateGetter is not a function
      at SystemUnderTest.getValue (test/test-stub-instance.js:9:21)
      at Context.<anonymous> (test/test-stub-instance.js:20:26)
      at process.processImmediate (node:internal/timers:471:21)

After

$ npx mocha test/test-stub-instance.js 


ooh, override of getValue
undefined
  ✔ behavior of restored instance

  1 passing (5ms)

[fatso83]

ping @dpraul

Is this what you want? Pushed the changes: https://github.com/sinonjs/sinon/compare/main...fatso83:sinon:issue-2477?expand=1

[dpraul]

Thanks for the ping - I accidentally let this issue fall off my radar this week. And thanks as well for your kind words :)

Your suggested behavior of restoring the stub to all no-ops makes sense to me. I patched your changes into our test suite and it ran without issue, and when I toggled on the Angular TestBed "automatic tear-down" it ran through without getting tripped up on the restored instances. Thanks for looking into this, looks great!

[fatso83]

@mroderick OK with the changes? Not sure if this should be regarded as a breaking change or just a patch. The behavior of a restored stub instance has up until now not been defined in any docs, so it does not change any documented behavior.

[fatso83]

fix published as sinon 15.0.2

[fatso83]

As I feared, this came back and bit us, unfortunately, as it makes callThrough impossible, now that it calls through to the no-op: #2501

Seems I might need to revert this 😢 Any ideas on how to get the best of both worlds without vastly complicating things? Right now, almost no logic, including cleanup, is specific to createStubInstance, and I would like to keep it that way, as far as possible. A stub does not know if it has been attached to a stub instance or anything, really.

[fatso83]

The fix to the bug I introduced by this is essentially moving the setting of the no-op fallback to after the restore. You should not see any functional changes in your own code as a result of this. The fix is present in 15.0.3