Fix diffing object contain readonly symbol key object #10414

WeiAnAn · 2020-08-16T10:50:09Z

Summary

The reason is forget to copy and overwrite symbol key object descriptor in deepCyclicCopyObject.

I refactor deepCyclicCopyObject to use same logic in both string key and symbol key.

Test plan

Add symbol key object test and readonly property test in deepCyclicCopyReplaceable.test.ts.
Add object contain readonly symbol key object test in printDiffOrStringify.test.

should also overwrite symbol key descriptors

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

CHANGELOG.md

SimenB

thanks!

WeiAnAn · 2020-08-17T01:21:56Z

All updated.
Please check it again, thanks!

SimenB · 2020-08-17T05:44:52Z

@WeiAnAn I'm still getting an error with the original reported error

/** @jest-environment jsdom */

test('boom', () => {
  expect(document.createElement('div')).toBe(document.createElement('div'));
});

The error has changed, tho

    PrettyFormatPluginError: Illegal invocationTypeError: Illegal invocation

      at HTMLDivElement.get nodeType [as nodeType] (node_modules/jsdom/lib/jsdom/living/generated/Node.js:460:15)
      at testNode (packages/pretty-format/build/plugins/DOMElement.js:26:10)
      at Object.test (packages/pretty-format/build/plugins/DOMElement.js:51:35)
      at findPlugin (packages/pretty-format/build/index.js:347:22)
      at prettyFormat (packages/pretty-format/build/index.js:527:22)
      at compareObjects (packages/jest-diff/build/index.js:229:48)
      at diff (packages/jest-diff/build/index.js:177:14)
      at printDiffOrStringify (packages/jest-matcher-utils/build/index.js:401:46)
      at message (packages/expect/build/matchers.js:99:56)
      at getMessage (packages/expect/build/index.js:173:15)

Something about the replaced getter isn't working properly it seems

WeiAnAn · 2020-08-17T13:33:53Z

@SimenB I found that it cause by we drop all nonenumerable property when copying.
JSDOM check the symbol which is nonenumerable, if not found, will throw Illegal invocationTypeError.
I think we should copy and not iterate them instead of droping them.

SimenB · 2020-08-17T13:35:38Z

Sounds good!

WeiAnAn · 2020-08-17T17:40:39Z

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

