feat(debug_helper): Adding debug helper when a get call fails #3

antsmartian · 2018-04-06T14:46:45Z

Fix for testing-library/react-testing-library#31

@kentcdodds Let me know if things are good to go!

codecov · 2018-04-06T14:50:38Z

Codecov Report

Merging #3 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master     #3   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      5    -2     
  Lines         127    102   -25     
  Branches       33     27    -6     
=====================================
- Hits          127    102   -25

Impacted Files	Coverage Δ
src/queries.js	`100% <100%> (ø)`	⬆️
src/extend-expect.js	`100% <0%> (ø)`	⬆️
src/jest-extensions.js	`100% <0%> (ø)`	⬆️
src/wait-for-element.js
src/events.js

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 d45b449...b681a47. Read the comment docs.

sompylasar · 2018-04-06T16:22:17Z

src/queries.js

@@ -1,4 +1,6 @@
 import {matches} from './matches'
+import prettyFormat from 'pretty-format' // eslint-disable-line


Why eslint-disable-line here?

So basically pretty-format is not part of the package.json - dependencies but its part of jest I believe. Hence I have added the eslint-disable-line, yes, I need to clean up a bit by adding the exact eslint-ignore for the same.

But what if I don't use jest with dom-testing-library?

Sorry, pretty-format is from jest-matcher-utils (https://github.com/facebook/jest/blob/master/packages/jest-matcher-utils/package.json#L14) which is in our dependencies. Sorry for the rushed comment, saying it as jest in first place!

Hmm.. Is there a reason for not depending on pretty-format directly as well? :)

@lgandecki We use custom jest assertions, for which we depend on jest-matcher-utils. As part of printing the DOM, we thought we could use pretty-format as it was already part of the jest-matcher-utils and it does actually a good job. It works well for our use case, so why not? :)

I'm not against using pretty-format, I'm just trying to understand if there is a reason to not put it in package.json as a direct dependency. Then there is no need to fight around eslint complaining (rightly). And if jest-matcher-utils switch to something else internally and stop requiring pretty-format, our package will still work.

@lgandecki Yes, I agree. Will make the changes. Thanks your comments!

sompylasar · 2018-04-06T16:25:07Z

src/queries.js

  }
  return el
 }

+function checkDebugLimit(debugContent) {


The name is confusong: it shouldn't be "check" (would return boolean), it is more of a "trimToDebugLimit".

sompylasar · 2018-04-06T16:27:47Z

src/queries.js

  }
  return el
 }

+function checkDebugLimit(debugContent) {
+  const limit = process.env.DEBUG_PRINT_LIMIT


What if the limit needed in browser where there's no env, e.g. via Cypress?

I suggest exporting a mutable "defaultOptions" object from the module instead, like Node's util.inspect.defaultOptions.

Yes, thats a good point.

sompylasar · 2018-04-06T16:33:20Z

src/queries.js

+    debugContent = debugContent.slice(0, 7000)
+    if (contentLength >= 7000)
+      // there are more chars we have stripped, just show to the user
+      debugContent += '\n . . .'


The above "if" and "else" are essentially copy-paste except limit is replaced with a default value.

I suggest pulling the "magic number" into a "defaultOptions" object and use it like so:

const limit = defaultOptions.limit; // OR: const limit = defaultOptions.limit || 7000; const contentLength = debugContent.length debugContent = debugContent.slice(0, limit) if (limit <= contentLength) // there are more chars we have stripped, just show to the user debugContent += '\n . . .'

Yes, I agree. Can be improved. This is just a rough draft, I have to clean up few things. Thanks for your comments.

antsmartian · 2018-04-06T17:15:49Z

@kentcdodds Hey Kent.. @sompylasar suggested the following:

What if the limit needed in browser where there's no env, e.g. via Cypress?

I suggest exporting a mutable "defaultOptions" object from the module instead, like Node's util.inspect.defaultOptions.

w.r.t to process env for DEBUG_PRINT_LIMIT. I was thinking it for a moment, if we do like defaultOptions, then we need to export it, as part of this lib. So was thinking, what your thoughts on that? Yes to me, thats a good way to support across all the env.

antsmartian · 2018-04-09T05:30:31Z

Ping, any thoughts on my last comment? Guess people missed it ;) cc @kentcdodds

