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

Conversation

WeiAnAn
Copy link
Contributor

@WeiAnAn WeiAnAn commented Aug 16, 2020

Summary

Fix #14018.

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.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@WeiAnAn
Copy link
Contributor Author

WeiAnAn commented Aug 17, 2020

All updated.
Please check it again, thanks!

@SimenB
Copy link
Member

SimenB commented Aug 17, 2020

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

WeiAnAn commented Aug 17, 2020

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

SimenB commented Aug 17, 2020

Sounds good!

Comment on lines 47 to 49
} else if (plugins.DOMElement.test(value)) {
//@ts-expect-error skip casting to Node
return value.cloneNode(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

WeiAnAn commented Aug 17, 2020

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.

@@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

maybe? not sure if it's better or not

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Comment override is fine

@SimenB
Copy link
Member

SimenB commented Aug 19, 2020

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

this is great, thank you!

@WeiAnAn
Copy link
Contributor Author

WeiAnAn commented Aug 19, 2020

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

@SimenB SimenB merged commit 2e30f52 into jestjs:master Aug 19, 2020
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Aug 21, 2020
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
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Sep 16, 2020
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
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Sep 17, 2020
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
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Sep 18, 2020
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
chrisbobbe added a commit to zulip/zulip-mobile that referenced this pull request Sep 18, 2020
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
IanLondon added a commit to Opentrons/opentrons that referenced this pull request Nov 24, 2020
jest update motivated by jestjs/jest#10414 -- unhelpful message instead of object diff in failed tests (eg when using immer)
IanLondon added a commit to Opentrons/opentrons that referenced this pull request Nov 24, 2020
@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.

Cannot assign to read only property '...' of object '...'
3 participants