feat: Add `filterNode` option to `prettyDOM` #907

eps1lon · 2021-03-01T11:02:35Z

BREAKING CHANGE: <script />, <style /> and comment nodes are now ignored by default in prettyDOM .If you whish to return to the old behavior, use a custom filterNode function. In this case prettyDOM(element, { filterNode: () => true }).

I would find this change useful. But I believe this is possible to do in userland so I'm preparing a PR that just implements this in userland. We can then decide whether we want to defer to userland, release it as a breaking change or make it backwards compatible.

What:

Ignore <script />, <style /> and comment nodes in prettyDOM

Why:

test frameworks like karma and mocha add <script /> and comment nodes that add quite a bit of noise making debugging harder.

Would also improve #902

How:

Fork DOMElement plugin from pretty-dom to allow filtering single Node instances.

Checklist:

Documentation added to the
docs site
Will do when we release the stable v8
Tests
Typescript definitions updated
Ready to be merged

codesandbox-ci · 2021-03-01T11:03:53Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e949974:

Sandbox	Source
react-testing-library-examples	Configuration

codecov · 2021-03-01T11:04:25Z

Codecov Report

Merging #907 (e949974) into alpha (532106b) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             alpha      #907   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        25    +1     
  Lines          902       914   +12     
  Branches       279       286    +7     
=========================================
+ Hits           902       914   +12

