prettyDOM error when container is document

[npeterkamps]

• dom-testing-library version: 2.3.2
• cypress-testing-library version: 2.0.0
• cypress version: 2.1.0
• react version: 16.3.2
• node version: 8.4.0
• npm version: 5.3.0

Relevant code or config:

cy.getByLabelText("something not on the page");

What you did:

I'm selecting an element using getByLabelText, but my code isn't setting the htmlFor correctly, so it couldn't find it.

What happened:

1. cypress-testing-library passes the window.document as container to the query.
2. Since getByLabelText can't find the element, it wants to log a friendly error message using prettyDOM.
3. prettyDOM uses outerHTML.length to determine whether to truncate. However, document.outerHTML is undefined resulting in the error.

TypeError: Cannot read property 'length' of undefined
    at prettyDOM (http://localhost:3001/__cypress/tests?p=cypress\support\index.js-147:898:59)
    at debugDOM (http://localhost:3001/__cypress/tests?p=cypress\support\index.js-147:920:35)
    at getAllByLabelText (http://localhost:3001/__cypress/tests?p=cypress\support\index.js-147:1181:216)
    at firstResultOrNull (http://localhost:3001/__cypress/tests?p=cypress\support\index.js-147:932:30)
    at getByLabelText (http://localhost:3001/__cypress/tests?p=cypress\support\index.js-147:1194:28)
    at container (http://localhost:3001/__cypress/tests?p=cypress\support\index.js-147:82:28)
    at onMutation (http://localhost:3001/__cypress/tests?p=cypress\support\index.js-147:1309:22)
    at http://localhost:3001/__cypress/tests?p=cypress\support\index.js-147:1327:7
    at new Promise (<anonymous>)
    at waitForElement (http://localhost:3001/__cypress/tests?p=cypress\support\index.js-147:1289:10)
From previous event:
    at Context.thenFn (http://localhost:3001/__cypress/runner/cypress_runner.js:56945:26)
    at Context.then (http://localhost:3001/__cypress/runner/cypress_runner.js:57207:21)
    at Context.<anonymous> (http://localhost:3001/__cypress/runner/cypress_runner.js:63338:21)
    at http://localhost:3001/__cypress/runner/cypress_runner.js:63053:33
From previous event:
    at runCommand (http://localhost:3001/__cypress/runner/cypress_runner.js:63043:14)
    at next (http://localhost:3001/__cypress/runner/cypress_runner.js:63125:14)
    at http://localhost:3001/__cypress/runner/cypress_runner.js:63137:18

Reproduction:

I've spent a little bit of time to try and test this, but with JSDOM, this doesn't seem to be easy. prettyDOM(document) crashes before it hits the outerHTML.length part due to pretty-format package (RangeError: Invalid string length).

Full `pretty-format` error

  RangeError: Invalid string length
      at printListItems (node_modules/pretty-format/build/collections.js:105:33)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printListItems (node_modules/pretty-format/build/collections.js:105:35)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)
      at printComplexValue (node_modules/pretty-format/build/index.js:187:169)
      at printer (node_modules/pretty-format/build/index.js:236:10)
      at printObjectProperties (node_modules/pretty-format/build/collections.js:140:21)

I believe this can be reproduced by just using cypress-testing-library and selecting something that isn't on the page? I'll do some more testing in the weekend.

Problem description:

prettyDOM function shouldn't assume outerHTML is defined.

Maybe a more accurate description would be: I don't know how to write a test case for this within dom-testing-library to test a pull request.

Suggested solution:

I'm not sure why outerHTML is being used, since it's the debugContent that is being truncated. So my suggestion would be to check debugContent.length instead. Otherwise add an && htmlContent.outerHTML before the length check.

[sompylasar]

pretty-format bugs out because it tries to print the whole document, which in your case is probably huge, without thinking about string memory limitations. Looks like it cannot apply the DOM plugins and tries to handle it as a regular object.

I believe the best solution would be to check if the htmlElement passed into prettyDOM is actually an HTMLElement, and if it's not, fall back to a shallow (non-recursive) printer. The check could be a naïve duck-typing by outerHTML for simplicity, or a more robust check.

function prettyDOM(htmlElement, maxLength) {
  const debugContent = prettyFormat(htmlElement, {

function prettyDOM(htmlElement, maxLength) {
  if (!htmlElement || typeof htmlelement.outerHTML !== 'string') {
    // TODO: Print a shallow string representation of htmlElement here.
    return;
  }
  const debugContent = prettyFormat(htmlElement, {

[kentcdodds]

pretty-format bugs out because it tries to print the whole document, which in your case is probably huge, without thinking about string memory limitations.

I think you're overestimating memory limits...

The problem is that document doesn't have an outerHTML property.

I think what we need is a check that if the htmlElement is document, then we should get things via document.documentElement.outerHTML. Should be pretty easy.

I'm not sure why outerHTML is being used, since it's the debugContent that is being truncated. So my suggestion would be to check debugContent.length instead. Otherwise add an && htmlContent.outerHTML before the length check.

The reason is because the debugContent includes extra characters for syntax highlighting.

[sompylasar]

@kentcdodds This error below is about the memory limits, pretty-format cannot build a string such big that it can contain the whole document printed recursively as a plain object.

RangeError: Invalid string length
      at printListItems (node_modules/pretty-format/build/collections.js:105:33)
      at printComplexValue (node_modules/pretty-format/build/index.js:176:141)
      at printer (node_modules/pretty-format/build/index.js:236:10)

I think what we need is a check that if the htmlElement is document, then we should get things via document.documentElement.outerHTML. Should be pretty easy.

Even if you get document.documentElement.outerHTML for the check below prettyFormat call, the prettyFormat call may still fail on the document (at least this is what we see with jsdom, maybe because it's not a real DOM and pretty-format DOM plugins check instanceof or something).

[kentcdodds]

Aahhh, gotcha. Maybe we should also allow for not using pretty-format or maybe not logging the DOM at all? Then cypress-testing-library can opt-into that. Cypress has better ways of inspecting the DOM anyway.

[sompylasar]

This isn't just about Cypress, the same can be reproduced with getByLabelText in dom-testing-library, it calls debugDOM with container that can easily be the document.

[kentcdodds]

I was suggesting that we do both. In the cypress situation it's more likely to run into the memory issue than in a JSDOM situation where you're normally not rendering as much DOM, so it shouldn't be necessary to opt-out of the prettyFormat stuff there (like it would be in cypress). But in both cases we should add a check for document and use document.documentElement instead in that case.

[sompylasar]

I was suggesting that we do both. In the cypress situation it's more likely to run into the memory issue than in a JSDOM situation where you're normally not rendering as much DOM, so it shouldn't be necessary to opt-out of the prettyFormat stuff there (like it would be in cypress). But in both cases we should add a check for document and use document.documentElement instead in that case.

👍 Yes, agree.

[npeterkamps]

When I pass the actual document (window.document within Cypress) to pretty-format, I get the following output:

"HTMLDocument {
  "_reactListenersID9641284360997817": 0,
  "location": Location {
    "ancestorOrigins": DOMStringList {
      "0": "http://localhost:3001",
    },
    "assign": [Function],
    "hash": "",
    "host": "localhost:3001",
    "hostname": "localhost",
    "href": "http://localhost:3001/some/path",
    "origin": "http://localhost:3001",
    "pathname": "/some/path",
    "port": "3001",
    "protocol": "http:",
    "reload": [Function],
    "replace": [Function],
    "search": "",
    "toString": [Function],
    Symbol(Symbol.toPrimitive): undefined,
  },
}"

I think what we need is a check that if the htmlElement is document, then we should get things via document.documentElement.outerHTML. Should be pretty easy.

htmlElement instanceof HTMLDocument doesn't work in Chrome. So I added a simple check for documentElement

if (htmlElement.documentElement) {
  htmlElement = htmlElement.documentElement;
}

Now I get the <html> node. outerHTML.length is 100285. debugContent.length is 201056.

I was suggesting that we do both. In the cypress situation it's more likely to run into the memory issue than in a JSDOM situation where you're normally not rendering as much DOM

So in Cypress situation, I'm not running into memory issues. As for JSDOM, I was just passing in the Jest global document into prettyDOM, which made it crash. So at most, the two <div> elements from previous tests were rendered (AFAIK).

Cypress' Command Log showed the error:

The colors aren't working, but if Cypress is opting out of pretty-format, that won't be a problem. If we were to log information about the document, I think <body> is more useful compared to <html>, because logging the contents of <head> is probably not very helpful.

When I use the documentElement code with jsdom, prettyDOM returns

<html>
  <head />
  <body />
</html>

So to summarize:

1. Use documentElement within prettyDOM when available.
2. Create opt-out mechanism to not log DOM.
3. Use opt-out in cypress-testing-library.

Is that correct?

About the opt-out mechanism, how would you like to see that implemented? As an extra option to query functions? As an environment variable (not sure how cypress-testing-library would use that...)?

[kentcdodds]

Actually, I wonder if we could be smarter and rather than opting out we just somehow determine whether we're in node or browser and only format in node. That would fix issues when running test in codesandbox as well.

[npeterkamps]

Sounds good to me. My proposal would be to add that check to debugDOM. I think it should determine whether or not to log the DOM node. All the call sites should also be updated, so debugDOM can determine whether \n\n is necessary or not.

[kentcdodds]

Actually, thinking about it further I think we have three cases:

1. Running in node? Highlight and format
2. Running in Cypress? Don't even log it.
3. Running in a browser (but not Cypress)? Format only.

Because I think it would still be valuable to have the log in codesandbox/a browser, just not in cypress. I'm not sure of the best way to accomplish this though. Ideas/PRs welcome

[npeterkamps]

We could do something like:

function debugDOM(htmlElement) {
  const limit =  process.env.DEBUG_PRINT_LIMIT || 7000
  if (typeof window !== 'undefined') {
    if (window.Cypress) {
      // don't log anything
      return ''
    }
    if (window.document) {
      // format only
      return `\n\n${prettyDOM(htmlElement, limit, { highlight: false })}`
    }
  }
  // format and highlight
  return `\n\n${prettyDOM(htmlElement, limit)}`
}

prettyDOM would merge the 3rd parameter with the options it creates for pretty-format.

Correction: The problem isn't so much identifying the environment as it is to prioritize the environments. In tests, we have both node and (simulated) browser, so we need to prioritize one over the other. So I think the correct order is:

1. Cypress, don't log anything.
2. Browser without Node, format only,
3. Node, format and highlight

function debugDOM(htmlElement) {
  const limit =  process.env.DEBUG_PRINT_LIMIT || 7000
  const inNode = (typeof module !== 'undefined' && module.exports)
  const inBrowser = (typeof window !== 'undefined' && window.document)
  const inCypress = (typeof window !== 'undefined' && window.Cypress)
  if (inCypress) {
    // don't log anything
    return ''
  }
  if (inBrowser && !inNode) {
    // format only
    return `\n\n${prettyDOM(htmlElement, limit, { highlight: false })}`
  }
  // highlight
  return `\n\n${prettyDOM(htmlElement, limit)}`
}

With this code, the snapshots won't change. It also works in Cypress:

[kentcdodds]

Thanks for that. At first I was going to say I didn't want to handle the cypress use case in this library because it feels like the wrong place for this abstraction, but I think that it's not really a big deal so let's go for it. A PR would be appreciated! Thanks!

Actually, thinking about it further, I'm not sure that this will work in codesandbox because I think module and module.exports are defined in codesandbox/webpack situations. Could you test this out in codesandbox to be sure?

[npeterkamps]

Actually, thinking about it further, I'm not sure that this will work in codesandbox because I think module and module.exports are defined in codesandbox/webpack situations. Could you test this out in codesandbox to be sure?

When running in codesandbox, all relevant tests pass (after some minor changes, see SANDBOX_CHANGES.md).

In other words, it uses the Node logging; highlight and format. So the question is whether this is undesirable. I think it's better to use Node logging, since it matches what will happen when testing using Node or npm scripts. Otherwise the environment change will break test snapshots.

I tried checking process.title (inspired by this StackOverflow answer), which apparently is set to 'browser' in codesandbox. When running the tests through npm t locally, it's set to 'npm'. So if we want to use browser logging and it's not considered too hacky ;), we could do:

function debugDOM(htmlElement) {
  const limit =  process.env.DEBUG_PRINT_LIMIT || 7000
  const inNode = (typeof module !== 'undefined' && module.exports)
  const inBrowser = (typeof window !== 'undefined' && window.document)
  const inCypress = (typeof window !== 'undefined' && window.Cypress)
  const inCodesandbox = (typeof process !== 'undefined' && process.title === 'browser')
  if (inCypress) {
    // don't log anything
    return ''
  }
  if ((inBrowser && !inNode) || inCodesandbox) {
    // format only
    return `\n\n${prettyDOM(htmlElement, limit, { highlight: false })}`
  }
  // highlight
  return `\n\n${prettyDOM(htmlElement, limit)}`
}