fix: drop getters and setters when diffing objects for error by SimenB · Pull Request #9757 · jestjs/jest

SimenB · 2020-04-02T15:59:14Z

Summary

Fixes #9745. Not sure if it makes sense to drop the fix from #9575? I can check if it's writeable at the same time I now check for a setter, which makes the test added at that time pass.

/cc @WeiAnAn

Test plan

Unit test added.

SimenB · 2020-04-02T16:12:04Z

 ### Features

 ### Fixes
+


CI is failing on master due to this missing newline, snuck it in here

SimenB · 2020-04-02T16:32:16Z


-    if (!('set' in descriptor)) {
+    if ('set' in descriptor) {
+      descriptor.set = undefined;


not sure if setting to undefined or deleting it makes the most sense

gaearon · 2020-04-02T16:35:42Z

+    if ('set' in descriptor) {
+      descriptor.set = undefined;
+    } else {
      descriptor.writable = true;


This should be outside a condition? You could have a frozen object with a setter on it, right?

We cannot set writable if there's a getter or a setter. So thanks for the question, we still failed if there was a getter but no setter 🙂 fixed and added a test

Object.defineProperty({}, 'foo', {set(_){}, writable: true}) // Invalid property descriptor. Cannot both specify accessors and a value or writable attribute Object.defineProperty({}, 'foo', {get(){}, writable: true}) // Invalid property descriptor. Cannot both specify accessors and a value or writable attribute

Or did you have another example in mind?

Added some more tests. With both getter and setter, only getter and only setter, both wrapped in Object.freeze and not.

jeysal · 2020-04-02T17:03:18Z

I've tried a bit to construct another failure case with a Proxy, but we seem to be mostly safe on that side as we will never call its set, only get, ownKeys, getOwnPropertyDescriptor, getPrototypeOf (all of which things expected to be called on an object being asserted on) and then copy stuff onto something that is inherited from the prototype, but is not itself a Proxy.

codecov-io · 2020-04-02T17:27:20Z

Codecov Report

Merging #9757 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #9757   +/-   ##
=======================================
  Coverage   64.90%   64.90%           
=======================================
  Files         288      288           
  Lines       12195    12190    -5     
  Branches     3022     3020    -2     
=======================================
- Hits         7915     7912    -3     
+ Misses       3639     3638    -1     
+ Partials      641      640    -1

Impacted Files	Coverage Δ
packages/jest-matcher-utils/src/Replaceable.ts	`100.00% <100.00%> (ø)`
...est-matcher-utils/src/deepCyclicCopyReplaceable.ts	`100.00% <100.00%> (ø)`
packages/expect/src/utils.ts	`96.22% <0.00%> (+1.25%)`	⬆️

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 26951bd...5f72f4e. Read the comment docs.

Sign in to view

      });
      Object.getOwnPropertySymbols(this.object).forEach(key => {
-        const descriptor = Object.getOwnPropertyDescriptor(this.object, key);
-        if ((descriptor as PropertyDescriptor).enumerable) {


SimenB · 2020-04-02T17:47:12Z

+      configurable: true,
+      enumerable: true,
+      value: deepCyclicCopyReplaceable(
+        (object as Record<string, unknown>)[key],


without casting

not sure if it's solvable?

gaearon · 2020-04-02T17:51:01Z

Hmm. I must be doing something wrong. I checked out your PR and removed all the implementation changes, but the tests still pass. What am I doing wrong?

I have both yarn watch and yarn test --watch running.

SimenB · 2020-04-02T18:13:05Z

@gaearon my bad, I simplified the tests so much that they didn't actually fail on master anymore 😬

Should be fixed now. I also deleted a test that verified getters weren't called (as we explicitly want that now, as per your suggestion)

gaearon · 2020-04-02T18:14:22Z

-    }
-
-    descriptor.configurable = true;
+    descriptors[key] = {


Can the enumerable check be here instead?

if (descriptors[key].enumerable) { descriptors[key] = ... } else { delete descriptors[key]; }

in that case we lose Symbol keys. Not sure why...

At least if I understood your comment correcly

diff --git i/packages/jest-matcher-utils/src/Replaceable.ts w/packages/jest-matcher-utils/src/Replaceable.ts index fc8c7343c..dd9e04f32 100644 --- i/packages/jest-matcher-utils/src/Replaceable.ts +++ w/packages/jest-matcher-utils/src/Replaceable.ts @@ -35,12 +35,6 @@ export default class Replaceable { Object.entries(this.object).forEach(([key, value]) => { cb(value, key, this.object); }); - Object.getOwnPropertySymbols(this.object).forEach(key => { - const descriptor = Object.getOwnPropertyDescriptor(this.object, key); - if (descriptor?.enumerable) { - cb(this.object[key], key, this.object); - } - }); } else { this.object.forEach(cb); } diff --git i/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts w/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts index ad98131dd..67271d5d8 100644 --- i/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts +++ w/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts @@ -56,15 +56,19 @@ function deepCyclicCopyObject<T>(object: T, cycles: WeakMap<any, any>): T { cycles.set(object, newObject); Object.keys(descriptors).forEach(key => { - descriptors[key] = { - configurable: true, - enumerable: true, - value: deepCyclicCopyReplaceable( - (object as Record<string, unknown>)[key], - cycles, - ), - writable: true, - }; + if (descriptors[key].enumerable) { + descriptors[key] = descriptors[key] = { + configurable: true, + enumerable: true, + value: deepCyclicCopyReplaceable( + (object as Record<string, unknown>)[key], + cycles, + ), + writable: true, + }; + } else { + delete descriptors[key]; + } }); return Object.defineProperties(newObject, descriptors);

You'd still need the Object.getOwnPropertySymbols call but you can lose the if (descriptor?.enumerable) { check because we would've not copied those over in the first place.

right! done 🙂

gaearon · 2020-04-02T18:19:26Z

I think our longer term aspiration should be that

  let i = 0;
  expect({
    get a() {
      i++;
      return i;
    },
  }).toEqual({ 
    a: 1
   });

should pass. But that's out of scope for sure. (I verified it fails on 24.)

gaearon

seems legit

SimenB · 2020-04-02T18:33:22Z

@jeysal just noticed the discussion you had going in the comments 😅 I'm not able to make the tests pass unless I use the current form of the code. Could you experiment with it? I like copying over the getter without invoking it, which the code on master does rather than just using value, even though the current code is a bit easier to read (imo).

Although I guess it doesn't really matter if we call the getter here or in pretty-format later. While not a "pure" copy, for the use case this exists it makes no difference. And we don't export this copying anywhere, so it's internal implementation details regardless

SimenB · 2020-04-02T18:37:06Z

  expect(deepCyclicCopyReplaceable(fn)).toBe(fn);
 });

-test('does not execute getters/setters, but copies them', () => {


So it's not just in the comments - we invoke the getters on purpose now, so this test doesn't make sense anymore

gaearon · 2020-04-02T18:37:37Z

I like copying over the getter without invoking it,

My concern with this is — are you going to call it on the right object later? What if the getter reads from this? Is this going to be the real thing or the copy? It needs to be the real thing or you risk breaking other subtle assumptions.

SimenB · 2020-04-02T18:47:07Z

      expect(cb.mock.calls[2]).toEqual([3, symbolKey, object]);
    });

-    test('object forEach do not iterate none enumerable symbol key', () => {


check moved

SimenB · 2020-04-02T18:51:08Z

My concern with this is — are you going to call it on the right object later?

Yeah, it's called by pretty-format when serializing later

gaearon · 2020-04-02T18:56:28Z

As long as the getter is called on the original object, seems fine to me. Let's add a test for that?

SimenB · 2020-04-03T07:38:06Z

Let's keep it as is now - I don't think copying the getter rather than calling it is observable to the user anyways

…pshots * upstream/master: (225 commits) docs: add CLA link to contributing docs (jestjs#9789) chore: roll new version of docs v25.3.0 chore: update changelog for release chore(jest-types): correct type testRegex for ProjectConfig (jestjs#9780) feat(circus): enable writing async test event handlers (jestjs#9397) feat: enable all babel syntax plugins (jestjs#9774) chore: add helper for getting Jest's config in e2e tests (jestjs#9770) feat: pass ESM options to transformers (jestjs#9597) chore: replace `any`s with `unknown`s (jestjs#9626) feat: pass ESM options to Babel (jestjs#9766) chore(website): add copy button the code blocks (jestjs#9750) chore: bump istanbul-reports for new uncovered lines design (jestjs#9758) chore: correct CHANGELOG.md (jestjs#9763) chore(jest-types): expose type `CacheKeyOptions` for `getCacheK… (jestjs#9762) docs: Fix simple typo, seperated -> separated (jestjs#9760) v25.2.7 chore: update changelog for release fix: drop getters and setters when diffing objects for error (jestjs#9757) chore(jest-types): correct return type of shouldRunTestSuite fo… (jestjs#9753) ...

github-actions · 2021-05-11T09:07:15Z

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.

fix: do not override properties with setters when diffing objects

2a33596

SimenB requested review from jeysal and thymikee April 2, 2020 15:59

facebook-github-bot added the cla signed label Apr 2, 2020

SimenB mentioned this pull request Apr 2, 2020

Jest 25 invokes setters during toEqual comparison, causing side effects #9745

Closed

link to PR

4dcb197

SimenB commented Apr 2, 2020

View reviewed changes

remove setters rather than ignoring properties that have them

395abcd

SimenB commented Apr 2, 2020

View reviewed changes

gaearon reviewed Apr 2, 2020

View reviewed changes

only set writable if not getter and no setter

3503d77

add some frozen and not example with getters and setters

b14a485

gaearon reviewed Apr 2, 2020

View reviewed changes

Comment thread packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts Outdated

SimenB added 3 commits April 2, 2020 19:30

use hasOwnProperty

cf56846

just create a new descriptor

8f5824c

simplify replaceable

3aed077

SimenB commented Apr 2, 2020

View reviewed changes

SimenB added 2 commits April 2, 2020 20:03

write tests that actually fail on master

9ff198f

remove faulty test and re-add enumerable check

5f72f4e

gaearon reviewed Apr 2, 2020

View reviewed changes

gaearon approved these changes Apr 2, 2020

View reviewed changes

SimenB commented Apr 2, 2020

View reviewed changes

SimenB added 2 commits April 2, 2020 20:45

move enumarable check

30495ca

add test for getter -> value change

fe41787

SimenB commented Apr 2, 2020

View reviewed changes

test enumerable skip

1aea89b

update changelog entry

c0edc89

SimenB changed the title ~~fix: do not override properties with setters when diffing objects~~ fix: drop getters and setters when diffing objects for error Apr 3, 2020

SimenB merged commit 3554623 into jestjs:master Apr 3, 2020

SimenB deleted the skip-setters branch April 3, 2020 07:38

This was referenced Apr 26, 2020

[Snyk] Upgrade jest from 25.2.6 to 25.2.7 mkai/express-absolute-url#7

Merged

[Snyk] Upgrade: jest, jest-cli lwojcik/sc2pte-panel-frontend#118

Closed

[Snyk] Upgrade jest from 25.2.7 to 25.3.0 mkai/express-absolute-url#10

Merged

snyk-bot mentioned this pull request May 4, 2020

[Snyk] Upgrade: jest, jest-cli lwojcik/sc2pte-panel-frontend#123

Closed

github-actions Bot locked as resolved and limited conversation to collaborators May 11, 2021

Uh oh!

Conversation

SimenB commented Apr 2, 2020

Summary

Test plan

Uh oh!

SimenB Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeysal commented Apr 2, 2020

Uh oh!

Uh oh!

codecov-io commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimenB commented Apr 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Apr 2, 2020

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

SimenB commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Apr 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB commented Apr 2, 2020

Uh oh!

gaearon commented Apr 2, 2020

Uh oh!

SimenB commented Apr 3, 2020

Uh oh!

github-actions Bot commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SimenB Apr 2, 2020 •

edited

Loading

codecov-io commented Apr 2, 2020 •

edited

Loading

gaearon commented Apr 2, 2020 •

edited

Loading

SimenB Apr 2, 2020 •

edited

Loading

SimenB commented Apr 2, 2020 •

edited

Loading