kentcdodds · 2018-04-09T05:58:57Z

I have thoughts. Patience. Not all of us have time to address this within hours or days. We'll get to it when we can 😉

One thing you could do is rebase your branch and fix merge conflicts 😀

antsmartian · 2018-04-09T06:15:14Z

@kentcdodds Oops! Sorry thought people had missed it. will do the rebase and update the branch.

kentcdodds

Some thoughts and ideas :)

kentcdodds · 2018-04-09T19:41:06Z

README.md

+For example:
+
+```javascript
+;`<div data-testid="debugging" data-otherid="debugging">


prettier added the ; to make this valid javascript.

Why don't we replace this whole block with:

```javascript // <div>Hello world</div> getByText(container, 'Goodbye world') // will fail by throwing error ```

This way:

There's less noise that could distract from people's understanding of what's happening

There's no weird ;

We're using getByText properly (with container as the first arg).

kentcdodds · 2018-04-09T19:44:17Z

README.md

+The above test case will fail, however it prints the state of your DOM under test, so you will get to see:
+
+```
+Unable to find an element with the text: Test value.. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.


"Test value." -> "Goodbye world"

README.md

+	>
+		Hello World!
+	</div>
+</div>


kentcdodds · 2018-04-09T19:49:28Z

README.md

+</div>
+```
+
+Note: Since the DOM size can go really large, you can set the limit of DOM content to be printed via environment variable `DEBUG_PRINT_LIMIT`.


"go" -> "get"

kentcdodds · 2018-04-09T19:50:15Z

