Improved logging

[npeterkamps]

What:

• fix: don't crash when jsdom's document is passed to prettyDOM.
• feat(logging): don't log DOM in Cypress, don't highlight in the browser.

Why:

• When passing in jsdom's document, pretty-format module crashes. (prettyDOM error when container is document #44)
• When passing in Cypress window.document to prettyDOM, it crashes because document.outerHTML.length results in Cannot read property length of undefined error. (prettyDOM error when container is document #44)
• As discussed, logging the DOM in Cypress isn't really useful, so the DOM logging has been removed in Cypress environment.

How:

• Use document.documentElement in prettyDOM to have the actual element with an outerHTML property.
• Refactored debugDOM function to determine the environment and how/whether to log the DOM element.

Checklist:

• Documentation
• Tests
• Ready to be merged
• Added myself to contributors table

[aulisius]

Hey. Nice PR! Waiting on this currently. @kentcdodds yo, how soon can you merge this?

Thanks,
Faizaan.

[kentcdodds]

This looks great! Just one thing.

[kentcdodds]

Rather than adding the \n\n here, let's do this in the usage:

throw new Error(['Unable to find... etc...', debugDOM(container)].filter(Boolean).join('\n\n'))

[npeterkamps]

In Cypress, that would result in having two empty lines at the end of the error message. debugDOM seems to be a private function for query errors, so I thought this implementation would be preferable.

[kentcdodds]

Updated my comment to avoid that issue

[kentcdodds]

Looks like were missing some coverage. If it's really difficult to simulate a non-browser environment at the moment, then you can use /* istanbul ignore next */ comments. Open ./coverage/lcov-report/index.html to see what's missing. Thanks!

[npeterkamps]

Looks like were missing some coverage. If it's really difficult to simulate a non-browser environment...

I'm wondering, in what kind of situation would dom-testing-library be running in a pure browser environment? We've handled Cypress' case. Codesandbox runs in a mixed environment and I think it should have the same results as Node for consistency (as explain in this comment). So maybe we don't need to support a browser-only environment and just have an exception for Cypress?

I'll look into the coverage issue, don't know why line 59 isn't being covered by the tests. EDIT: jsdom/jsdom#2175 is fixed 😄

[npeterkamps]

@kentcdodds I've pushed an update.

The question remains whether we want to keep the browser specific code or whether dom-testing-library isn't going to be used in a browser-only environment (in which case, we might as well remove the code). I'm talking about the following lines:

if (inBrowser && !inNode) {
  return prettyDOM(htmlElement, limit, { highlight: false })
}

Let me know what you think!

[kentcdodds]

Some thoughts and ideas.

[kentcdodds]

Yeah, we could probably change this to:

if (inCypress) {
  return ''
} else if (inNode) {
  return prettyDOM(htmlElement, limit)
} else {
  return prettyDOM(htmlElement, limit, {highlight: false})
}

[kentcdodds]

I'm starting to think this may be a good case for something like:

throw getElementError(message, container)

where getElementError is just:

const getElementError => new Error([message, debugDOM(container)].filter(Boolean).join('\n\n'))

What do you think?

[npeterkamps]

Agreed. Changes pushed.

[kentcdodds]

Super duper! Thank you so much for iterating on this so well!

[kentcdodds]

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

[kentcdodds]

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