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

Support text and comment nodes in html pretty printing #3355

Merged
merged 7 commits into from
Apr 28, 2017

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Apr 23, 2017

Summary
(I've edited this since opening (and deleted my comments on the way), as I was able to fix it myself)
Support printing text and comment nodes with the HTML pretty printer.

Test plan
yarn jest

/cc @mute

);
const pass = prettyFormatted === expected;
getPrettyPrint: plugins =>
function(received, expected, opts) {
Copy link
Member Author

@SimenB SimenB Apr 23, 2017

Choose a reason for hiding this comment

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

Had to make this function instead of arrow, otherwise I get

  ● HTMLElement Plugin › supports siblings

    TypeError: Cannot read property 'matcherHint' of undefined

      at pass (packages/pretty-format/src/__tests__/expect-util.js:42:20)
      at process._tickCallback (internal/process/next_tick.js:109:7)

Rest of the changes is prettier auto-formatting (https://github.com/facebook/jest/pull/3355/files?w=1)

@SimenB SimenB changed the title [WIP] Support sibling elements in html pretty printing [WIP] Support sibling text elements in html pretty printing Apr 23, 2017
@SimenB SimenB changed the title [WIP] Support sibling text elements in html pretty printing [WIP] Support sibling text nodes in html pretty printing Apr 23, 2017
@SimenB SimenB changed the title [WIP] Support sibling text nodes in html pretty printing Support text nodes in html pretty printing Apr 23, 2017
@SimenB SimenB changed the title Support text nodes in html pretty printing Support text and comment nodes in html pretty printing Apr 23, 2017
@codecov-io
Copy link

codecov-io commented Apr 23, 2017

Codecov Report

Merging #3355 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3355      +/-   ##
==========================================
- Coverage   64.12%   64.09%   -0.03%     
==========================================
  Files         177      177              
  Lines        6558     6561       +3     
  Branches        4        4              
==========================================
  Hits         4205     4205              
- Misses       2352     2355       +3     
  Partials        1        1
Impacted Files Coverage Δ
packages/pretty-format/src/index.js 98.06% <100%> (-0.01%) ⬇️
packages/pretty-format/src/plugins/HTMLElement.js 88.23% <100%> (-8.44%) ⬇️

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 79203df...9c07d02. Read the comment docs.

@SimenB SimenB force-pushed the sibing-html branch 4 times, most recently from 7723299 to baff25e Compare April 23, 2017 10:26
print: Print,
indent: Indent,
opts: Options,
colors: Colors,
) => {
): string => {
if (element instanceof Text) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if you want theseinstanceof checks, but it was the only way I could get flow to accept my input. Will it work outside of JSDOM? Do we care?

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't work outside of having the jsdom globals added as globals in node. Assuming that's nok OK, any idea on how to make flow accept that element.nodeType === 3 is the "same" as element instanceof Text?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Checking instanceof works at least for Chrome.

screen shot 2017-04-23 at 19 03 01

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it works in the browser (and "wannabe-browsers", like jsdom), but not in Node. The issue is if you want to use the plugin from node without polluting global namespace

@SimenB SimenB force-pushed the sibing-html branch 2 times, most recently from 86e06bc to af36162 Compare April 23, 2017 14:40
@@ -790,6 +790,7 @@ const DEFAULTS: Options = {
printFunctionName: true,
spacing: '\n',
theme: {
comment: 'gray',
Copy link
Member Author

Choose a reason for hiding this comment

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

No idea if this is considered a breaking change, but the next version is a major anyways

@cpojer
Copy link
Member

cpojer commented Apr 28, 2017

This is great Simen! I fixed it up a bit and addressed the instanceof checks and made custom types for them.

@cpojer cpojer merged commit 5d5e213 into jestjs:master Apr 28, 2017
@SimenB SimenB deleted the sibing-html branch April 28, 2017 15:56
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Make sure pretty-format expect util has correct context

* Support comment nodes in html pretty printing

* Support text nodes in html pretty printing

* Add comment key to pretty-format theme

* Add type to element in html pretty printer

* Remove unused arg from html pretty printer

* Cleanup
@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 13, 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.

5 participants