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

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

Merged
merged 14 commits into from
Apr 3, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Apr 2, 2020

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.

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

### Fixes

Copy link
Member Author

@SimenB SimenB Apr 2, 2020

Choose a reason for hiding this comment

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

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

@@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

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

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

if (!('set' in descriptor)) {
if ('set' in descriptor) {
descriptor.set = undefined;
} else {
descriptor.writable = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Or did you have another example in mind?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jeysal
Copy link
Contributor

jeysal commented Apr 2, 2020

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
Copy link

codecov-io commented Apr 2, 2020

Codecov Report

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

Impacted file tree graph

@@           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.

@@ -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) {

This comment was marked as outdated.

configurable: true,
enumerable: true,
value: deepCyclicCopyReplaceable(
(object as Record<string, unknown>)[key],
Copy link
Member Author

Choose a reason for hiding this comment

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

without casting
image

not sure if it's solvable?

@gaearon
Copy link
Contributor

gaearon commented Apr 2, 2020

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
Copy link
Member Author

SimenB commented Apr 2, 2020

@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)

}

descriptor.configurable = true;
descriptors[key] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the enumerable check be here instead?

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

Copy link
Member Author

@SimenB SimenB Apr 2, 2020

Choose a reason for hiding this comment

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

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

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);

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

right! done 🙂

@gaearon
Copy link
Contributor

gaearon commented Apr 2, 2020

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

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

seems legit

@SimenB
Copy link
Member Author

SimenB commented Apr 2, 2020

@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

@@ -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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@gaearon
Copy link
Contributor

gaearon commented Apr 2, 2020

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.

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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

check moved

@SimenB
Copy link
Member Author

SimenB commented Apr 2, 2020

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
Copy link
Contributor

gaearon commented Apr 2, 2020

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

@SimenB
Copy link
Member Author

SimenB commented Apr 3, 2020

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

@SimenB 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 SimenB merged commit 3554623 into jestjs:master Apr 3, 2020
@SimenB SimenB deleted the skip-setters branch April 3, 2020 07:38
jeysal added a commit to mmkal/jest that referenced this pull request Apr 10, 2020
…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
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 11, 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 25 invokes setters during toEqual comparison, causing side effects
5 participants