fix: spyOn should not rely on hasOwnProperty from the spied object #11721

cexbrayat · 2021-08-03T07:17:41Z

Summary

The spyOn function uses hasOwnProperty from the spied object, but this method can be unavailable,
for example if the object has the null prototype, or if it is a proxy that filters some keys.

This arises in Vue 3 projects where the proxies returned by the framework do not expose all methods,
and forces the testing library to manually patch the proxies with hasOwnProperty to let Jest do its work
https://github.com/vuejs/vue-test-utils-next/blob/23d3d3e1f4178a87de5023f5255e0623653affdc/src/mount.ts#L493-L495

Test plan

This commit changes the code to use Object.prototype.hasOwnProperty and fixes this issue.
A unit test has been added to reproduce the issue.

facebook-github-bot · 2021-08-03T07:17:45Z

Hi @cexbrayat!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

codecov-commenter · 2021-08-03T07:36:59Z

Codecov Report

Merging #11721 (d614bb1) into master (fdc74af) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #11721   +/-   ##
=======================================
  Coverage   69.01%   69.01%           
=======================================
  Files         312      312           
  Lines       16335    16335           
  Branches     4734     4734           
=======================================
  Hits        11273    11273           
  Misses       5034     5034           
  Partials       28       28

Impacted Files	Coverage Δ
packages/jest-mock/src/index.ts	`84.98% <100.00%> (ø)`

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 fdc74af...d614bb1. Read the comment docs.

facebook-github-bot · 2021-08-03T07:58:15Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

sigveio

Test case demonstrates the problem well, and borrowing hasOwnProperty from the Object prototype is good practise in this kind of context. 👍 Especially since hasOwnProperty is not protected, meaning objects could even have their own implementation that produces unpredictable results.

Only thing I see missing is a changelog entry for the fix

cexbrayat · 2021-08-03T18:24:38Z

@sigveio Thanks for the feedback.
I added an entry to the changelog. Let me know if anything else is needed.

Smrtnyk · 2021-08-03T21:49:49Z

This is definitely a good improvement

Smrtnyk · 2021-08-03T21:52:32Z

packages/jest-mock/src/__tests__/index.test.ts

@@ -1216,6 +1216,18 @@ describe('moduleMocker', () => {
      expect(originalCallArguments[1]).toBe(secondArg);
      expect(spy).not.toHaveBeenCalled();
    });
