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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jest stalls after comparing to complex objects #1772

Closed
sebmarkbage opened this issue Sep 22, 2016 · 41 comments · Fixed by #6961 or #2078
Closed

Jest stalls after comparing to complex objects #1772

sebmarkbage opened this issue Sep 22, 2016 · 41 comments · Fixed by #6961 or #2078

Comments

@sebmarkbage
Copy link

(version v15.1.1)

In this PR, I can't use expect(a).toBe(b); on two complex and cyclic objects. It works fine if the test pass but if the test fails I think it tries to print/report the object structure or something. Interestingly enough it works fine on some of the other similar objects.

The end result is that jest stalls and doesn't print or proceed with anything else. I thought it might have been an infinite loop but if I run it with the Chrome debugger in Node 6 I can't stop. There's no break point so it seems like the event loop is just paused waiting for new messages that never arrive.

@chase
Copy link

chase commented Oct 5, 2016

@sebmarkbage It looks like the original commit that caused this failure was dropped during a history rewrite.

I checked out your fork and went to the next similar commit at sebmarkbage/react@f904163, then replaced the originally problematic line in src/renderers/shared/fiber/tests/ReactTopLevelFragment-test.js

    ReactNoop.flush();

    var instanceC = instance;

-    expect(instanceC === instanceA).toBe(true);
+    expect(instanceC).toBe(instanceA);
  });

When running jest at master (commit 2e36e25), the test fails but does not stall. I doubt that this is comparing the correct object, because the expected and received values are both null.

This is the output of the test: https://gist.github.com/chase/cc980da37005dc0d48bed7b3cdd9f4d5

It might be fixed in jest@master now, but I don't have access to the original commit where it broke.

@sighrobot
Copy link

(react 15.3, jest-cli 16.0)

This happens consistently when I expect(instance).toBeNull() when instance is in fact not null.

@gaearon
Copy link
Contributor

gaearon commented Oct 17, 2016

I can reproduce this with a simple test:

it('hangs', function() {
  var elem = document.createElement('div');
  expect(elem).toBe(undefined);
});

It hangs forever on Jest 16.0.1.

@quantizor
Copy link
Contributor

@cpojer any idea if you guys will be able to get to this soonish? Keeps coming up for my team and tracking down the culprit assertion is not always easy.

@cpojer
Copy link
Member

cpojer commented Oct 26, 2016

It's faster if you send a PR! Unfortunately I've been pretty backed up with a ton of work.

@quantizor
Copy link
Contributor

Hmm, ok I'll try and make time to look into this tomorrow.

@chase
Copy link

chase commented Oct 27, 2016

I did a little digging and it seems that @sebmarkbage's analysis was right.
Large or cyclic objects when passed to prettyFormat causes the cli to stall as the maximum depth is Infinity by default.

I will put in a basic PR tomorrow morning that at least deals with the symptoms mentioned, but I think that a long term solution would be to limit the verbosity or object depth being printed with a CLI option to adjust it.

@quantizor
Copy link
Contributor

Fix submitted to pretty-print jamiebuilds/pretty-format#47

@quantizor
Copy link
Contributor

Downgrade to jest-cli@15 if you want to avoid this bug until it's addressed.

@chase
Copy link

chase commented Nov 2, 2016

@yaycmyk, thanks for putting in the effort for HTML element printing. I'm sure your hours of work won't go to waste.

I got pulled for some crunch time tasks and ultimately got distracted from my pull request.
@cpojer, I forgot to ask: would the test be a better fit as an integration test? Seems like there's no easy option to use jsdom for a package's unit test as they all default to node for the testEnvironment.

@cpojer
Copy link
Member

cpojer commented Nov 2, 2016

Sure, that's fine too.

@quantizor
Copy link
Contributor

#2078

@gaearon
Copy link
Contributor

gaearon commented Nov 12, 2016

#2078 (comment)

@gaearon
Copy link
Contributor

gaearon commented Nov 21, 2016

This still happens for us in React repo with the latest Jest.
Unfortunately I'm having troubles trying to reproduce it outside of React repo :-(

