Pretty format for DOMStringMap and NamedNodeMap #5246
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5246 +/- ##
==========================================
+ Coverage 61.18% 61.25% +0.07%
==========================================
Files 202 203 +1
Lines 6771 6786 +15
Branches 3 4 +1
==========================================
+ Hits 4143 4157 +14
- Misses 2627 2628 +1
Partials 1 1
Continue to review full report at Codecov.
|
|
/cc @pedrottimark |
| return result; | ||
| }; | ||
|
|
||
| export const serialize = ( |
There was a problem hiding this comment.
There was a problem hiding this comment.
or at least rename this file to dom_collection
| DOMElement, | ||
| Immutable, | ||
| ReactElement, | ||
| ReactTestComponent, | ||
| } = prettyFormat.plugins; | ||
|
|
||
| let PLUGINS = [ | ||
| DOMCollection, |
There was a problem hiding this comment.
Although the destructuring assignment above is in alphabetic order, in the array let’s move DOMCollection to follow DOMElement so that the order of testing plugins is descending in expected frequency.
|
@zouxuoz Thank you for this pull request. For consistent serialization within Jest, can you please add the
|
| refs: Refs, | ||
| printer: Printer, | ||
| ): string => | ||
| collection.constructor.name + |
There was a problem hiding this comment.
A possible improvement which you can decide whether to make, or not, is implement the depth limit:
Oops, EDIT corrected property key: maxDepth
++depth > config.maxDepth
? '[' + collection.constructor.name + ']'
: /* existing expression */For example:
- https://github.com/facebook/jest/blob/master/packages/pretty-format/src/index.js#L186-L244
- https://github.com/facebook/jest/blob/master/packages/pretty-format/src/plugins/dom_element.js#L82-L84
- https://github.com/facebook/jest/blob/master/packages/pretty-format/src/plugins/immutable.js#L29-L51
There was a problem hiding this comment.
I think depth limit is not needed for DOM collections, because usually it's flat, but I can do it if needed.
Update: I'm added it 😉
There was a problem hiding this comment.
Agree, let’s keep it short and simple.
You are so quick with this super PR that I have not had time to test hypothesis that the problem is extra props in jsdom implementation and that pretty-format could serialize real DOM collections in browser. If we find problems with any other data structures, we might consider whether there is a correct and efficient test to detect jsdom objects more generically.
|
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. |
Summary
Resolve #5233
Test plan
I'm added tests for that and all tests is passed.