Prevent error in pretty-format for window in jsdom test env

[pedrottimark]

Summary

Fixes #4612

Instead of RangeError: Invalid string length return [Window] the constructor name in jsdom.

The serialization is not even relevant in snapshot if window is prop of React element.

Add a condition where pretty-format short-circuits serialization of object at max depth.

Theoretically it could serialize window if global is not defined (results would vary by browser) but so far I have not succeeded in configuring webpack for the bundle not to have global too.

From that I hypothesize both global and window will exist when running tests in-browser?

However, I am happy to forget about global and simplify the code so it tests only window

Test plan

Added a test (which failed first) to dom_element.test.js because although not related to plugin, that is only test file which specifies @jest-environment jsdom

[thymikee]

why not just:

const isGlobalWindow = val => {
  try {
    /* global window */
    return window === global;
  } catch (error) {
    return false;
  }
};

[pedrottimark]

In answer to your question, must return result of comparison to val

The complexity is in case some overhead to enclose val === window in try-catch, lazy evaluation of window at first attempt to serialize an object.

The more I think about it though, considering global seems like a mistake.

[pedrottimark]

Do you think I should do a set of perf measurements compared to simplest possible code?

[thymikee]

I was just referring to make it a bit simpler (I now see I forgot to include val in the equation :D). If you could do some local perf testing, that's always nice 👍

[SimenB]

Changelog? :D

[SimenB]

Try catch feels like overkill. Why not just typeof window != 'undefined'?

[pedrottimark]

It catches ReferenceError: window is not defined for --env=node in Jest or if pretty-format package is used for other purpose than testing.

Keep the questions coming. This one caused me to add comments in the code.

[SimenB]

I added a new comment - it does not.

$ node -p 'typeof window'
undefined

[SimenB]

Or well, the current test throws, but that's because the test itself is referring to window, not because it throws from inside pretty-format.

You can always typeof anything - even stuff that is not defined.

And this way you avoid the try-catch entirely

[SimenB]

I still don't think this is necessary. This change passes your test as well:

diff --git i/packages/pretty-format/src/index.js w/packages/pretty-format/src/index.js
index 35a591c9..ba4f91f1 100644
--- i/packages/pretty-format/src/index.js
+++ w/packages/pretty-format/src/index.js
@@ -41,6 +41,8 @@ const errorToString = Error.prototype.toString;
 const regExpToString = RegExp.prototype.toString;
 const symbolToString = Symbol.prototype.toString;
 
+const isWindow = val => typeof window !== 'undefined' && val === window;
+
 const SYMBOL_REGEXP = /^Symbol\((.*)\)(.*)$/;
 const NEWLINE_REGEXP = /\n/gi;
 
@@ -224,7 +226,7 @@ function printComplexValue(
         '}';
   }
 
-  return hitMaxDepth
+  return hitMaxDepth || isWindow(val)
     ? '[' + (val.constructor ? val.constructor.name : 'Object') + ']'
     : (min ? '' : (val.constructor ? val.constructor.name : 'Object') + ' ') +
       '{' +

[SimenB]

I suppose you can cache the typeof check, but that doesn't seem like an optimization

[pedrottimark]

Yes indeed, typeof window works in my experimental codes where window throws.

Thank you very much, I will update pull request according to your one liner above.

[pedrottimark]

Performance value in each cell is geometric mean of time for 3 runs:

scenario	baseline	try-catch first call	try-catch every call
basic	4605	4552	4529
complex	260972	262152	403992
massive	26163800	26739005	32063603

Interpretation

• basic is not affected by this change so is our control for precision of measurement
• complex includes objects among arrays, maps, sets, and so on
• massive formats one object from 256K of JSON

Simpler code for try-catch every call is slower enough that I did not commit the following change:

try {
  /* global window */
  return val === window;
} catch (e) {
  return false;
}

[pedrottimark]

Segmentation fault on node 4 :( how high is the chance that these changes caused it?

[SimenB]

how high is the chance that these changes caused it?

Next to nil, I've seen it quite a few times before. Restarted the build

[RwwL]

I'm sorry to comment on such an old, already-merged PR, not even sure if people in the discussion will get notifications, but I'm stuck on something, this is the only relevant-looking result I've been able to find, and it made me wonder about the possibility of a regression with what's discussed above.

I have a super-contrived test, myTestFile.js, using @jest-environment node:

/**
 * @jest-environment node
 */

describe('node env test', () => {
  it('runs a test', () => {
    expect('foo').toBe('foo');
  });
});

And running it yields me this:

 FAIL  __tests__/myTestFile.js
  ● Test suite failed to run

    ReferenceError: window is not defined

      at Object.<anonymous> (node_modules/pretty-format/build-es5/webpack:/prettyFormat/webpack/universalModuleDefinition:10:2)

If I remove the @jest-environment, the test runs and passes.

I'm using Jest 24.9.0 here.

[github-actions]

This pull request 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.