I made a repro case inside React repo.

git clone https://github.com/gaearon/react.git react-jest-bug-repro
cd react-jest-bug-repro
git checkout jest-bug
yarn
npm test -- JestBug

Expected: it doesn't hang.
Actual: it hangs.

Commit with the repro: gaearon/react@8a0e694.

@gaearon gaearon reopened this Nov 21, 2016
@aaronabramov
Copy link
Contributor

should be fixed in #2148
@gaearon thanks for the repro case! I think it was getting stuck trying to send a very long error message from a child process to the master jest process.

@sebmarkbage
Copy link
Author

I have another case:

sebmarkbage/react@32f450c

I tried to patch in the fix in #2148 but it still stalls.

@cpojer
Copy link
Member

cpojer commented Dec 3, 2016

@sebmarkbage can you send a PR to Jest with a failing test?

@sebmarkbage
Copy link
Author

@cpojer I don't know how to reduce it yet. Pretty complex and don't know how to debug. Looks to be something with jest-diff when it calculates the diff.

@sebmarkbage
Copy link
Author

The issues is that jest-diff returns a too big string: https://github.com/facebook/jest/blob/master/packages/jest-diff/src/index.js#L107

I tried to use the same fix as #2148 but that makes it way too slow because running it over and over again at different depths is very slow when the tree is large.

Max depth is also not sufficient even for the first fix when you have a flat tree but many properties and large strings in each object. Even just a single long string at many characters (>10000) would probably be enough.

Gotta run but I guess that's another plausible unit test.

@ArtemGovorov
Copy link
Contributor

Here's a simple test that hangs jest:

it('takes 37 seconds', () => {
  expect(location).toBe({});
});

It actually finishes in ~37 seconds on my machine, seems like producing a huge diff.

@pedrottimark
Copy link
Contributor

@cpojer @gaearon The comments about maxDepth brought me here, because the pretty-format plugin API which incompletely exposes current indentation also doesn’t expose current depth, therefore the maxDepth option:

  • doesn’t apply to levels of structure that the plugin prints directly (for example, React elements)
  • but does apply to its print callback arg (for example, if a prop value is an object)

Although depth of React elements wasn’t a problem here for <div>Hello</div> or <div>World</div> what are your general expectations for a message when a test fails?

If it’s hard to imagine limited depth for React elements, consider plugins for Immutable data.

To be clear, this comment isn’t about the difference from jest-diff (which is the subject of the preceding few comments) but about the message from jest-matchers

expect(received).toBe(expected)
    
    Expected value to be (using ===):
      tweedle dee pretty format options include min and maxDepth…
    Received:
      tweedle dum …but plugins cannot enforce maxDepth, nor refs

@cpojer
Copy link
Member

cpojer commented Dec 15, 2017

Is this still a problem with the latest Jest 21.3 betas?

@gaearon
Copy link
Contributor

gaearon commented Dec 15, 2017

