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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check _isMockFunction is true rather than truthy on global properties #7017

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

danrr
Copy link

@danrr danrr commented Sep 21, 2018

Fixes #7009

Summary

resetModules checks if _isMockFunction is truthy on all properties of the global object, which can lead to TypeErrors when a global property is mocked using identity-obj-proxy. Changing the condition to check if _isMockFunction is true rather than truthy will fix the issue and keep the original intent of the code, so it shouldn't cause any breaking changes.

Test plan

Tests all ran OK.

@codecov-io
Copy link

codecov-io commented Sep 21, 2018

Codecov Report

Merging #7017 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7017      +/-   ##
==========================================
+ Coverage   66.95%   66.95%   +<.01%     
==========================================
  Files         250      250              
  Lines       10430    10428       -2     
  Branches        4        4              
==========================================
- Hits         6983     6982       -1     
+ Misses       3446     3445       -1     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-editor-support/src/Runner.js 74% <0%> (-0.26%) ⬇️
...est-cli/src/getNoTestFoundRelatedToChangedFiles.js 0% <0%> (ø) ⬆️

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 37caedd...328b16a. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Sep 21, 2018

Mind fixing the ones in https://github.com/facebook/jest/blob/0418cab88c5e927c94a02ecb445fc694e6b2bbda/packages/jest-mock/src/index.js#L729-L731 as well?

Also, an integration test would be sweet 🙂

@danrr
Copy link
Author

danrr commented Sep 23, 2018

@SimenB thanks for having a look. I've updated the other instances of _isMockFunction being checked for truthiness but I'm not quite sure where best to have any new tests live.

@SimenB
Copy link
Member

SimenB commented Sep 23, 2018

You can add an e2e test that uses identity-obj-proxy without blowing up 🙂

Look in https://github.com/facebook/jest/tree/master/e2e - all directories are "projects" which you can run with ../../../jest locally, and that we run - and assert on the output from, from __tests__ in https://github.com/facebook/jest/tree/master/e2e, where there usually is a test named similarly to a directory in the e2e directory.

@danrr danrr force-pushed the strict-mock-check branch 2 times, most recently from df27bd3 to 213dbd8 Compare September 28, 2018 09:23
@danrr
Copy link
Author

danrr commented Sep 28, 2018

@SimenB Thanks, I've added an e2e test and rebased

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@SimenB SimenB merged commit f8efff9 into jestjs:master Sep 28, 2018
@danrr danrr deleted the strict-mock-check branch September 28, 2018 12:16
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jest.resetModules() can fail when using identity-obj-proxy
5 participants