pass testConsole to TestEnvironment #5227

kentcdodds · 2018-01-04T00:30:16Z

Summary

This ensures the console used in jsdom's virtualConsole is the same as the one used in the tests.

Test plan

I'm not sure of the best way to test this. Help appreciated :) I did make this work by manually changing the code in my node_modules of the reproduction repo and things worked as expected :)

Thanks!

codecov-io · 2018-01-04T00:40:57Z

Codecov Report

Merging #5227 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master    #5227   +/-   ##
=======================================
  Coverage   61.18%   61.18%           
=======================================
  Files         202      202           
  Lines        6765     6765           
  Branches        3        3           
=======================================
  Hits         4139     4139           
  Misses       2625     2625           
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-environment-jsdom/src/index.js	`40% <ø> (ø)`	⬆️
packages/jest-runner/src/run_test.js	`2.22% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cbc26b...d62e9ab. Read the comment docs.

cpojer · 2018-01-04T08:41:36Z

packages/jest-runner/src/run_test.js

@@ -98,6 +93,13 @@ async function runTestInternal(
    testConsole = new BufferedConsole();
  }

+  const environment = new TestEnvironment(
+    Object.assign({}, config, {console: testConsole}),


Can you split this up into separate options and pass two arguments instead? Let's not add the console to the main config object of Jest.

Didn't we add an environmentOptions recently? Can we reuse that?

I thought you might ask that (proof: watch the video on my twitch, I live streamed myself making this pr 😄). But decided against it because I thought that an other argument might be less desirable considering the TestEnvironment can be many things, not just the jsdom one. However, if there's an environmentOptions thing already, I'm assuming that would be an object as a second argument. If so, I don't think that's currently part of the type signature of a test environment constructor, but I can definitely add it and it makes more sense to me. Just let me know what you'd prefer :)

Here's the PR: #5003

It's still on the config though, so I guess @cpojer's original objection still stands

Ooooh, yeah, that's how my workaround works. I'm fine to do it either way. Though I think an extra argument might be better because using testEnvironmentOptions to add a virtualConsole would only make sense if the test environment is the jsdom one...

We cannot add this as a config option.

cpojer · 2018-01-04T08:42:58Z

packages/jest-runner/src/run_test.js

+  const leakDetector = config.detectLeaks
+    ? new LeakDetector(environment)
+    : null;
+
  const cacheFS = {[path]: testSource};
  setGlobal(environment.global, 'console', testConsole);


Are you saying that this function doesn't work anymore in the latest version of jsdom? Can we just fix that instead (using Object.defineProperty?).

I'd prefer using the offered API rather than trying to patch it. The way it's done here is a bit ad hoc though. Saying that all custom envs must attach console themselves is pretty breaking, so not sure about best way forward

The problem is that VirtualConsole gets called with sendTo and is passed the console object and holds a reference to that (calling it anyConsole) rather than referencing global.console.

I don't understand how either of your comments relate to my question about the setGlobal call.

Mine is: "JSDOM has an API for setting the console, we should use it rather than just assigning to the global".

Why it stopped working I don't know, though

@SimenB but then it would only work for jsdom and every other environment will have to do something different.

I'll provide a link when I get to my computer. But I hoped my comment above would explain why the current solution wouldn't ever work...

So, it's not a regression. It just never appeared before now. It happened now because of the changes in React. Look closer at my original issue and I explain.

When JSDOM is initialized, if we don't provide our own implementation of VirtualConsole, then it uses one which references the global console:

https://github.com/tmpvar/jsdom/blob/8a6894c34f14b9131d0fe4acadb71dae4f49daca/lib/api.js#L291-L293

This is all happening outside the test environment, so using setGlobal wouldn't work anyway. On top of that, the sendTo method called accepts an argument called anyConsole. It uses anyConsole.error instead of console.error. That's here in the VirtualConsole code:

https://github.com/tmpvar/jsdom/blob/5bc6b3bbcc0f18eccecca551bfcfe7d6bbe3e2ba/lib/jsdom/virtual-console.js#L14-L33

This is why we need to initialize JSDOM with our own instance of VirtualConsole. One which uses the testConsole rather than the global console.

I hope that makes sense.

Anyway, I'm fine adding this as an additional parameter to the TestEnvironment constructor, but because of how JSDOM constructs a VirtualConsole with a reference to console rather than using the global console, we do have to provide our own that references the right console.

What would you prefer?

cpojer · 2018-01-04T08:43:28Z

Thanks for submitting a fix. Can you check out the integration tests folder and add a test there to confirm that this is working properly again?

SimenB · 2018-01-04T11:14:39Z

Why not use https://github.com/tmpvar/jsdom/blob/master/README.md#virtual-consoles?

EDIT: which is what this PR is doing, that's what I get for just reading the comments and not the diff

kentcdodds · 2018-01-04T14:46:15Z

I'll be able to make changes in a few hours 👍 Thanks for the quick feedback!

SimenB · 2018-01-04T15:10:28Z

I think we should strive to make it work for people doing

class MyEnv extends JestTestEnvironment {
    constructor(config) {
        super(config);
    }
}

And not just those doing

class MyEnv extends JestTestEnvironment {
    constructor(...args) {
        super(...args);
    }
}

And maybe try to make environmentOptions be a second argument in the next major? Then we can do more with it as well. I think that maybe it should be the environment's own responsibility to inject globals from jest, instead of us overriding them after instantiating the env.

kentcdodds · 2018-01-04T18:28:33Z

Alrighty, I just pushed up an integration test. Hopefully that will help clear up any confusion. If you try to add this integration test to master without my changes, it will fail because console.error is never called, so the toHaveBeenCalledTimes(1) will fail. However, the console.error referenced in the test is not the same as the one that JSDOM is using, which is why my changes are necessary.

kentcdodds · 2018-01-04T20:34:15Z

If you've not had a chance to run the example repo, here's a build of the downshift project where it shows the problem. Some of our tests assert that validation errors are thrown during render and despite mocking console.error we still see all the error output from jsdom.

SimenB · 2018-01-04T21:12:59Z

Getting this merged would probably fix the logging I found when testing #4400 (comment) as well.

kentcdodds · 2018-01-04T21:23:12Z

Yes, that stack trace is familiar 😉

...
   at reportException (/Users/simbekkh/repos/framsie-dashboard/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:66:24)
...

I think this will fix that. It wont hide it mind you, it'll just allow you to mock the implementation.

SimenB

I think this makes sense as the only way to pass through a console without making a breaking change to the api of testEnvironments.
I think we should look into how we can pass testEnvironment config in general, separately from this PR.

kentcdodds · 2018-01-04T21:24:48Z

Note: if we do want to hide the logs, then we can do this:

new VirtualConsole().sendTo(config.console || console, {
// this config will prevent the JSDOM errors from being logged
  omitJSDOMErrors: true,
}),

But I don't think that's what we want.

kentcdodds · 2018-01-04T21:25:51Z

I agree with @SimenB.

In addition, the config is not change necessarily. We're simply passing a copy to the TestEnvironment constructor that has the console property on it. Shouldn't break anything existing.

SimenB · 2018-01-04T21:38:32Z

But I don't think that's what we want.

I think we might want to, as we listen to error ourselves. You can see the filtered stack below. Maybe we only get that when there are emitted errors instead of all the time? It would be bad if we hid errors.

Anyways, this will make it part of jest's logging, so you can do --silent or something and hide it

kentcdodds · 2018-01-04T21:43:08Z

One problem I see with not hiding it is that now instead of console being called once like it will in the normal app/browser (by react) it'll be called twice, once by jsdom and then by the browser. So you're right, it might be a good idea to set omitJSDOMErrors to true. I can do that 👍

kentcdodds · 2018-01-04T21:44:03Z

Actually, on that note, we could make that the only change and not worry about forwarding on the test console to JSDOM. I'm not sure what situation would cause the JSDOM console to log though.

SimenB · 2018-01-04T21:52:24Z

I think our .on('error') is enough to pick up the errors that would get sent to the console using jsdomError. @domenic might be able to clarify? Is a new JSDOM().window.document.defaultView.on('error') enough to pick up all errors that would be emitted as jsdomErrors, or will we potentially hide some other errors by passing in {omitJSDOMErrors : true}?

Our code for it (from #4669 and #4767):
https://github.com/facebook/jest/blob/c12bce6595b08626b8654f3ddb50a58d33071f0c/packages/jest-environment-jsdom/src/index.js#L43-L67

If we do pass omit, but we have to listen ourselves, then we're back to having to forward it to the correct console.

SimenB · 2018-01-04T22:42:20Z

types/Config.js

@@ -210,6 +210,7 @@ export type ProjectConfig = {|
  cache: boolean,
  cacheDirectory: Path,
  clearMocks: boolean,
+  console?: Object,


can you just do console?: console? Flow has it typed out: https://github.com/facebook/flow/blob/e2325d0e216e0cc14d17f00fc96624b364a27902/lib/core.js#L812-L835

Hmmm... When I try that I get:

object type. Ineligible value used in/as type annotation (did you forget 'typeof'?)

I'm still pretty new to flow so maybe I'm doing something wrong... I did:

console?: console,

Is that wrong?

I'm really bad at flow myself :P Try the suggestion of console?: typeof console

Haha, that gives me a crazy amount of errors. Most of them look unrelated, but some are:

Error: packages/jest-runner/src/run_test.js:97 97: Object.assign({}, config, {console: testConsole}), ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object literal. This type is incompatible with the expected param type of 16: constructor(config: ProjectConfig): void; ^^^^^^^^^^^^^ object type. See: types/Environment.js:16 Property `console` is incompatible: 97: Object.assign({}, config, {console: testConsole}), ^^^^^^^^^^^ BufferedConsole. This type is incompatible with 213: console?: typeof console, ^^^^^^^^^^^^^^ object type. See: types/Config.js:213 Property `dirxml` is incompatible: 213: console?: typeof console, ^^^^^^^^^^^^^^ property `dirxml`. Property not found in. See: types/Config.js:213 97: Object.assign({}, config, {console: testConsole}), ^^^^^^^^^^^ BufferedConsole

🤷‍♂️

Heh, yeah that's a bit too strict I guess. Thanks for checking!

Yap, whether adding this property when it's passed over a worker or not doesn't really change the purpose of the type though. Another solution would be to use a union type but I definitely would prefer adding the console prop as an argument to the TestEnvironment constructor. Since we still have the setGlobal call, this should be backward compatible.

Sounds good. Do you think I should make it an object that has a console property? That would provide the flexibility to adding more properties if other test environments need something like this in the future.

anybody extending JSDOMEnvironment would then not get this improvement for free. Might be edge-casey enough to ignore, I guess.

^{Could add a // $FlowIgnoreMe :D}

EDIT: An object makes sense, so we can add other stuff later

Yeah, I think passing the "config" object together with an "options" object is my preferred solution here.

Sounds good. I'll do that tomorrow then. It's 1:30 AM where I am 😅 Thanks!

SimenB · 2018-01-04T22:44:07Z

integration_tests/console-jsdom/__tests__/console.test.js

+  fakeNode.dispatchEvent(evt);
+  window.removeEventListener('error', onError);
+
+  expect(console.error).toHaveBeenCalledTimes(1);


could do expect(console.error).toMatchSnapshot();, then you should see in the snapshot that 'this is an error in an event callback' is passed as well. Not needed, but even more explicit
If it contains the whole stack, don't bother

Yeah, it does. It's kinda big :)

domenic · 2018-01-04T23:27:09Z

new JSDOM().window.document.defaultView.on('error') enough to pick up all errors that would be emitted as jsdomErrors, or will we potentially hide some other errors by passing in {omitJSDOMErrors : true}?

window === window.document.defaultView does not have an .on() method, so I don't know what this will do.

If you mean addEventListener, no, jsdomErrors are fired for many other cases as well, besides JavaScript errors. E.g. failed CSS parsing or image loading.

kentcdodds · 2018-01-04T23:38:27Z

Thanks @domenic

Hmmm... Ok, I think that this is probably the best we can do...

Actually, my initial concern/annoyance was that when writing a test like this:

const React = require('react')
const ReactDOM = require('react-dom')

class ThrowingComp extends React.Component {
  render() {
    throw new Error('this is an error in react')
  }
}

beforeEach(() => {
  jest.spyOn(console, 'error')
  console.error.mockImplementation(() => {})
})

afterEach(() => {
  console.error.mockRestore()
})

test('fails', () => {
  expect(() => {
    ReactDOM.render(
      React.createElement(ThrowingComp),
      document.createElement('div')
    )
  }).toThrow()
  // What should "x" be?
  expect(console.error).toHaveBeenCalledTimes(x)
})

Initially I assumed that console.error should only be called once in this situation, but I guess there's no reason to expect it should only be called once. In fact this is not the kind of assertion we should be making anyway (testing implementation details etc.).

So that said, I'm totally fine with this as it is. The PR accomplishes the initial goal of making the console that JSDOM uses be the same one that our tests have access to in their environment. So we're good 👍

kentcdodds · 2018-01-05T18:48:51Z

Ok, I've made some changes. I think this is good to go :)

kentcdodds · 2018-01-05T18:51:56Z

CHANGELOG.md

@@ -24,6 +24,12 @@
 * `[docs]` Add documentation for .toHaveProperty matcher to accept the keyPath
  argument as an array of properties/indices.
  ([#5220](https://github.com/facebook/jest/pull/5220))
+* `[jest-runner]` test environments are now passed a new `options` parameter.


I moved these to the Features section because whether this is a bug or a new feature is debatable, so it made more sense as a feature to me.

kentcdodds · 2018-01-05T18:53:13Z

types/Environment.js

 declare class $JestEnvironment {
-  constructor(config: ProjectConfig): void;
+  constructor(config: ProjectConfig, options?: EnvironmentOptions): void;


I expect in the next major version we can make this a required argument, but thought it best to leave it as an optional argument for now. I default to an empty object in the jsdom environment.

SimenB

Looks great! Thanks for being patient with us!

cpojer · 2018-01-05T19:20:40Z

Awesome, thank you so much Kent!

thymikee · 2018-01-05T22:28:48Z

packages/jest-environment-jsdom/src/index.js

@@ -22,14 +23,17 @@ class JSDOMEnvironment {
  errorEventListener: ?Function;
  moduleMocker: ?ModuleMocker;

-  constructor(config: ProjectConfig) {
+  constructor(config: ProjectConfig, options: EnvironmentOptions = {}) {


Small nit: options?: EnvironmentOptions.

Good point. Actually, I'm curious to know why it's necessary to add type information to the constructor. Isn't it already defined in types/Environment.js?

I'm not so sure, but I guess it's for the ease of writing your own custom envs and updating flow-typed defs, as Jest doesn't expose flow types generated from its source.
But you're right that we should strive for easier type management; this is not ideal.

Happy to make a PR to add the ? 👍

@thymikee

As suggested by @thymikee in jestjs#5227 (comment)

@thymikee

As suggested by @thymikee in #5227 (comment)

mqliutie · 2018-04-27T08:20:06Z

@kentcdodds

import {VirtualConsole} from 'jsdom';
new VirtualConsole().sendTo(console, {
// this config will prevent the JSDOM errors from being logged
    omitJSDOMErrors: true,
})

Errors still appearing on the console

github-actions · 2021-05-12T16:10:08Z

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.

facebook-github-bot added the CLA Signed ✔️ label Jan 4, 2018

kentcdodds mentioned this pull request Jan 4, 2018

jsdom console is unmockable #5223

Closed

cpojer reviewed Jan 4, 2018

View reviewed changes

kentcdodds force-pushed the pr/jsdom-console branch 5 times, most recently from a901f36 to ab1bdd4 Compare January 4, 2018 18:26

SimenB approved these changes Jan 4, 2018

View reviewed changes

SimenB reviewed Jan 4, 2018

View reviewed changes

kentcdodds force-pushed the pr/jsdom-console branch from ab1bdd4 to b781c68 Compare January 5, 2018 18:48

pass testConsole to TestEnvironment

d62e9ab

kentcdodds force-pushed the pr/jsdom-console branch from b781c68 to d62e9ab Compare January 5, 2018 18:50

kentcdodds commented Jan 5, 2018

View reviewed changes

SimenB approved these changes Jan 5, 2018

View reviewed changes

cpojer merged commit e776fdd into jestjs:master Jan 5, 2018

kentcdodds deleted the pr/jsdom-console branch January 5, 2018 19:24

natenorberg mentioned this pull request Jan 5, 2018

Test errors since version 1.10.0 downshift-js/downshift#216

Closed

thymikee reviewed Jan 5, 2018

View reviewed changes

kentcdodds pushed a commit to kentcdodds/jest that referenced this pull request Jan 5, 2018

jest-environment-jsdom: add ? to options parameter

38ca926

As suggested by @thymikee in jestjs#5227 (comment)

kentcdodds mentioned this pull request Jan 5, 2018

jest-environment-jsdom: add ? to options parameter #5240

Merged

cpojer pushed a commit that referenced this pull request Jan 5, 2018

jest-environment-jsdom: add ? to options parameter (#5240)

8b518a4

As suggested by @thymikee in #5227 (comment)

kentcdodds mentioned this pull request Jan 10, 2018

Prevent logging all the JSDOM internal errors. #5267

Closed

mqliutie mentioned this pull request Apr 27, 2018

XXX.spec.js import sweetalert2 seem there are some error? #6078

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

pass testConsole to TestEnvironment #5227

pass testConsole to TestEnvironment #5227

Conversation

kentcdodds commented Jan 4, 2018 • edited Loading

codecov-io commented Jan 4, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Jan 4, 2018

SimenB commented Jan 4, 2018 • edited Loading

kentcdodds commented Jan 4, 2018

SimenB commented Jan 4, 2018 • edited Loading

kentcdodds commented Jan 4, 2018

kentcdodds commented Jan 4, 2018

SimenB commented Jan 4, 2018

kentcdodds commented Jan 4, 2018

SimenB left a comment

Choose a reason for hiding this comment

kentcdodds commented Jan 4, 2018

kentcdodds commented Jan 4, 2018

SimenB commented Jan 4, 2018

kentcdodds commented Jan 4, 2018

kentcdodds commented Jan 4, 2018

SimenB commented Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

SimenB Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Jan 4, 2018

kentcdodds commented Jan 4, 2018

kentcdodds commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

cpojer commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mqliutie commented Apr 27, 2018

github-actions bot commented May 12, 2021

kentcdodds commented Jan 4, 2018 •

edited

Loading

codecov-io commented Jan 4, 2018 •

edited

Loading

SimenB commented Jan 4, 2018 •

edited

Loading

SimenB commented Jan 4, 2018 •

edited

Loading

SimenB commented Jan 4, 2018 •

edited

Loading

SimenB Jan 5, 2018 •

edited

Loading

kentcdodds Jan 5, 2018 •

edited

Loading

SimenB Jan 4, 2018 •

edited

Loading