Summary

Previously, when restoring a stubbed method that didn't exist in the object passed to spyOn we would always reassign to object[methodName] the previous result of object[methodName].

Since object[methodName] causes the prototype chain to be traversed if we don't find methodName in object itself, this could cause us to assign the method that was previously in the prototype to a method in object which did not exist before.

This PR fixes #8625.

Also, @fongandrew actually deserves a lot of credit for this. He's done most of the hard work by providing an accurate report detailing what the expected behaviour should be and by helping out to get to the optimal solution. Thanks @fongandrew 💖

The Problem in Detail

You can reproduce this bug with the following snippet.

    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'
    });

The test above would previously fail because restoreAllMocks would cause us to reassign the func we originally got from the parent to the child. Then, when calling func in the child after restoring it, the result would come from the child itself instead of reaching up to the func in parent.

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

Implementation Choices

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 strange side-effects. Mocking methods in the "child" itself also complies with the Principle of Least Astonishment.

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

Test plan

I have tested this with the exact piece of code which I used to demonstrate the bug was present. I also added an extra assertion to that test to prove that the func property also does not exist in the child anymore after restoration.

To ensure that we would have precise feedback in tests, I added a separate test-case to ensure that the method is mocked in the child itself and not in its parent.

I think these two tests are sufficient.

PS: The previous CI failure is because I had removed what my Mac thought was an "unused directive" because fsevents was available, but that is in fact necessary in CI as per this comment since it does not use a Mac (which therefore causes the directive to be valid 😆).