Autocomplete breaks in tests run with jsdom and vitest

mattcosta7 commented 2 years ago

Description

This issue may better exist on primer/behaviors (or even upstreamed to jsdom if we can repro simply enough)

Initially reported in slack by @itsbagpack https://github.slack.com/archives/C01L618AEP9/p1666186967969609

:wave::skin-tone-2: hi friends, i’m running into some testing issues around using the Autocomplete component and i was wondering what we were missing for context, here’s the PR we’re working with: https://github.com/github/delorean/pull/127

 TypeError: Failed to execute 'addEventListener' on 'EventTarget': parameter 3 dictionary has member 'signal' that is not of type 'AbortSignal'.
 ❯ exports.convert node_modules/jsdom/lib/jsdom/living/generated/AddEventListenerOptions.js:53:11
 ❯ HTMLDivElement.addEventListener node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:139:46
 ❯ Module.focusZone node_modules/@primer/behaviors/dist/esm/focus-zone.js:248:15
    246|     });
    247|     let elementIndexFocusedByClick = undefined;
    248|     container.addEventListener('mousedown', event => {
       |               ^
    249|         if (event.target instanceof HTMLElement && event.target !== document.activeElement) {
    250|             elementIndexFocusedByClick = focusableElements.indexOf(event.target);
 ❯ node_modules/@primer/react/lib-esm/hooks/useFocusZone.js:21:34

It appears that jsdom is throwing when trying to add an event listener, and appears that's due to the primer/behaviors focusZone attempting to pass a signal, which appears to conflict with the accepted signal jsdom expects. I added

Primer/react uses JSDOM (with jsdom) to test this component, which is confusing to me, given there's no indication that it currently has issues here

This leads me to think either a version of JSDOM begins breaking vite and jest somehow differ in the globals they send to jsdom in this scenario

I suggested a workaround, temporarily, mocking focusZone entirely -

 vi.mock("@primer/behaviors", async () => {
  const behaviors = await vi.importActual<typeof import("@primer/behaviors")>(
    "@primer/behaviors",
  )

  return {
    ...behaviors,
    focusZone: vi.fn(),
  }
})

but this changes behavior of the autocomplete, and is not ideal

Steps to reproduce

start a project with vite/vitest render an AutoComplete in a test using the jsdom environment

see jsdom throw

Version

All related versions of multiple packages

It's unclear to me whether this problem already exists and is becoming clear due to versioning of react and testing-libary, is due to something with how vite interops globals compared to jest, or if this is jest/jsdom.

"dependencies": {
    "@primer/octicons-react": "^17.7.0",
    "@primer/react": "^35.11.0-rc.65a8bb8a",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "styled-components": "^5.3.5"
  },
  "devDependencies": {
    "@testing-library/jest-dom": "^5.16.5",
    "@testing-library/react": "^13.4.0",
    "@testing-library/user-event": "^14.4.3",
    "@types/react": "^18.0.17",
    "@types/react-dom": "^18.0.6",
    "@vitejs/plugin-react": "^2.0.1",
    "typescript": "^4.6.4",
    "vite": "^3.0.7",
    "vitest": "^0.23.4"
  }

Browser

No response

github-actions[bot] commented 1 year ago

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

github-actions[bot] commented 6 months ago

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

keithamus commented 6 months ago

This is likely due to using an old version of jsdom. jsdom@20.0.0 is the first version of the lib to add support for signal in addeventlistener. Looks like the monolith and Delorean now use a version > 20, so I'll close this.

If in doubt, in future, please update to the latest version of jsdom as it'll likely fix issues like this.

mattcosta7 commented 6 months ago

This is likely due to using an old version of jsdom. jsdom@20.0.0 is the first version of the lib to add support for signal in addeventlistener. Looks like the monolith and Delorean now use a version > 20, so I'll close this.

If in doubt, in future, please update to the latest version of jsdom as it'll likely fix issues like this.

makese sense. IIRC that wasn't an option at the time we opened this

primer / react