Flag	Coverage Δ
node-10.14.2	`100.00% <100.00%> (ø)`
node-12	`100.00% <100.00%> (ø)`
node-14	`100.00% <100.00%> (ø)`
node-15	`100.00% <100.00%> (ø)`
node-16	`100.00% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/queries/text.ts	`100.00% <ø> (ø)`
src/suggestions.js	`100.00% <ø> (ø)`
src/config.ts	`100.00% <100.00%> (ø)`
src/pretty-dom.js	`100.00% <100.00%> (ø)`
src/shared.ts	`100.00% <100.00%> (ø)`

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 532106b...e949974. Read the comment docs.

MatanBobi · 2021-03-07T15:43:41Z

I can see the value of this one but it is a breaking change and should be considered thoroughly.
What about adding this one behind a configuration?

eps1lon · 2021-03-07T15:59:52Z

I can see the value of this one but it is a breaking change and should be considered thoroughly.
What about adding this one behind a configuration?

I think I referred to all of this in the PR description: "We can then decide whether we want to defer to userland, release it as a breaking change or make it backwards compatible."

nickserv · 2021-03-18T00:24:55Z

Perhaps we could hide this behind a config boolean, and set the default to true on a major release.

eps1lon · 2021-03-18T08:57:36Z

I appreciate the review but let's focus on the high level problem: Do we want this as a feature or breaking change? What is the use case for the current behavior?

Then we can talk about the implementation. The implementation is problematic for a feature since it adds configuration but trivial to do.

MatanBobi · 2021-03-24T08:58:35Z

I appreciate the review but let's focus on the high level problem: Do we want this as a feature or breaking change? What is the use case for the current behavior?

I'm actually not so sure I'm in favor of adding this one. I think that isn't a part of our responsibility and may just cause bugs/issues in some cases. As you already wrote, this can be patched on the user-land so I'm not sure a solution on our end is really necessary.
That's just my opinion of course.

eps1lon · 2021-03-24T09:15:05Z

I think that isn't a part of our responsibility and may just cause bugs/issues in some cases.

I think neither of these statements are relevant. What is our responsibility is better error diagnostics. So the question is if this helps or hinders error diagnostic which we should talk about.

Causing bugs is inherent to any change. The question is how likely they are.

As you already wrote, this can be patched on the user-land so I'm not sure a solution on our end is really necessary.

I believe. I haven't actually done it so let's not jump to conclusions and assume that the userland implementation is trivial.

kentcdodds · 2021-04-24T20:32:52Z

We already have DEFAULT_IGNORE_TAGS:

dom-testing-library/src/config.ts

Line 44 in ffc8f26

export const DEFAULT_IGNORE_TAGS = 'script, style'

That's used for the ignore option on queries. I wonder whether it'd be possible to ignore everything that the failing query would ignore by default. That would be more correct anyway because right now we're displaying stuff that actually couldn't be queried.

kentcdodds · 2021-04-24T20:33:23Z

To be clear, I'm in favor of this change in light of that :)

eps1lon · 2021-04-24T20:52:23Z

We already have DEFAULT_IGNORE_TAGS:

That makes a lot of sense. I was a bit suprised that I couldn't find existing implementation. Will try to see if I can leverage DEFAULT_IGNORE_TAGS usage instead.

kentcdodds · 2021-04-25T01:51:54Z

To be clear, I think we should try to use whatever the ignore configuration is for the query that's failed. In most cases this will be the default, but it's an important distinction.

kentcdodds · 2021-04-25T01:52:24Z

In fact, it may be useful to also display the ignore configuration as well so folks know that not everything is being displayed.

eps1lon · 2021-04-25T10:50:09Z

src/shared.ts

@@ -0,0 +1 @@
+export const DEFAULT_IGNORE_TAGS = 'script, style'


Breaks up a circular dep between config and prettyDOM

src/DOMElementFilter.ts

Adds empty lines :(

kentcdodds

Thanks for this work!

Not excited about vendoring the DOMElementFilter file. I'm guessing they weren't receptive to a PR for this feature?

How much work would it be to take the ignore option into account for the errors that are thrown? For example, let's say I'm trying to find text within a style tag. I would set the selector and ignore values like so:

screen.getByText('.css-selector', {selector: 'style', ignore: 'script'})

Then my error should only show the things that could have matched. And it should show the selector and ignore configuration options as well.

So would it be possible to automatically filter the error message print out in a way that matches the selector and ignore values? I just think that a mismatch in those two could lead to confusing results.

I think it would amount to passing the selector and ignore options to prettyDOM and when those are provided, a filterNode function is created which only let's nodes that match the selector and don't match the ignore through. That way the error message displayed shows exactly what the user could have queried for given the options.

eps1lon · 2021-06-08T16:41:20Z

Not excited about vendoring the DOMElementFilter file.

Me neither 😄

I'm guessing they weren't receptive to a PR for this feature?

It was pretty cumbersome to integrate while not breaking all the other tests. They generally weren't receptive to filtering stuff out at all which makes sense considering pretty-format is used for generating snapshot matchers. And if you want to filter stuff out from snapshots you're better off implementing custom snapshot matchers. But that's not how we're using pretty-format.

I think I'm generally fine with the "fork now, upstream later" approach here.

I just think that a mismatch in those two could lead to confusing results.

This is a real concern I agree. But we might want to revisit if we want to consider screen.getByText('.css-selector', {selector: 'style', ignore: 'script'}) valid in the first place. I can't imagine that this is a useful query. It looks like querying implementation details when you're better off using querySelect(.css-selector) and assert the computed styles of that element.
In other words: Is the effort worth the edge cases it covers or does this edge case virtually never happen i.e. we're aware of the defect and are willing to push a bug fix if somebody needs it.

kentcdodds · 2021-06-08T18:08:46Z

I think I'm generally fine with the "fork now, upstream later" approach here.

Agreed.

valid in the first place.

Yeah, I was worried my contrived example would be distracting 😅

I think in general the biggest problem is the mismatch between what we see in the error and what we ignore/select. If you don't use ignore or select then it's fine because we can just go with the defaults, but once you use those options then it can become pretty unhelpful, and that's what I would like to avoid.

Is the effort worth the edge cases it covers or does this edge case virtually never happen i.e. we're aware of the defect and are willing to push a bug fix if somebody needs it.

That's fair. I currently don't have a use case for ignore or selector personally so I would be satisfied with a TODO code comment just so folks know that we'd like to support this in the future (rather than trying to figure out why we didn't do it in the first place).

eps1lon · 2021-06-08T18:21:32Z

I currently don't have a use case for ignore or selector personally so I would be satisfied with a TODO code comment just so folks know that we'd like to support this in the future (rather than trying to figure out why we didn't do it in the first place).

Sounds good. I just go ahead and add a test so that it's really "just" about fixing the problem. Makes it easier to get started contributing.

timdeschryver

This would be a great addition to ATL, where I believe that Karma is the most popular. Just a small typo...

types/pretty-dom.d.ts

Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>

kentcdodds

Solid 👍 Thanks!

github-actions · 2021-06-11T20:51:14Z

🎉 This PR is included in version 8.0.0-alpha.5 🎉

The release is available on:

npm package (@alpha dist-tag)
GitHub release

Your semantic-release bot 📦🚀

eps1lon added enhancement New feature or request BREAKING CHANGE This change will require a major version bump labels Mar 1, 2021

eps1lon changed the title ~~Add expected behavior~~ feat: Ignore <script /> and comment nodes when logging the DOM Mar 1, 2021

eps1lon changed the title ~~feat: Ignore <script /> and comment nodes when logging the DOM~~ BREAKING CHANGE: Ignore <script /> and comment nodes when logging the DOM Mar 1, 2021

eps1lon changed the title ~~BREAKING CHANGE: Ignore <script /> and comment nodes when logging the DOM~~ BREAKING CHANGE: Ignore <script />, <style />, and comment nodes when logging the DOM Apr 25, 2021

eps1lon commented Apr 25, 2021

View reviewed changes

src/DOMElementFilter.ts Outdated Show resolved Hide resolved

eps1lon commented Apr 25, 2021

View reviewed changes

src/DOMElementFilter.ts Show resolved Hide resolved

eps1lon mentioned this pull request Jun 1, 2021

chore(packages): support pretty-dom esm build #964

Closed

4 tasks

eps1lon added 7 commits June 8, 2021 11:47

Add expected behavior

820838c

WIP implement filterNode

2d68b77

Adds empty lines :(

REVERT LATER leverage jestjs/jest#11130

582ad1d

Use a fork of DOMElement plugin

13f4714

Update snapshots

7bb11b3

Expand what we mean by "useful error message"

458c7f1

Update src/DOMElementFilter.ts

4ee6ec8

eps1lon force-pushed the feat/pretty-dom-noise branch from 1378c7d to 4ee6ec8 Compare June 8, 2021 09:50

eps1lon changed the base branch from main to alpha June 8, 2021 09:50

eps1lon added 2 commits June 8, 2021 12:02

Bump pretty-format

41ac9b4

Update types

e514cc7

eps1lon changed the title ~~BREAKING CHANGE: Ignore <script />, <style />, and comment nodes when logging the DOM~~ feat: Add filterNode option to prettyDOM Jun 8, 2021

eps1lon added 2 commits June 8, 2021 12:20

Ignore coverage for fork

35c1824

poke codecov

d1a54bb

eps1lon marked this pull request as ready for review June 8, 2021 10:25

eps1lon requested review from kentcdodds, MatanBobi and nickserv June 8, 2021 10:25

kentcdodds reviewed Jun 8, 2021

View reviewed changes

Added test for undesired behavior

bd68e32

eps1lon requested a review from kentcdodds June 9, 2021 08:39

timdeschryver reviewed Jun 9, 2021

View reviewed changes

types/pretty-dom.d.ts Outdated Show resolved Hide resolved

Update types/pretty-dom.d.ts

e949974

Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>

kentcdodds approved these changes Jun 11, 2021

View reviewed changes

kentcdodds merged commit 9410e11 into testing-library:alpha Jun 11, 2021

github-actions bot added the released on @alpha label Jun 11, 2021

eps1lon deleted the feat/pretty-dom-noise branch June 12, 2021 08:11

eps1lon mentioned this pull request Jun 12, 2021

fix: Update @testing-library/dom testing-library/react-testing-library#931

Merged

3 tasks

eps1lon mentioned this pull request Jun 20, 2021

chore: PrettyFormatOptions -> PrettyDOMOptions #984

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `filterNode` option to `prettyDOM` #907

feat: Add `filterNode` option to `prettyDOM` #907

eps1lon commented Mar 1, 2021 •

edited

Loading

codesandbox-ci bot commented Mar 1, 2021 •

edited

Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

MatanBobi commented Mar 7, 2021

eps1lon commented Mar 7, 2021 •

edited

Loading

nickserv commented Mar 18, 2021

eps1lon commented Mar 18, 2021

MatanBobi commented Mar 24, 2021

eps1lon commented Mar 24, 2021

kentcdodds commented Apr 24, 2021

kentcdodds commented Apr 24, 2021

eps1lon commented Apr 24, 2021

kentcdodds commented Apr 25, 2021

kentcdodds commented Apr 25, 2021

eps1lon Apr 25, 2021

kentcdodds left a comment

eps1lon commented Jun 8, 2021

kentcdodds commented Jun 8, 2021

eps1lon commented Jun 8, 2021

timdeschryver left a comment

kentcdodds left a comment

github-actions bot commented Jun 11, 2021

		@@ -0,0 +1 @@
		export const DEFAULT_IGNORE_TAGS = 'script, style'

feat: Add filterNode option to prettyDOM #907

feat: Add filterNode option to prettyDOM #907

Conversation

eps1lon commented Mar 1, 2021 • edited Loading

codesandbox-ci bot commented Mar 1, 2021 • edited Loading

codecov bot commented Mar 1, 2021 • edited Loading

Codecov Report

MatanBobi commented Mar 7, 2021

eps1lon commented Mar 7, 2021 • edited Loading

nickserv commented Mar 18, 2021

eps1lon commented Mar 18, 2021

MatanBobi commented Mar 24, 2021

eps1lon commented Mar 24, 2021

kentcdodds commented Apr 24, 2021

kentcdodds commented Apr 24, 2021

eps1lon commented Apr 24, 2021

kentcdodds commented Apr 25, 2021

kentcdodds commented Apr 25, 2021

eps1lon Apr 25, 2021

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

eps1lon commented Jun 8, 2021

kentcdodds commented Jun 8, 2021

eps1lon commented Jun 8, 2021

timdeschryver left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 11, 2021

feat: Add `filterNode` option to `prettyDOM` #907

feat: Add `filterNode` option to `prettyDOM` #907

eps1lon commented Mar 1, 2021 •

edited

Loading

codesandbox-ci bot commented Mar 1, 2021 •

edited

Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

eps1lon commented Mar 7, 2021 •

edited

Loading