Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mock reassigning to previously unexisting method #8631

Conversation

@lucasfcosta
Copy link
Contributor

lucasfcosta commented Jul 2, 2019

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 馃槅).

@lucasfcosta lucasfcosta force-pushed the lucasfcosta:fix-mock-reassigning-to-previously-unexisting-method branch 2 times, most recently from 1a08a06 to fab1852 Jul 2, 2019
@jeysal
jeysal approved these changes Jul 7, 2019
Copy link
Collaborator

jeysal left a comment

Awesome @lucasfcosta, what an awesome first contribution on a nicely isolated bug :)

@jeysal jeysal requested review from SimenB and thymikee Jul 7, 2019
@lucasfcosta lucasfcosta force-pushed the lucasfcosta:fix-mock-reassigning-to-previously-unexisting-method branch from fab1852 to 10f63b9 Jul 7, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 7, 2019

Codecov Report

Merging #8631 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8631      +/-   ##
==========================================
- Coverage   63.45%   63.43%   -0.03%     
==========================================
  Files         274      274              
  Lines       11382    11388       +6     
  Branches     2770     2772       +2     
==========================================
+ Hits         7223     7224       +1     
- Misses       3546     3547       +1     
- Partials      613      617       +4
Impacted Files Coverage 螖
packages/jest-mock/src/index.ts 77.71% <100%> (+0.19%) 猬嗭笍
packages/expect/src/spyMatchers.ts 89.02% <0%> (-2%) 猬囷笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 517235a...1ad9864. Read the comment docs.

@lucasfcosta lucasfcosta force-pushed the lucasfcosta:fix-mock-reassigning-to-previously-unexisting-method branch 2 times, most recently from 9d346c4 to 5c68cff Jul 7, 2019
@lucasfcosta

This comment has been minimized.

Copy link
Contributor Author

lucasfcosta commented Jul 10, 2019

The Azure task in the pipeline seems to have failed due to network issues in their connection.

2019-07-09T07:55:42.5576070Z 锟絒2K锟絒1G锟絒94minfo锟絒39m There appears to be trouble with your network connection. Retrying...
2019-07-09T07:57:18.7374809Z 锟絒2K锟絒1G锟絒94minfo锟絒39m There appears to be trouble with your network connection. Retrying...
2019-07-09T07:57:51.9756877Z 锟絒2K锟絒1G锟絒94minfo锟絒39m There appears to be trouble with your network connection. Retrying...
2019-07-09T07:58:25.1406368Z 锟絒2K锟絒1G锟絒94minfo锟絒39m There appears to be trouble with your network connection. Retrying...
2019-07-09T07:58:33.1502461Z 锟絒2K锟絒1G锟絒94minfo锟絒39m There appears to be trouble with your network connection. Retrying...
2019-07-09T07:58:58.3002599Z 锟絒2K锟絒1G锟絒94minfo锟絒39m There appears to be trouble with your network connection. Retrying...
2019-07-09T07:59:32.8300249Z 锟絒2K锟絒1G锟絒31merror锟絒39m An unexpected error occurred: "https://registry.yarnpkg.com/rxjs/-/rxjs-6.4.0.tgz: ESOCKETTIMEDOUT".

CC @jeysal are you able to rerun it? I couldn't rerun the build myself.

(No need to rush btw, just pinging you because I've just noticed the build there has failed for no apparent reason)

@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Jul 10, 2019

On Azure, I actually don't think I can - just do git commit --amend --no-edit && git push -f to rerun it :)

@lucasfcosta lucasfcosta force-pushed the lucasfcosta:fix-mock-reassigning-to-previously-unexisting-method branch from 5c68cff to 1ad9864 Jul 11, 2019
@lucasfcosta

This comment has been minimized.

Copy link
Contributor Author

lucasfcosta commented Jul 11, 2019

@jeysal ah that makes sense. It seems like --no-edit did the trick. I had tried --allow-empty before but it didn't rerun the builds. Thanks, just learned something new 馃槃

@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Jul 11, 2019

Actually, --no-edit just avoids opening the editor to edit the message. Any --amend will update the commit date (not author date) and thus create a different commit :)

@jeysal jeysal merged commit b76f01b into facebook:master Jul 11, 2019
11 checks passed
11 checks passed
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190711.5 succeeded
Details
@lucasfcosta lucasfcosta deleted the lucasfcosta:fix-mock-reassigning-to-previously-unexisting-method branch Jul 11, 2019
aliaksandr-yermalayeu added a commit to aliaksandr-yermalayeu/jest that referenced this pull request Jul 16, 2019
This was referenced Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.