fix: drop getters and setters when diffing objects for error #9757

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

CHANGELOG.md

@@ -3,6 +3,8 @@
 ### Features

 ### Fixes
+


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

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

packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts

@@ -59,7 +59,9 @@ function deepCyclicCopyObject<T>(object: T, cycles: WeakMap<any, any>): T {
      descriptor.value = deepCyclicCopyReplaceable(descriptor.value, cycles);
    }

-    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

packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts

-    if (!('set' in descriptor)) {
+    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.

packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts

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.

packages/jest-matcher-utils/src/Replaceable.ts

@@ -36,10 +36,7 @@ export default class Replaceable {
        cb(value, key, this.object);
      });
      Object.getOwnPropertySymbols(this.object).forEach(key => {
-        const descriptor = Object.getOwnPropertyDescriptor(this.object, key);
-        if ((descriptor as PropertyDescriptor).enumerable) {


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

packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts

+      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

packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts

-    }
-
-    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

packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts

@@ -20,20 +20,6 @@ test('returns the same value for primitive or function values', () => {
  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

packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts

      expect(cb.mock.calls[0]).toEqual([1, 'a', object]);
      expect(cb.mock.calls[1]).toEqual([2, 'b', object]);
      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 thymikee and jeysal 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

packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: drop getters and setters when diffing objects for error #9757

fix: drop getters and setters when diffing objects for error #9757

SimenB commented Apr 2, 2020

SimenB Apr 2, 2020 •

edited

SimenB Apr 2, 2020

gaearon Apr 2, 2020

SimenB Apr 2, 2020

SimenB Apr 2, 2020

SimenB Apr 2, 2020

jeysal commented Apr 2, 2020

codecov-io commented Apr 2, 2020 •

edited

This comment was marked as outdated.

SimenB Apr 2, 2020

gaearon commented Apr 2, 2020 •

edited

SimenB commented Apr 2, 2020

gaearon Apr 2, 2020

SimenB Apr 2, 2020 •

edited

SimenB Apr 2, 2020

gaearon Apr 2, 2020

SimenB Apr 2, 2020

gaearon commented Apr 2, 2020

gaearon left a comment

SimenB commented Apr 2, 2020 •

edited

SimenB Apr 2, 2020

gaearon commented Apr 2, 2020

SimenB Apr 2, 2020

SimenB commented Apr 2, 2020

gaearon commented Apr 2, 2020

SimenB commented Apr 3, 2020

github-actions bot commented May 11, 2021

fix: drop getters and setters when diffing objects for error #9757

fix: drop getters and setters when diffing objects for error #9757

Conversation

SimenB commented Apr 2, 2020

Summary

Test plan

SimenB Apr 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeysal commented Apr 2, 2020

codecov-io commented Apr 2, 2020 • edited

Codecov Report

This comment was marked as outdated.

Choose a reason for hiding this comment

gaearon commented Apr 2, 2020 • edited

SimenB commented Apr 2, 2020

Choose a reason for hiding this comment

SimenB Apr 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Apr 2, 2020

gaearon left a comment

Choose a reason for hiding this comment

SimenB commented Apr 2, 2020 • edited

Choose a reason for hiding this comment

gaearon commented Apr 2, 2020

Choose a reason for hiding this comment

SimenB commented Apr 2, 2020

gaearon commented Apr 2, 2020

SimenB commented Apr 3, 2020

github-actions bot commented May 11, 2021

SimenB Apr 2, 2020 •

edited

codecov-io commented Apr 2, 2020 •

edited

gaearon commented Apr 2, 2020 •

edited

SimenB Apr 2, 2020 •

edited

SimenB commented Apr 2, 2020 •

edited