src/queries.js

    throw new Error(
-      `Unable to find an element with the placeholder text of: ${text}`,
+      `Unable to find an element with the placeholder text of: ${text} \nHere is the state of your container: \n${htmlElement}`,


Let's remove the text: "Here is the state of your container" entirely. I think people will know what the printed HTML is without that.

kentcdodds · 2018-04-09T19:59:28Z

src/queries.js

  }
  return el
 }

 function getByPlaceholderText(container, text, ...rest) {
  const el = queryByPlaceholderText(container, text, ...rest)
  if (!el) {
+    const htmlElement = trimToDebugLimit(


Could we make a function called htmlElementToDisplay that trims and formats it, so we don't have to duplicate this everywhere.. Also, make sure that it compares the length of the html string rather than the formatted version because the formatted version will have extra characters for the highlighting.

I envision the error code below looking like:

throw new Error( `Unable to find an element with the placeholder text of: ${text}\n\n${htmlElementToDisplay(htmlElement)}`, )

And the htmlElementToDisplay should be something like:

function htmlElementToDisplay(htmlElement) { const debugContent = prettyFormat(htmlElement, { plugins: [DOMElement, DOMCollection], printFunction: false, highlight: true }) const maxLength = process.env.DEBUG_PRINT_LIMIT || 7000 return htmlElement.outerHTML.length > maxLength ? `${debugContent.slice(0, maxLength)}...` : debugContent }

Note the addition of DOMCollection to the plugins (handles an array of nodes I think). Also note that we checking the maxLength against htmlElement.outerHTML. If we checked it against debugContent then we'd get a length that wasn't quite right (because the debugContent includes a bunch of characters for the coloring).

src/__tests__/element-queries.js

+  process.env.DEBUG_PRINT_LIMIT = 10000 // user shouldn't see `. . .`
+  expect(() =>
+    expect(getByLabelText('not present')).toBeInTheDOM(),
+  ).toThrowError()


kentcdodds · 2018-04-09T20:05:45Z

src/__tests__/element-queries.js

+    </div>`
+  const {getByTestId} = render(Hello)
+  process.env.DEBUG_PRINT_LIMIT = 10 // user should see `. . .`
+  expect(() => expect(getByTestId('not present')).toBeInTheDOM()).toThrowError()


This is a good assertion, but we should add another assertion to make sure the error includes the ...

You can do this by adding a regex to the toThrowError call:

.toThrowError(/\.\.\./)

In the one below we should do the opposite

.toThrowError(/^((?!\.\.\.).)*$/)

I'm pretty sure that should verify that there is no ... in the error message.

kentcdodds

Sorry, meant to set my review to "Request changes" rather than "Approve"

kentcdodds

Sorry, meant to set my review to "Request changes" rather than "Approve"

kentcdodds

This is super! Thanks so much for your help on this!

sompylasar · 2018-04-10T16:13:35Z

@kentcdodds Did we go with unchecked process.env for configuration?

kentcdodds · 2018-04-10T16:14:47Z

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

kentcdodds · 2018-04-10T16:19:54Z

Yes, for this first release I thought it'd be fine to go with the process.env-only approach. Though now that I think about it that will break the browser case. We should probably protect that reference 🤔

I was planning on accepting a PR for enhancing the configurability of this feature. I didn't consider that this would break the browser use-case.

kentcdodds · 2018-04-10T16:20:13Z

That said, anyone using this in the browser will be using a bundler so I'm pretty sure this'll be fine.

sompylasar · 2018-04-10T16:26:31Z

That said, anyone using this in the browser will be using a bundler so I'm pretty sure this'll be fine.

I thought about this, too. Although I'm not 100% sure.

@antoaravinth Could you please check if this holds true at least for Cypress? You can do so from cypress-testing-library: clone it, update dom-testing-library dependency, npm run -s test:cypress or test:cypress:open. Thanks!

antsmartian · 2018-04-10T16:39:51Z

@sompylasar Yes it will. I have checked anyways:

Opening Cypress...
  (Tests Starting)


  dom-testing-library commands
    ✓ getByPlaceholderText (537ms)
    ✓ getByText (129ms)
    ✓ getByLabelText (495ms)
    ✓ getByAltText (134ms)
    ✓ getByTestId (134ms)


  5 passing (2s)


  (Tests Finished)

  - Tests:           5
  - Passes:          5
  - Failures:        0
  - Pending:         0
  - Duration:        2 seconds
  - Screenshots:     0
  - Video Recorded:  false
  - Cypress Version: 2.1.0


  (All Done)

sompylasar · 2018-04-10T16:42:34Z

@antoaravinth Awesome! 👍

feat(debug_helper): Adding debug helper when a get call fails

0ec31b3

sompylasar reviewed Apr 6, 2018

View reviewed changes

fix(review): Few minor changes

4605b14

Merging with master branch

7535c35

kentcdodds mentioned this pull request Apr 9, 2018

Utilities for dispatching events? #5

Closed

moving pretty-format to the dep

3ef675d

kentcdodds approved these changes Apr 9, 2018

View reviewed changes

kentcdodds reviewed Apr 9, 2018

View reviewed changes

kentcdodds requested changes Apr 9, 2018

View reviewed changes

Anto Aravinth and others added 4 commits April 10, 2018 20:42

fix(review_comments): Making the fixes mentioned in the review comments

56b7531

Merging with remote master:

dfb3904

Update README.md

7b93c5d

Update element-queries.js

b681a47

kentcdodds approved these changes Apr 10, 2018

View reviewed changes

kentcdodds merged commit c8069e3 into testing-library:master Apr 10, 2018

		@@ -1,4 +1,6 @@
		import {matches} from './matches'
		import prettyFormat from 'pretty-format' // eslint-disable-line

feat(debug_helper): Adding debug helper when a get call fails #3

feat(debug_helper): Adding debug helper when a get call fails #3

Conversation

antsmartian commented Apr 6, 2018

codecov bot commented Apr 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

antsmartian Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian commented Apr 6, 2018 • edited Loading

antsmartian commented Apr 9, 2018

kentcdodds commented Apr 9, 2018

antsmartian commented Apr 9, 2018 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

sompylasar commented Apr 10, 2018

kentcdodds commented Apr 10, 2018

kentcdodds commented Apr 10, 2018

kentcdodds commented Apr 10, 2018

sompylasar commented Apr 10, 2018

antsmartian commented Apr 10, 2018

sompylasar commented Apr 10, 2018

codecov bot commented Apr 6, 2018 •

edited

Loading

antsmartian Apr 6, 2018 •

edited

Loading

antsmartian Apr 6, 2018 •

edited

Loading

antsmartian Apr 6, 2018 •

edited

Loading

antsmartian commented Apr 6, 2018 •

edited

Loading

antsmartian commented Apr 9, 2018 •

edited

Loading