+
+    it('should not rely on hasOwnProperty', () => {


The description of the test I find not the best.
It still relies on hasOwnProperty as it is calling it from the Object.prototype
I would name it as should not throw an error if spying on object without prototype or something like that

Perhaps it could be simplified to 'should work with object of null prototype'?

sounds good

The `spyOn` function uses `hasOwnProperty` from the spied object, but this method can be unavailable, for example if the object has the `null` prototype, or if it is a proxy that filters some keys. This arises in Vue 3 projects where the proxies returned by the framework do not expose all methods, and forces the testing library to manually patch the proxies with `hasOwnProperty` to let Jest do its work https://github.com/vuejs/vue-test-utils-next/blob/23d3d3e1f4178a87de5023f5255e0623653affdc/src/mount.ts#L493-L495 This commit changes the code to use `Object.prototype.hasOwnProperty` and fixes this issue.

SimenB

thanks!

…estjs#11721)

Updates jest and ts-jest to v27, as jest v27.1.0 includes a necessary fix to unblock further work on spying issues. See jestjs/jest#11721

vue-test-utils-next now uses jest v27.1+, which includes a fix for hasOwnPropery (see jestjs/jest#11721). This allows to remove the dirty workaround we had in our codebase, but users will have to update to Jest v27.1+.

Updates jest and ts-jest to v27, as jest v27.1.0 includes a necessary fix to unblock further work on spying issues. See jestjs/jest#11721

github-actions · 2021-09-16T00:04:20Z

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.

cexbrayat mentioned this pull request Aug 3, 2021

fix(runtime-core): allow calling hasOwnProperty on proxy vuejs/core#4215

Closed

facebook-github-bot added the cla signed label Aug 3, 2021

sigveio reviewed Aug 3, 2021

View reviewed changes

cexbrayat force-pushed the fix/has-own-property branch from e1f582a to a34e4b6 Compare August 3, 2021 18:24

Smrtnyk reviewed Aug 3, 2021

View reviewed changes

cexbrayat force-pushed the fix/has-own-property branch from a34e4b6 to d614bb1 Compare August 4, 2021 05:59

SimenB approved these changes Aug 16, 2021

View reviewed changes

SimenB added 2 commits August 16, 2021 10:22

Update CHANGELOG.md

87756cc

Merge branch 'master' into fix/has-own-property

c859576

SimenB merged commit f3ae13c into jestjs:master Aug 16, 2021

splincode pushed a commit to splincode/jest that referenced this pull request Aug 24, 2021

fix: spyOn should not rely on hasOwnProperty from the spied object (j…

f0a56e0

…estjs#11721)

cexbrayat added a commit to cexbrayat/vue-next that referenced this pull request Aug 30, 2021

chore(deps): update jest to v27

5e121ad

Updates jest and ts-jest to v27, as jest v27.1.0 includes a necessary fix to unblock further work on spying issues. See jestjs/jest#11721

cexbrayat mentioned this pull request Aug 30, 2021

chore(deps): update jest to v27 vuejs/core#4472

Merged

cexbrayat mentioned this pull request Aug 30, 2021

chore: update to jest 27 vuejs/test-utils#811

Merged

1 task

yyx990803 pushed a commit to vuejs/core that referenced this pull request Sep 1, 2021

chore(deps): update jest to v27 (#4472)

b49f446

Updates jest and ts-jest to v27, as jest v27.1.0 includes a necessary fix to unblock further work on spying issues. See jestjs/jest#11721

docschina-bot pushed a commit to docschina/vue-compat-docs-cn that referenced this pull request Sep 1, 2021

chore(deps): update jest to v27 (#4472)

b689ae5

Updates jest and ts-jest to v27, as jest v27.1.0 includes a necessary fix to unblock further work on spying issues. See jestjs/jest#11721

github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: spyOn should not rely on hasOwnProperty from the spied object #11721

fix: spyOn should not rely on hasOwnProperty from the spied object #11721

cexbrayat commented Aug 3, 2021

facebook-github-bot commented Aug 3, 2021

codecov-commenter commented Aug 3, 2021 •

edited

facebook-github-bot commented Aug 3, 2021

sigveio left a comment

cexbrayat commented Aug 3, 2021

Smrtnyk commented Aug 3, 2021

Smrtnyk Aug 3, 2021 •

edited

sigveio Aug 3, 2021

Smrtnyk Aug 3, 2021

cexbrayat Aug 4, 2021

SimenB left a comment

github-actions bot commented Sep 16, 2021

fix: spyOn should not rely on hasOwnProperty from the spied object #11721

fix: spyOn should not rely on hasOwnProperty from the spied object #11721

Conversation

cexbrayat commented Aug 3, 2021

Summary

Test plan

facebook-github-bot commented Aug 3, 2021

Action Required

Process

codecov-commenter commented Aug 3, 2021 • edited

Codecov Report

facebook-github-bot commented Aug 3, 2021

sigveio left a comment

Choose a reason for hiding this comment

cexbrayat commented Aug 3, 2021

Smrtnyk commented Aug 3, 2021

Smrtnyk Aug 3, 2021 • edited

Choose a reason for hiding this comment

sigveio Aug 3, 2021

Choose a reason for hiding this comment

Smrtnyk Aug 3, 2021

Choose a reason for hiding this comment

cexbrayat Aug 4, 2021

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 16, 2021

codecov-commenter commented Aug 3, 2021 •

edited

Smrtnyk Aug 3, 2021 •

edited