Does 21.3.0-beta.4 count as latest? (I haven't checked yet, just asking)

@thymikee
Copy link
Collaborator

@cpojer AFAIK @pedrottimark is working on a proper fix for that (but it's probably going to be a big rewrite of our diffing algo, so it will take a while), so please keep this open for reference.
@gaearon FYI latest Jest is 21.3.0-beta.15

@pedrottimark
Copy link
Contributor

Yes, I’m working on tests for new jest-diff-arrays package at this very moment :)

An update is overdue: if I came anywhere close to reproducing the original problem, when the test failed before implementing the feature:

  • serialization of instanceA was about 500 lines
  • serialization of instanceC was about 35000 lines

For this edge case of many instead of few differences, the diff dependency took about 5 minutes on Node.js 8 and 4 minutes on Node.js 9 because it allocates hundreds of millions of temporary objects. The replacement package for arrays has similar optimizations as diff-match-patch for strings to run in 0.025 seconds because its only heap storage is array of 35000 small integers and it short circuits zillions of loop iterations.

However, end result is that unusual looking assertion expect(instanceC === instanceA).toBe(true) remains if you don’t want 35000 lines of diff in console, no matter how fast Jest computes it.

@cpojer
Copy link
Member

cpojer commented Dec 16, 2017

Got it, thanks for the update!

billiegoose added a commit to isomorphic-git/isomorphic-git that referenced this issue Feb 9, 2018
…a multi-valued key.

* feat: Add 'all' argument to git.config to return array of values for a multi-valued key.
* chore: Consolidating tests
* chore: Try to stop Travis tests from stalling by reducing snapshot sizes. (I think I'm affected by jestjs/jest#1772)
tkrotoff added a commit to tkrotoff/react-form-with-constraints that referenced this issue May 22, 2018
…nts.test.tsx

Maybe related to Jest stalls after comparing to complex objects jestjs/jest#1772
Solution is to clone ValidityState
tkrotoff added a commit to tkrotoff/react-form-with-constraints that referenced this issue May 22, 2018
…nts.test.tsx

Maybe related to Jest stalls after comparing to complex objects jestjs/jest#1772
Solution is to clone ValidityState
@carusology
Copy link

carusology commented Jul 21, 2018

I got this to occur with image files. I was seeing if an image conversion tool I was using omitted an identical image as output by comparing buffers using Jest. If they were identical buffers, it passed without issue. If they weren't it hung. It seems to be just as @sebmarkbage described. Swapping to a simple boolean check or a hash was an easy workaround once I figured out this assertion was hanging.

I can't seem to get it to repro with any ol' test image file though, so you get to see my test pdf 😅. Maybe this will help @pedrottimark ?

const http = require('http');
const path = require('path');
const fs = require('fs');

test('this test will stall', async () => {
  let stallingImage1 = await download('stalled-image-1.png');
  let stallingImage2 = await download('stalled-image-2.png');

  let stallingBuffer1 = fs.readFileSync(stallingImage1);
  let stallingBuffer2 = fs.readFileSync(stallingImage2);

  expect(stallingBuffer1).toEqual(stallingBuffer2);
});

var download = (image) => {
  const bucket = 'http://storage.googleapis.com/gitlab-issue-downloads/';

  let localPath = path.resolve(__dirname, image);
  let file = fs.createWriteStream(localPath);

  return new Promise(
    (resolve, reject) => {
      http.get(bucket + image, function(response) {
        response.pipe(file);
        resolve(localPath);
      });
    }
  );
}

@davidlygagnon
Copy link

We're also seeing this issue with large snapshot files. Has anyone found a workaround?

@TUgonna
Copy link

TUgonna commented Aug 30, 2018

Currently, experiencing the same issue. any fix?

@matthew-dean
Copy link

I've also ran into this, where a failure in a comparison of two deep objects (with circular references) will cause the script to appear to stall for nearly a minute. I just want it to fail and say, "No, it's not the correct object reference," not a deep comparison of the instance properties, which is not valuable at all.

@matthew-dean
Copy link

Seems like the fix is to change expect(a).toBe(b) to expect(a === b).toBe(true)

@pedrottimark
Copy link
Contributor

@matthew-dean Yes, expect(a === b).toBe(true) avoids the problem.

Although expect(a).toBe(b) does test object identity instead of deep equality, the problem is if the assertion fails, then Jest displays the differences in the serialization of received and expected values. That output might not be relevant, even if we can speed it up.

@carusology Yeah, the serialization for buffers is huge, and I doubt that the diff is relevant to you.

What do you think about expect(stallingBuffer1.equals(stallingBuffer2)).toBe(true) as assertion?

@davidlygagnon @TUgonna Am back on this again. In jest-diff which displays results when an assertion fails, we will replace diff package with diff-sequences package. There is a huge performance gain when there is a big difference in number of lines between received and expected value.

Do you know if that is the situation when y’all see this issue?

@hisapy
Copy link

hisapy commented Oct 12, 2018

I'm having this problem with not so big snapshot files. In my case I got errors like

<--- Last few GCs --->

[65064:0x105800000]   242971 ms: Mark-sweep 1411.7 (1473.4) -> 1411.5 (1442.4) MB, 5512.1 / 0.2 ms  last resort GC in old space requested
[65064:0x105800000]   250116 ms: Mark-sweep 1411.5 (1442.4) -> 1411.5 (1442.4) MB, 7144.9 / 0.1 ms  last resort GC in old space requested


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x184239f25879 <JSObject>
    1: indexOf(this=0x1842b7c3ec71 <Very long string[424567]>)
    2: /* anonymous */(aka /* anonymous */) [/Users/hisa/workspace/eljurista/node_modules/pretty-format/build/plugins/lib/markup.js:43] [bytecode=0x1842aac77119 offset=60](this=0x18425d9822d1 <undefined>,key=0x1842c4ab87a1 <String[5]: relay>)
    3: arguments adaptor frame: 3->1
    4: map(this=0x1842b7c79869 <JSArray[17]>)
    5: /*...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
 1: node::Abort() [/usr/local/Cellar/node@8/8.12.0/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/usr/local/Cellar/node@8/8.12.0/bin/node]
 3: v8::Utils::ReportOOMFailure(char const*, bool) [/usr/local/Cellar/node@8/8.12.0/bin/node]
 4: v8::internal::V8::FatalProcessOutOfMemory(char const*, bool) [/usr/local/Cellar/node@8/8.12.0/bin/node]
 5: v8::internal::Factory::NewRawTwoByteString(int, v8::internal::PretenureFlag) [/usr/local/Cellar/node@8/8.12.0/bin/node]
 6: v8::internal::String::SlowFlatten(v8::internal::Handle<v8::internal::ConsString>, v8::internal::PretenureFlag) [/usr/local/Cellar/node@8/8.12.0/bin/node]
 7: v8::internal::String::IndexOf(v8::internal::Isolate*, v8::internal::Handle<v8::internal::String>, v8::internal::Handle<v8::internal::String>, int) [/usr/local/Cellar/node@8/8.12.0/bin/node]
 8: v8::internal::Runtime_StringIndexOfUnchecked(int, v8::internal::Object**, v8::internal::Isolate*) [/usr/local/Cellar/node@8/8.12.0/bin/node]
 9: 0x17f1435042fd
10: 0x17f1435683ba
error Command failed with signal "SIGABRT"

it hangs on

expect(tree).toMatchSpecificSnapshot(snapshotFilename);

//  tree is a mount wrapper from enzyme

Is there any progress or workaround for this for snapshot testing?

@hisapy
Copy link

hisapy commented Oct 12, 2018

I found that my snapshots were huge and were the cause of the out of memory error... luckily I also found mode: 'deep' option of enzyme-to-json and changed my snapshot strategy to split large files into multiple smaller files.

@siluri
Copy link

siluri commented Oct 24, 2018

i detect at v23.6.0 following bug:

arrStatus[0] is a complex object

that expect is correct and the expect works in miliseconds

expect(arrStatus[0]).toEqual({
   property1: expect.any(Object),
   property2: expect.any(Object),
   stringValue: 'included'
});

but do the same expect, with wrong string which not expect to the object - never ends up and my pc fan running on full throttle

expect(arrStatus[0]).toEqual({
   property1: expect.any(Object),
   property2: expect.any(Object),
   stringValue: 'not_included'
});

edit
change toEqual to toBe - never ends up also

expect(arrStatus[0]).toBe({
   property1: expect.any(Object),
   property2: expect.any(Object),
   stringValue: 'not_included'
});

Got following Errorby stringify the complex Object

JSON.stringify(arrStatus[0].property1);
TypeError: Converting circular structure to JSON

so i guess the main problem is a infinite loop because we have a circular structure in the complex object. so we need a solution to prevent this:

  • stop expecting after defined times runs up
  • throw error by comparison of circular structure object
  • or better solution ;)

@SimenB
Copy link
Member

SimenB commented Oct 24, 2018

We have an open PR that should hopefully fix this, see #6961. Will land as part of Jest 24

@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.