Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snapshotting <select>.options breaks in pretty-format #7121

Closed
theneva opened this issue Oct 8, 2018 · 7 comments 路 Fixed by #7125
Closed

Snapshotting <select>.options breaks in pretty-format #7121

theneva opened this issue Oct 8, 2018 · 7 comments 路 Fixed by #7125

Comments

@theneva
Copy link
Contributor

theneva commented Oct 8, 2018

馃悰 Bug Report

Snapshotting the options property on a <select> element breaks in pretty-format.

Jest v22.1.2 on node v7.4.0 (the provided repl.it, see below) fails with:

TypeError: Cannot delete property 'cldr' of #<Object>
      
      at printObjectProperties (../../usr/local/share/.config/yarn/global/node_modules/pretty-format/build/collections.js:140:32)

Jest v23.6.0 on Node 10.11.0 (locally) fails with:

TypeError: Method get TypedArray.prototype.length called on incompatible receiver [object Object]
        at Uint8Array.get length [as length] (<anonymous>)

      at printComplexValue (../../node_modules/pretty-format/build/index.js:200:24)

To Reproduce

Create a <select> with at least one <option>, then attempt to snapshot select.options:

it('snapshots select options', () => {
  const select = document.createElement('select');
  select.innerHTML = '<option value="one">one</option>';

  expect(select.options).toMatchSnapshot();
});

Expected behavior

A snapshot should be created with the content of the options collection.

Link to repl or repo (highly encouraged)

https://repl.it/repls/CurvyKnobbyModel

Run npx envinfo --preset jest

Paste the results here:

  System:
    OS: macOS High Sierra 10.13.6
    CPU: x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
  Binaries:
    Node: 10.11.0 - /usr/local/bin/node
    Yarn: 1.10.1 - /usr/local/bin/yarn
    npm: 6.4.1 - /usr/local/bin/npm
  npmPackages:
    jest: ^23.6.0 => 23.6.0
@SimenB
Copy link
Member

SimenB commented Oct 8, 2018

@pedrottimark mind taking a look at this? Note that using a more complex options (like the one react spits out) makes the node process abort on a hefty macbook pro 2018

@pedrottimark
Copy link
Contributor

@SimenB Thanks for cc. Notice HTML elements from jsdom instead of React elements :)

Problem: pretty-format fails to serialize internals of a simulated DOM object instance as a generic object, because built-in snapshot serializers don鈥檛 match the class of select.options property:

HTMLOptionsCollection is an interface representing a collection of HTML option elements (in document order) and offers methods and properties for traversing the list as well as optionally altering its items. This type is returned solely by the "options" property of select.

Solution: Explicitly convert options or children property from array-like object to JavaScript array:

it('snapshots select options', () => {
  const select = document.createElement('select');
  select.innerHTML = '<option value="one">one</option>';

  expect(Array.from(select.options)).toMatchSnapshot();
});
it('snapshots select options', () => {
  const select = document.createElement('select');
  select.innerHTML = '<option value="one">one</option>';

  expect(Array.from(select.children)).toMatchSnapshot();
});

Here is the stored snapshot:

exports[`snapshots select options 1`] = `
Array [
  <option
    value="one"
  >
    one
  </option>,
]
`;

Although the thought has crossed my mind whether built-in plugins could support HTMLCollection or NodeList object instances created in browser or by jsdom package, so far I decided not, because everyone pays in every snapshot test for each additional test condition.

@theneva If this change to the assertion works for you, can you please close the issue?

@SimenB
Copy link
Member

SimenB commented Oct 8, 2018

Notice HTML elements from jsdom instead of React elements :)

Yeah, I know. @theneva is a coworker of mine, he showed me Jest's process abort using React (though rendering in jsdom, so kinda similar), the example in the OP is a smaller reproduction of what we believe to be the same issue.

everyone pays in every snapshot test for each additional test condition.

I might agree with not serializing it by default if the hit is high, but currently jest aborts, which is not OK (the example in the OP just takes a stupid amount of time before giving an error). I'd rather throw and tell the user to add a custom plugin. However, is the hit adding HTMLCollection/NodeList really that big?

Fun fact, expect(select).toMatchSnapshot(); serializes just fine to

exports[`options snapshot 1`] = `
<select>
  <option
    value="one"
  >
    one
  </option>
</select>
`;

@pedrottimark
Copy link
Contributor

Y鈥檃ll taught me a new DOM interface, for sure.

Was the original error also array-like object versus array as received value?

currently jest aborts, which is not OK (the example in the OP just takes a stupid amount of time before giving an error). I'd rather throw and tell the user to add a custom plugin.

Ja, good thought. I need to see if plugin can detect jsdom instances and skip their implementation.

@SimenB
Copy link
Member

SimenB commented Oct 8, 2018

Was the original error also array-like object versus array as received value?

No, the node process crashes because it runs out of memory

I need to see if plugin can detect jsdom instances and skip their implementation.

I'd prefer implementing support for the collections in https://github.com/facebook/jest/blob/master/packages/pretty-format/src/plugins/dom_collection.js and potentially pulling it if it turns out that this slows down e.g. test suites. Bailing early should be the backup plan 馃檪

@pedrottimark
Copy link
Contributor

pedrottimark commented Oct 8, 2018

Yes, I agree with you to support array-like collections in the plugin.

Ha ha, wrong day to search for other related classes under Web APIs on MDN:

There was a scripting error on this page. While it is being addressed by site editors, you can view partial content below.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants