Expose pretty-dom utility #25
Conversation
src/pretty-dom.js
Outdated
| printFunctionName: false, | ||
| highlight: true, | ||
| }) | ||
| const maxLength = process.env.DEBUG_PRINT_LIMIT || 7000 |
There was a problem hiding this comment.
Perhaps we should accept this as an argument? I'm thinking that folks who are logging things themselves might want to control this more easily and in general probably want to see the whole thing. So perhaps we should only have this default for where it was already in use and the real default should be Infinity?
There was a problem hiding this comment.
Yes I agree with @kentcdodds. Currently when a test case is failing, its hiding the contents of the container and I need to increase the DEBUG_PRINT_LIMIT to see the whole dom, which is causing me to re-run the case again. Since we are going to expose this to outside world, the default value should be infinity. That should make life easier :)
There was a problem hiding this comment.
I changed it, and now the env var is only used internally when pretty printing out DOM inside thrown exceptions by the element getters. The exposed prettyDOM does not limit the output at all, and allows a second optional argument for the user to set a limit per call.
|
As for the coverage thing: jest.spyOn(console, 'log').mockImplementation(() => {})
logDOM(document.createElement('div'))
expect(console.log.mock.calls).toMatchSnapshot()
console.log.mockRestore()😄 |
|
Yes, I was aware that documentation is pending but wanted to kick off the discussion of code here. I'll change the approach to the limit. I also wanted to discuss the code coverage issue plus the potential linting issue with |
|
@kentcdodds what do you think about not providing As a reference, I think I recall that enzyme's pretty print helper also did not do the |
|
I'd be fine with not including |
src/pretty-dom.js
Outdated
|
|
||
| const {DOMElement, DOMCollection} = prettyFormat.plugins | ||
|
|
||
| function prettyDOM(htmlElement, maxLength = undefined) { |
There was a problem hiding this comment.
Don't need the = undefined because that's functionally equivalent to what the value of maxLength would be anyway :)
There was a problem hiding this comment.
I know, I just like to be more explicit in the declaration that the argument is not expected, to anyone that passes by and read the code. Guess flow or typescript would be clearer for that purpose but I have yet to learn to use one of them, it's on my list ;)
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
Wow, that was fast! Thanks all! |
* added waitForExpect with test * added typescript and simplified version of the waitForExpect, used and exports its typings * added initial notes about waitForExpect * fixed styling * minor stylistic change * updated tests to remove the nesting * updated readme * fixed d .md syntax * improved style Closes testing-library#21
What:
This exposes the already existing internal utility function to print out a DOM in a human-friendly way.
Why:
As it was discussed in #24, this can be useful for users to print out the DOM under a certain html element, to debug more easily when writing tests.
How:
Simply moved the already existing utility to its own module.
Checklist:
Code coverage is not full now because of the
logDOMutility function. Now in hindsight I'm not sure if it's a good idea to provide it. This is because there's no eslint rule now warning the users that they left this command in use in their tests, when I think the intent is that this is only to be used while creating and changing tests, but not meant to be left in actual committed code.If you still think we should provide it, how do you think we should resolve the code coverage issue? Should we create a new test that uses it? Wouldn't that pollute the tests output? Is there a way to ignore some lines in the coverage report?