Adds HtmlElement pretty-format plugin. by mute · Pull Request #3230 · jestjs/jest

mute · 2017-03-30T05:27:22Z

Summary

This addresses the issue noted in #2146, with jsdom HTMLElement's producing an absurdly long string when pretty formatted. It adds an HTMLElement plugin which can be used to appropriately pretty-format these elements.

Test plan

I tested this using a variety of unit tests, similar to the existing tests that were covering ReactElement.

The cases I covered were ensuring that the class / title properties would pretty-format correctly, along with attributes on single elements and nested elements.

Here is the output from running those tests:

Merging changes from Origin

thymikee

Nice work! Left some comments inlined. Btw, that's great usage of @jest-environment pragma 👍

thymikee · 2017-03-30T05:54:30Z

packages/pretty-format/src/__tests__/expect-util.js

+module.exports = {
+  getPrettyPrint: plugins =>
+    (received, expected, opts) => {
+      const prettyPrintImmutable = prettyFormat(


We could use some generic naming here, e.g. prettyPrintPlugin

thymikee · 2017-03-30T05:54:37Z

packages/pretty-format/src/__tests__/expect-util.js

+          opts,
+        ),
+      );
+      const pass = prettyPrintImmutable === expected;


thymikee · 2017-03-30T06:01:37Z

packages/pretty-format/src/plugins/HTMLElement.js

+
+const escapeHTML = require('./lib/escapeHTML');
+const HTML_ELEMENT_REGEXP = /(HTML\w*?Element)/;
+const test = (maybeSet: any) => isHTMLElement;


change naming, it's not Set

thymikee · 2017-03-30T06:05:19Z

packages/pretty-format/src/plugins/HTMLElement.js

+      val.constructor &&
+      val.constructor.name &&
+      HTML_ELEMENT_REGEXP.test(val.constructor.name));
+}


This is potentially a hot place, please keep perf in mind: https://twitter.com/bmeurer/status/846951275480711168

thymikee · 2017-03-30T06:10:26Z

packages/pretty-format/src/__tests__/HTMLElementPlugin-test.js