+  } else if (plugins.DOMElement.test(value)) {
+    //@ts-expect-error skip casting to Node
+    return value.cloneNode(true);


I need a suggestion.
I don't think use @ts-expect-error to ignore Property 'cloneNode' does not exist on type 'T' is a good idea.

WeiAnAn · 2020-08-17T17:48:31Z

I met a problem to do DOM element tests, because the testEnvironment is not jsdom.

Add e2e to test is a way, but how to do in the unit test.

SimenB · 2020-08-18T07:14:42Z

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

@@ -42,6 +44,9 @@ export default function deepCyclicCopyReplaceable<T>(
    return deepCyclicCopyMap(value, cycles);
  } else if (isBuiltInObject(value)) {
    return value;
+  } else if (plugins.DOMElement.test(value)) {


Suggested change

} else if (plugins.DOMElement.test(value)) {

} else if (typeof value.cloneNode === 'function') {

maybe? not sure if it's better or not

WeiAnAn · 2020-08-19T14:59:26Z

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

@@ -42,6 +44,9 @@ export default function deepCyclicCopyReplaceable<T>(
    return deepCyclicCopyMap(value, cycles);
  } else if (isBuiltInObject(value)) {
    return value;
+  } else if (plugins.DOMElement.test(value)) {
+    //@ts-expect-error skip casting to Node
+    return value.cloneNode(true);


Suggested change

return value.cloneNode(true);

return (((value as unknown) as Element).cloneNode(true) as unknown) as T;

Can fix error with this, but a little bit ugly.

Comment override is fine

SimenB · 2020-08-19T16:41:36Z

As for the unit test, you can use a code comment: https://github.com/facebook/jest/blob/200adc053a40b1dae27d6304d24a605785c6b468/packages/pretty-format/src/__tests__/DOMElement.test.ts#L7

Just create a separate test file

SimenB

this is great, thank you!

WeiAnAn · 2020-08-19T18:08:11Z

@SimenB, thank you too, you help me alot.

In particular, this gets us Jest 26.4.1, which contains jestjs/jest#10414. This should fix an issue [1] we'd otherwise encounter very soon, where differences in a URL object encountered with `expect(...).toBe(...)` or `expect(...).toEqual(...)` would result in a very cryptic error message instead of a straightforward test failure with the desired explanation. This upgrade was pretty easy to do! I'm hoping fb23341 has increased the ease of staying up-to-date with Jest. Since we're going from a 22.x to 23.x of eslint-plugin-jest, we check the declared breaking changes at 23.0.0 [2]. The only one that concerns us is that part of the job of `jest/valid-describe` has been transferred to `jest/valid-title`, and `jest/valid-title` has been added to the `jest/recommended` config, which we take. The validation in question is silly, and unfortunately, we can't reasonably take all of `jest/valid-title` except that part, so, turn it all off. See also some prep work for this upgrade in a recent commit. [1] jestjs/jest#10408 [2] https://github.com/jest-community/eslint-plugin-jest/releases/tag/v23.0.0

jest update motivated by jestjs/jest#10414 -- unhelpful message instead of object diff in failed tests (eg when using immer)

… failed tests (eg when using immer)

github-actions · 2021-05-11T04:06:59Z

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.

WeiAnAn added 2 commits August 16, 2020 18:20

refactor deepCyclicCopyObject

8ae2068

should also overwrite symbol key descriptors

add object contain readonly symbol key object test

58af88e

facebook-github-bot added the cla signed label Aug 16, 2020

WeiAnAn added 2 commits August 16, 2020 18:53

update wrong test position

95eb84c

update CHANGELOG

b9fcdf6

SimenB reviewed Aug 16, 2020

View reviewed changes

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

SimenB reviewed Aug 16, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

SimenB approved these changes Aug 16, 2020

View reviewed changes

WeiAnAn added 2 commits August 17, 2020 09:20

add a link to the typescript issue

04e47ad

sort changelog alphabetical

930e198

WeiAnAn added 3 commits August 18, 2020 01:30

shuold not skips non-enumerables property

b9436a1

use node.cloneNode(true) to copy DOM node

f8e4885

skip nonenumerable property when calling forEach

8656b70

WeiAnAn commented Aug 17, 2020

View reviewed changes

SimenB reviewed Aug 18, 2020

View reviewed changes

WeiAnAn commented Aug 19, 2020

View reviewed changes

WeiAnAn added 3 commits August 20, 2020 00:33

remove comment

c524e02

add domDiffing e2e test

df80075

replace output time

48c5dca

WeiAnAn added 3 commits August 20, 2020 01:17

add eslint-env and copyright header

b90b04f

add deepCyclicCopyReplaceableDom test

a4208a5

update snapshot

6ccc83b

SimenB approved these changes Aug 19, 2020

View reviewed changes

SimenB merged commit 2e30f52 into jestjs:master Aug 19, 2020

IanLondon added a commit to Opentrons/opentrons that referenced this pull request Nov 24, 2020

chore(js): update jest to 26.6.3

b7b8837

jest update motivated by jestjs/jest#10414 -- unhelpful message instead of object diff in failed tests (eg when using immer)

IanLondon mentioned this pull request Nov 24, 2020

chore(js): update jest to 26.6.3 Opentrons/opentrons#7050

Merged

IanLondon added a commit to Opentrons/opentrons that referenced this pull request Nov 24, 2020

update jest to fix jestjs/jest#10414 unhelpful object diff message in…

39e1fa8

… failed tests (eg when using immer)

This was referenced Dec 17, 2020

[Snyk] Security upgrade @jest/reporters from 25.5.1 to 26.4.1 meghasfdc/jest#44

Open

[Snyk] Security upgrade @jest/reporters from 25.5.1 to 26.4.1 tjenkinson/jest#55

Open

chrisbobbe mentioned this pull request May 7, 2021

unreads: Don't add messages with "read" flag to unreads. zulip/zulip-mobile#4710

Merged

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 diffing object contain readonly symbol key object #10414

Fix diffing object contain readonly symbol key object #10414

WeiAnAn commented Aug 16, 2020

SimenB left a comment

WeiAnAn commented Aug 17, 2020

SimenB commented Aug 17, 2020

WeiAnAn commented Aug 17, 2020 •

edited

Loading

SimenB commented Aug 17, 2020

WeiAnAn Aug 17, 2020

WeiAnAn commented Aug 17, 2020

SimenB Aug 18, 2020

WeiAnAn Aug 19, 2020

SimenB Aug 19, 2020

SimenB commented Aug 19, 2020

SimenB left a comment

WeiAnAn commented Aug 19, 2020

github-actions bot commented May 11, 2021

	} else if (plugins.DOMElement.test(value)) {
	} else if (typeof value.cloneNode === 'function') {

	return value.cloneNode(true);
	return (((value as unknown) as Element).cloneNode(true) as unknown) as T;

Fix diffing object contain readonly symbol key object #10414

Fix diffing object contain readonly symbol key object #10414

Conversation

WeiAnAn commented Aug 16, 2020

Summary

Test plan

SimenB left a comment

Choose a reason for hiding this comment

WeiAnAn commented Aug 17, 2020

SimenB commented Aug 17, 2020

WeiAnAn commented Aug 17, 2020 • edited Loading

SimenB commented Aug 17, 2020

WeiAnAn Aug 17, 2020

Choose a reason for hiding this comment

WeiAnAn commented Aug 17, 2020

SimenB Aug 18, 2020

Choose a reason for hiding this comment

WeiAnAn Aug 19, 2020

Choose a reason for hiding this comment

SimenB Aug 19, 2020

Choose a reason for hiding this comment

SimenB commented Aug 19, 2020

SimenB left a comment

Choose a reason for hiding this comment

WeiAnAn commented Aug 19, 2020

github-actions bot commented May 11, 2021

WeiAnAn commented Aug 17, 2020 •

edited

Loading