When a byRole query fails, crashes with RangeError: Invalid String Length

CreativeTechGuy commented 1 year ago

Similar to #16

DOM Testing Library Source - The root of the issue is here. When a byRole query fails, it'll try to prettify all of the roles, but when attempting to do this for some web components (like ones from lit), it'll recurse infinitely, hit the max JS string size, and crash.

In other cases, we've been able to get around this by customizing the getElementError method, but in this case, the above getMissingError function is called first and the result is passed into getElementError. So the crash happens before we can intercept it. Ref

This isn't directly because of this library, but since the goal of this library is to make working with web components easier, it'd be great if this could be fixed somehow by your library so that a basic query doesn't crash with an unintelligible error.

KonnorRogers commented 1 year ago

@CreativeTechGuy I think this should be possible by adding the createDOMElementFilter as a plugin to your printer. Let me dig around and I'll see if I can get an example for you.

https://github.com/KonnorRogers/shadow-dom-testing-library/blob/5f941e87e749d2504aa378d5ce7a8da89ad8989c/src/pretty-shadow-dom.ts#L242

KonnorRogers commented 1 year ago

Ohhhh....turns out this is never exported in index.ts

KonnorRogers commented 1 year ago

First of all thanks for reporting this, I tried to reproduce it, but can't seem to hit that prettyRoles function. Heres what I get when I try to reproduce locally:

➜  shadow-dom-testing-library git:(main) ✗ npm run jest __tests__/patch-testing-library-plugin.test.tsx

> shadow-dom-testing-library@1.10.0 jest
> jest __tests__/patch-testing-library-plugin.test.tsx

 FAIL  __tests__/patch-testing-library-plugin.test.tsx (6.49 s)
  ✕ Should serialize custom elements properly (272 ms)

  ● Should serialize custom elements properly

    ShadowDOMTestingLibraryElementError: Unable to find an accessible element with the role "button"

    There are no accessible roles. But there might be some inaccessible roles. If you wish to access them, then set the `hidden` option to `true`. Learn more about this here: https://testing-library.com/docs/dom-testing-library/api-queries#byrole

    Ignored nodes: comments, script, style
    <body>
      <div>
        <triple-shadow-roots>
          <ShadowRoot>
            <nested-shadow-roots>
              <ShadowRoot>
                <duplicate-buttons>
                  <ShadowRoot>
                    <slot
                      name="start"
                    />
                    <button>
                      Button One
                    </button>
                    <br />
                    <slot />
                    <br />
                    <button>
                      Button Two
                    </button>
                    <slot
                      name="end"
                    />
                  </ShadowRoot>
                </duplicate-buttons>
              </ShadowRoot>
            </nested-shadow-roots>
          </ShadowRoot>
        </triple-shadow-roots>
      </div>
    </body>
      11 |   getElementError (message, container) {
      12 |     const prettifiedDOM = prettyShadowDOM(container)
    > 13 |     const error = new Error(
         |                   ^
      14 |       [
      15 |         message,
      16 |         `Ignored nodes: comments, ${getConfig().defaultIgnore}\n${prettifiedDOM}`,

      at Object.getElementError (src/index.ts:13:19)
      at node_modules/@testing-library/dom/dist/query-helpers.js:90:38
      at node_modules/@testing-library/dom/dist/query-helpers.js:62:17
      at node_modules/@testing-library/dom/dist/query-helpers.js:111:19
      at Object.<anonymous> (__tests__/patch-testing-library-plugin.test.tsx:12:22)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        11.089 s
Ran all test suites matching /__tests__\/patch-testing-library-plugin.test.tsx/i.

Also, I now will be configuring the getElementError by default for the screen object exported by SDTL to make the out of the box experience easier.

import { configure } from "@testing-library/dom"
configure({
  getElementError (message, container) {
    const prettifiedDOM = prettyShadowDOM(container)
    const error = new Error(
      [
        message,
        `Ignored nodes: comments, ${getConfig().defaultIgnore}\n${prettifiedDOM}`,
      ]
        .filter(Boolean)
        .join('\n\n'),
    )
    error.name = 'ShadowDOMTestingLibraryElementError'
    return error
  }
})

If you have a minute, the branch is here:

konnorrogers/log-element-errors-with-pretty-shadow-dom

and the relevant test file is in:

__tests__/patch-testing-library-plugin.test.tsx

CreativeTechGuy commented 1 year ago

Based on your error message, you are hitting this branch which means that roles.length === 0 so this loop was never entered since container.children didn't have any elements. I think container might need to be a shadowRoot or have multiple web components as children so it'll enter that loop.

Thanks for looking into it!

CreativeTechGuy commented 1 year ago

As far as the getElementError method, that looks pretty good. One issue though (which you've seen with your test above) is that the stack trace now points to this error rather than the actual root cause. In our app, we are modifying the stack created by new Error to remove references to this file. I don't know if you'll need to do that as it might automatically remove the stack frames which are in node_modules (you aren't seeing this since you are testing from the library itself). So maybe it's a non-issue. But wanted to call it out as it stuck out to me.

KonnorRogers commented 1 year ago

I'm pretty sure it'll auto-remove the frame. But not 100%. FWIW, I just pulled this code straight from here:

https://github.com/testing-library/dom-testing-library/blob/39a64d4b862f706d09f0cd225ce9eda892f1e8d8/src/config.ts#L36-L51

As for the issue of getMissingError, I'm not sure if there's a good way around this wthout copying everything here:

https://github.com/testing-library/dom-testing-library/blob/39a64d4b862f706d09f0cd225ce9eda892f1e8d8/src/queries/role.ts

and then re-building the role queries, which feels quite fragile.

I think the better option may be to introduce a "depth" option.

So instead of getByRole("button") you would do: getByShadowRole("button", { depth: 0 }) and possibly have a shortcut like:

getByShadowRole("button", { excludeShadowDOM: true })

KonnorRogers / shadow-dom-testing-library

When a byRole query fails, crashes with RangeError: Invalid String Length #47