+describe('HTMLElement Plugin', () => {
+  it('supports a single HTML element', () => {
+    expect(document.createElement('div')).toPrettyPrintTo(
+      '<HTMLDivElement  />',


Why double space?

thymikee · 2017-03-30T06:11:17Z

packages/pretty-format/src/__tests__/HTMLElementPlugin-test.js

+    const child = document.createElement('span');
+    parent.appendChild(child);
+    expect(parent).toPrettyPrintTo(
+      '<HTMLDivElement >\n  <HTMLSpanElement  />\n</HTMLDivElement >',


Please remove this extra space also here: <HTMLDivElement >

thymikee · 2017-03-30T06:12:48Z

packages/pretty-format/src/__tests__/HTMLElementPlugin-test.js

+    child.setAttribute('class', 'classy');
+
+    expect(parent).toPrettyPrintTo(
+      '<HTMLDivElement >\n  <HTMLSpanElement \n    id=123\n    class=classy\n  />\n</HTMLDivElement >',


Attributes should be strings id="123" class="classy".

Reworks short circuiting logic for `isHTMLElement` function.

thymikee

Awesome!

cpojer · 2017-03-30T12:21:13Z

packages/pretty-format/src/plugins/HTMLElement.js

+const test = isHTMLElement;
+
+function isHTMLElement(value: any) {
+  const toStringed = value.toString();


I'm a bit worried about performance here. This will call toString on any value passed to pretty-format. toString may be expensive if it is overwritten on a class instance. Can we do Object.prototype.toString.call?

That's a really good call -- I'll check how that behaves on an actual JSDOM HTMLElement to verify.

Looking at it again, I think it's possible that we could just remove this toString completely and just rely on the constructor validation, but I was deferring to the existing implementation from the previous PR.

Does that seem reasonable?

Yup, I think we could make it even more performant. According to ElementImpl we could check for property nodeType. Although this prop alone could possibly result in some false positives (nodeType feels not so unique...), it's good for an early check. Only when we know there's such prop, we could perform constructor and regex check:

const test = (value: any) => value !== undefined && value.nodeType === 1 && value.constructor !== undefined && HTML_ELEMENT_REGEXP.test(value.constructor.name);

Awesome -- I've updated the PR with those changes.

I'm not sure what to do about the tests that are failing; do I need to have mercurial configured locally for those?

Yes, as stated here, point 6: https://github.com/facebook/jest/blob/master/CONTRIBUTING.md#workflow-and-pull-requests

This reverts commit 55f797f.

thymikee · 2017-04-11T05:28:36Z

packages/pretty-format/src/__tests__/ImmutablePlugins-test.js

+const toPrettyPrintTo = require('./expect-util').getPrettyPrint([
+  ReactElementPlugin,
+  ReactTestComponentPlugin,
+  ...ImmutablePlugins,


Node 4 fails because of this destructuring. You can use concat instead

codecov-io · 2017-04-11T06:08:05Z

Codecov Report

Merging #3230 into master will increase coverage by 0.16%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master    #3230      +/-   ##
==========================================
+ Coverage   64.24%   64.41%   +0.16%     
==========================================
  Files         175      176       +1     
  Lines        6436     6469      +33     
  Branches        4        4              
==========================================
+ Hits         4135     4167      +32     
- Misses       2300     2301       +1     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-diff/src/index.js	`80% <100%> (+0.4%)`	⬆️
packages/jest-matcher-utils/src/index.js	`98.66% <100%> (+0.01%)`	⬆️
packages/jest-snapshot/src/plugins.js	`100% <100%> (ø)`	⬆️
packages/pretty-format/src/plugins/HTMLElement.js	`96.66% <96.66%> (ø)`

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 b93f9c6...e86a25d. Read the comment docs.

cpojer · 2017-04-11T10:49:52Z

What was the decision to use "HTMLDivElement" instead of "div"? Can we instead simply use node.tagName?
Do you mind adding this plugin everywhere in Jest where prettyFormat is called?

mute · 2017-04-13T05:16:30Z

Sure thing!

thymikee · 2017-04-13T05:52:00Z

packages/pretty-format/src/plugins/HTMLElement.js

@@ -60,7 +61,7 @@ const print = (
 ) => {
  let result = colors.tag.open + '<';
  const elementName = element.constructor


const elementName = element.tagName

tagName defaults to all uppercase, which felt odd to me -- should I leave it that way or is the .toLowerCase() call okay here?

lower case is better, leave it as is :)

cpojer · 2017-04-17T23:57:51Z

Thanks for doing this!

* Adds HtmlElement pretty-format plugin. * Adds missing copyright and 'use strict' * Fixes variable naming / formatting. Reworks short circuiting logic for `isHTMLElement` function. * Improves performance for isHtmlElement. * Updating snapshots * Revert "Updating snapshots" This reverts commit 55f797f. * Fixes node 4 syntax * - Switches to tagName rather than constructor. * Adds HTMLElement Plugin usage. * Removes `HTMLElement` fallback in favor of tagName

github-actions · 2021-05-13T18:15:57Z

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.

mute and others added 2 commits March 29, 2017 22:01

Merge pull request #1 from facebook/master

ec3a6a0

Merging changes from Origin

Adds HtmlElement pretty-format plugin.

15ef7d9

facebook-github-bot added the CLA Signed ✔️ label Mar 30, 2017

Adds missing copyright and 'use strict'

311afcf

thymikee requested changes Mar 30, 2017

View reviewed changes

Fixes variable naming / formatting.

6bcf0fa

Reworks short circuiting logic for `isHTMLElement` function.

thymikee approved these changes Mar 30, 2017

View reviewed changes

cpojer reviewed Mar 30, 2017

View reviewed changes

Natan LaFontaine added 5 commits April 9, 2017 21:44

Improves performance for isHtmlElement.

478f108

Merge remote-tracking branch 'jester/master'

33d2633

Merge branch 'master' into 2146-pretty-jsdom

25c85c8

Updating snapshots

55f797f

Revert "Updating snapshots"

cb5781a

This reverts commit 55f797f.

thymikee reviewed Apr 11, 2017

View reviewed changes

Fixes node 4 syntax

4195c59

Natan LaFontaine added 2 commits April 12, 2017 22:45

- Switches to tagName rather than constructor.

b3cff75

Adds HTMLElement Plugin usage.

b08ee74

thymikee reviewed Apr 13, 2017

View reviewed changes

Removes HTMLElement fallback in favor of tagName

e86a25d

cpojer merged commit adf7b2c into jestjs:master Apr 17, 2017

mute deleted the 2146-pretty-jsdom branch April 18, 2017 00:01

SimenB mentioned this pull request Apr 23, 2017

Add html elements to built-in serialisers #3356

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Uh oh!

Conversation

mute commented Mar 30, 2017

Uh oh!

thymikee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thymikee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mute Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cpojer commented Apr 11, 2017

Uh oh!

mute commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpojer commented Apr 17, 2017

Uh oh!

github-actions bot commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mute Mar 31, 2017 •

edited

Loading

codecov-io commented Apr 11, 2017 •

edited

Loading