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

pretty-format: Support HTMLCollection and NodeList in DOMCollection plugin #7125

Merged
merged 4 commits into from
Oct 11, 2018

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Oct 8, 2018

Summary

Fixes #7121

Problem: When pretty-format serializes a jsdom object instance for which no plugin matches, it takes a “stupid amount of time” on the property with key Symbol(impl) and then fails.

Solution: Support HTML collection classes to write assertions for received values:

  • element properties childNodes, children, elements, options
  • getElementsByTagName and so on
  • querySelectorAll

Also added support for config.min option to omit class name.

Be aware that class names can be moving targets even in browsers:

  • form.elements returned HTMLFormControlsCollection
  • fieldset.elements returned HTMLCollection

despite the claim at Mozilla Developer Network

The HTMLFormControlsCollection interface represents a collection of HTML form control elements. It represents the lists returned by the HTMLFormElement interface's elements property and the HTMLFieldSetElement interface's elements property.

So there is some risk for test writers that snapshots might need to be updated at major version upgrades of jsdom package.

Test plan

  • added 8 tests for collection classes
  • added 1 test for config.min option
  • added 1 test for config.maxDepth option

@codecov-io
Copy link

Codecov Report

Merging #7125 into master will not change coverage.
The diff coverage is 88.88%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #7125   +/-   ##
======================================
  Coverage    66.6%   66.6%           
======================================
  Files         253     253           
  Lines       10629   10629           
  Branches        3       4    +1     
======================================
  Hits         7079    7079           
  Misses       3549    3549           
  Partials        1       1
Impacted Files Coverage Δ
...ckages/pretty-format/src/plugins/dom_collection.js 93.33% <88.88%> (ø) ⬆️

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 c4646b9...69dc91f. Read the comment docs.


// Omitted a test for form.elements because constructor.name
// is HTMLCollection in jsdom version 11
// might become HTMLFormControlsCollectio in jsdom 12
Copy link
Member

Choose a reason for hiding this comment

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

jsdom 12 is released, and while we won't upgrade for some time, maybe we can normalize the name to HTMLFormControlsCollection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified outside Jest that form.elements still HTMLCollection class in [email protected] so added the test. Wrote a comment so that we will know to update the criterion if it ever changes.

'}'
: '[' +
printListItems(
Array.prototype.slice.call(collection),
Copy link
Member

Choose a reason for hiding this comment

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

Array.from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will leave the compiling to Babel :)

@pedrottimark
Copy link
Contributor Author

Documenting decision to serialize in list item format:

  • con: comma after item of markup looks awkward
  • pro: minimize change to snapshot if class names change in future jsdom versions
  • pro: minimize change to snapshot if received value changes to or from Array.from idiom

@SimenB SimenB merged commit 5fcaa24 into jestjs:master Oct 11, 2018
@pedrottimark pedrottimark deleted the dom-collection branch October 12, 2018 17:48
@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 12, 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.

Snapshotting <select>.options breaks in pretty-format
4 participants