Node crashes with OOM if you pass pretty-format a large object

[theneva]

🐛 Bug Report

Node runs out of heap space and aborts with stack trace from V8 if you pass a large enough object to pretty-format, for example via toMatch[Inline]Snapshot().

I suppose this ties in with #4645, although that was closed by forcing pretty-format to not serialise the object type in question as a regular object.

To Reproduce

Assuming you run in the default jest-environment-jsdom, document is all you need to provoke this behaviour:

test('break', () => {
  expect(document).toMatchInlineSnapshot('');
});

Expected behavior

I think pretty-format should fail the test with an informative message instead of letting the Jest/Node process die if the serialisation is about to fill the heap.

One way to do this could be to abort object serialisation when when the test would otherwise time out. It takes (much) longer than the default test timeout of 5 seconds to run out of memory on my late 2013 MacBook Pro:

➜  jest-expect-document git:(master) time y jest
yarn run v1.12.1
$ /Users/theneva/code/jest-expect-document/node_modules/.bin/jest

 RUNS  ./index.test.js

<--- Last few GCs --->

[redacted]

<--- JS stacktrace --->

[redacted]

info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
yarn jest  47.43s user 1.56s system 185% cpu 26.362 total

Link to repl or repo (highly encouraged)

See screenshot and repro using that test here: https://github.com/theneva/jest-expect-document

Run npx envinfo --preset jest

Paste the results here:

System:
  OS: macOS Sierra 10.12.6
  CPU: (8) x64 Intel(R) Core(TM) i7-4850HQ CPU @ 2.30GHz
Binaries:
  Node: 10.6.0 - ~/.nvm/versions/node/v10.6.0/bin/node
  Yarn: 1.12.1 - /usr/local/bin/yarn
  npm: 6.1.0 - ~/.nvm/versions/node/v10.6.0/bin/npm
npmPackages:
  jest: ^23.6.0 => 23.6.0

[SimenB]

"sufficiently large object" is tracked in #1772. However, document should be special-cased like other jsdom objects to look like markup after serialization, imo.

@rickhanlonii @pedrottimark agree?

[rickhanlonii]

Yeah that makes sense to me 👍

[pedrottimark]

We have added special cases for real use cases related to jsdom in #4612 and #4645

It seems like document was easy repro, not request to support it.

@SimenB

• Is it possible to interrupt serialization if test times out? That makes more sense to me than what we attempt here: https://github.com/facebook/jest/blob/master/packages/jest-matcher-utils/src/index.js#L59-L81
• For your info Jest stalls after comparing to complex objects #1772 is for situation when jest-diff fails after pretty-format succeeded.

[SimenB]

It seems like document was easy repro, not request to support it.

Consider this a request to support it, then 😀

Is it possible to interrupt serialization if test times out?

Seeing as serialization is synchronous, I don't think so. Unless we spin up a separate vm for it, as we can pass timeout to them.

But maybe we can abort if the string we are creating is getting bigger than, say, 5000 chars? Or depth passes 15?

[pedrottimark]

@theneva Here are questions to select from possible changes to improve developer experience:

• the test was written correctly and failed before implementation was written (fail-first TDD)
• the test was written correctly and plugins don’t match object instance (for example, from jsdom)
• the test was written incorrectly and it was inconvenient to discover which test to fix

[theneva]

Hi! Sorry about the delay on this…

Yeah, document was just intended as an easy reproduction case, not the actual subject of the bug report. It would definitely be nice if document were serialised properly, though!

To be honest, I've never had a case of objects that are too big for serialisation except when I accidentally expected document to match a snapshot. I just figured I'd report it as an issue with an easy repro.

From a slightly ivory tower-y standpoint, though, I think it makes sense for every utility that comes out of the box with Jest (such as matchers) to honour the test timeouts if possible.

I took a quick look at pretty-format's printComplexValue(), and that already recurses indirectly (via printer()). It doesn't seem to me like it would be that hard to:

1. introduce an optional timeout parameter to prettyFormat() (defaulting to Infinity for backwards compatibility)
2. record the start time (Date.now() or something) and pass that to printComplexValue() as startTime
3. at the start of printComplexValue(), throw an error if Date.now() - startTime > timeout

If you don't see anything obviously wrong with that approach, I would be happy to try my hand at it.

@SimenB I agree that aborting on a max value for string length or object depth would be much better than the current behaviour, but I think it would be really hard to find a good max value: 100k character snapshots are a real use case—for example, a slightly complex GraphQL schema parsed with graphql-tag and then serialised in object form—and that example's snapshot writes quickly enough to be unnoticeable.

@pedrottimark I'm not entirely sure what you're asking. Please elaborate? 😄

[SimenB]

I think it makes sense for every utility that comes out of the box with Jest (such as matchers) to honour the test timeouts if possible.

prettyFormat is synchronous - no way to interrupt it in JS.

---

We've shipped a new diffing algorithm in Jest 24, so I think we can close this. Might revisit some upper bound on diff size if it's still an issue

[github-actions]

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.