Jest does not restore mocks on methods inherited from a prototype properly

[fongandrew]

🐛 Bug Report

When spying on an object or class, it is possible to mock a method that's defined higher up on the prototype chain. But when the original implementation is restored, the parent object's method is assigned to the child's.

To Reproduce

describe("Spying on protoypes", () => {
  const A = { test: () => 1 };

  const B = Object.create(A);

  afterEach(() => {
    jest.restoreAllMocks();
  });

  it("works with B", () => {
    jest.spyOn(B, "test").mockReturnValue(3);
    expect(B.test()).toEqual(3);
  });

  it("works with A", () => {
    jest.spyOn(A, "test").mockReturnValue(2);
    expect(B.test()).toEqual(2); // Fails, equals 1
  });
});

The second test case above fails with the first test case is present, but passes if the first test case does not exist. If you console log / debugger this, you'll see that the first test is adding a new method to B's prototype, and restoreAllMocks "restored" the mock from the first test by assigning A.test to B.test.

Expected behavior

Mock restoration should work regardless of whether the method is defined higher up on the prototype chain or not.

If B.hasOwnProperty('test') is false, when jest.spyOn(B, 'test') is restored, Jest should just delete B.test rather than assign a value that might have existed only on B's prototype.

Link to repl or repo

Example above ☝️but here's a minimum setup with the latest version of Jest if needed: https://github.com/fongandrew/jest-proto-repo. Also includes a repro with an ES6 class inheritance example.

npx envinfo --preset jest

System:
    OS: macOS 10.14.5
    CPU: (12) x64 Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz
  Binaries:
    Node: 10.1.0 - ~/.nvm/versions/node/v10.1.0/bin/node
    npm: 6.0.1 - ~/.nvm/versions/node/v10.1.0/bin/npm
  npmPackages:
    jest: ^24.8.0 => 24.8.0 

[lucasfcosta]

I have just confirmed this is easily reproducible in the current version of master. The problem seems to be the restoreAllMocks which happens in between tests.

To confirm I've written the following test in jest-mock:

    it('supports mocking value in the prototype chain', () => {
      const parent = {func: () => 'abcd'};
      const child = Object.create(parent);

      moduleMocker.spyOn(parent, 'func').mockReturnValue('jklm');

      expect(child.func()).toEqual('jklm'); // works just fine
    });

The test above passes ✅.

When adding a restoreAllMocks in between mockReturnValue calls we get 'abcd' again.

    it('supports mocking value in the prototype chain after restoration', () => {
      const parent = {func: () => 'abcd'};
      const child = Object.create(parent);

      moduleMocker.spyOn(child, 'func').mockReturnValue('efgh');
      moduleMocker.restoreAllMocks();
      moduleMocker.spyOn(parent, 'func').mockReturnValue('jklm');

      expect(child.func()).toEqual('jklm'); // is equal 'abcd'
    });

(btw @fongandrew provided an amazingly good report 💖 )

As per the description, the problem happens due to these lines.

Here we do an assigment to object[methodName], which will end-up assigning to the object itself. In the restore function (the callback passed as the second argument) we reassign to that very same property: object[methodName] which will end-up causing the "child" to have the methodName property on itself. Due to this, when calling object[methodName] again we won't reach up to mocked function in the prototype but instead use the methodName property in the "child" itself.

One could argue that the correct behaviour here would be to mock the property in the prototype itself since accessing methodName would end-up reaching the prototype anyway. Making restoreAllMocks delete the property on the children might seem like patching unexpected behaviour with unexpected behaviour. On the other hand, mocking the function straight on the prototype may cause all sorts of weird problems since one might not expect that since the rest of the api does mocks in the obj itself and may also cause weird side-effects.

For the sake of comparison with similar libraries, sinonjs's current version seems to currently do the same (stub the property in the "child") for stubs.

const sinon = require('sinon')
const a = { val: () => 1 }
const b = Object.create(a);
sinon.stub(b, 'val').returns(2);
b.val(); // 2
a.val(); // 1

---

Implementation

As I went into investigating this anyway (and since it was a quick fix) I created a branch which contains the necessary changes.

However, since @fongandrew has already brilliantly done the hardest part of this which is uncovering the bug, providing reproducibility steps and even a reproducible repo I don't want to open a PR if he wants to give it a try himself.

In case we need to discuss possible implementations or desired behaviour, here's a link to the commit with this fix.

If everyone's happy with it I can open a PR with the commit above.

[fongandrew]

Thanks! I'm happy to defer to @lucasfcosta on the actual fix. I'm not super familiar with Jest's implementation.

I'm curious why we need to crawl up the object's prototype chain in the proposed fix though (https://github.com/lucasfcosta/jest/blob/ba1885ac06a06f53ab18f13abefbd9157037cbc9/packages/jest-mock/src/index.ts#L1012-L1015). In my mind, I though it'd just be something like this:

const isMethodOwner = object.hasOwnProperty(methodName);

And the restore would just delete if it was untrue (the prototype still "owns" the original, so subsequent invocations would still go to the prototype in the absence of anything assigned to the child object).

if (isMethodOwner) {
  object[methodName] = original;
} else {
  delete object[methodName];
}

[lucasfcosta]

@fongandrew that's definitely true. I over-engineered this.

Crawling up the prototype chain was not necessary as the only reason why we obtain methodOwner is to check whether the object was the previous owner of the method. We can do this in a single line with your suggestion.

I guess that's a relic from the previous attempt I've had at stubbing the method up in the prototype (which I then moved back to stubbing the method always at "child" due to the last paragraph in my previous comment).

You are absolutely right in this.

I'm gonna update that branch accordingly when I get home today. Btw, by all means please feel free to open the PR before if you want to, you've done most of the hard work 💖