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

pass testConsole to TestEnvironment #5227

Merged
merged 1 commit into from Jan 5, 2018

Conversation

@kentcdodds
Copy link
Contributor

commented Jan 4, 2018

Summary

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

Closes #5223

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

This comment has been minimized.

Copy link

commented Jan 4, 2018

Codecov Report

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

Impacted file tree graph

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

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

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

This comment has been minimized.

Copy link
@cpojer

cpojer Jan 4, 2018

Contributor

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.

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 4, 2018

Collaborator

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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 :)

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 4, 2018

Collaborator

Here's the PR: #5003

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 4, 2018

Collaborator

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@cpojer

cpojer Jan 4, 2018

Contributor

We cannot add this as a config option.

const leakDetector = config.detectLeaks
? new LeakDetector(environment)
: null;

const cacheFS = {[path]: testSource};
setGlobal(environment.global, 'console', testConsole);

This comment has been minimized.

Copy link
@cpojer

cpojer Jan 4, 2018

Contributor

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?).

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 4, 2018

Collaborator

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@cpojer

cpojer Jan 4, 2018

Contributor

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

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 4, 2018

Collaborator

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

This comment has been minimized.

Copy link
@cpojer

cpojer Jan 4, 2018

Contributor

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

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

@SimenB

This comment has been minimized.

Copy link
Collaborator

commented Jan 4, 2018

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 kentcdodds force-pushed the kentcdodds:pr/jsdom-console branch 5 times, most recently from a901f36 to ab1bdd4 Jan 4, 2018

@kentcdodds

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 4, 2018

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

@kentcdodds

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

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

SimenB approved these changes Jan 4, 2018

Copy link
Collaborator

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 4, 2018

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):

// Report uncaught errors.
this.errorEventListener = event => {
if (userErrorListenerCount === 0 && event.error) {
process.emit('uncaughtException', event.error);
}
};
global.addEventListener('error', this.errorEventListener);
// However, don't report them as uncaught if the user listens to 'error' event.
// In that case, we assume the might have custom error handling logic.
const originalAddListener = global.addEventListener;
const originalRemoveListener = global.removeEventListener;
let userErrorListenerCount = 0;
global.addEventListener = function(name) {
if (name === 'error') {
userErrorListenerCount++;
}
return originalAddListener.apply(this, arguments);
};
global.removeEventListener = function(name) {
if (name === 'error') {
userErrorListenerCount--;
}
return originalRemoveListener.apply(this, arguments);
};

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

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

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 4, 2018

Collaborator

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 4, 2018

Collaborator

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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

🤷‍♂️

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 4, 2018

Collaborator

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

This comment has been minimized.

Copy link
@cpojer

cpojer Jan 5, 2018

Contributor

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.

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 5, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 5, 2018

Collaborator

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

This comment has been minimized.

Copy link
@cpojer

cpojer Jan 5, 2018

Contributor

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 5, 2018

Author Contributor

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

fakeNode.dispatchEvent(evt);
window.removeEventListener('error', onError);

expect(console.error).toHaveBeenCalledTimes(1);

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 4, 2018

Collaborator

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 4, 2018

Author Contributor

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

@domenic

This comment has been minimized.

Copy link

commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

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 kentcdodds force-pushed the kentcdodds:pr/jsdom-console branch from ab1bdd4 to b781c68 Jan 5, 2018

@kentcdodds

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2018

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

@kentcdodds kentcdodds force-pushed the kentcdodds:pr/jsdom-console branch from b781c68 to d62e9ab Jan 5, 2018

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 5, 2018

Author Contributor

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.

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

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 5, 2018

Author Contributor

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

SimenB approved these changes Jan 5, 2018

Copy link
Collaborator

left a comment

Looks great! Thanks for being patient with us!

@cpojer cpojer merged commit e776fdd into facebook:master Jan 5, 2018

6 checks passed

ci/circleci: test-and-deploy-website Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-node-9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cpojer

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2018

Awesome, thank you so much Kent!

@kentcdodds kentcdodds deleted the kentcdodds:pr/jsdom-console branch Jan 5, 2018

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

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

This comment has been minimized.

Copy link
@thymikee

thymikee Jan 5, 2018

Collaborator

Small nit: options?: EnvironmentOptions.

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 5, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@thymikee

thymikee Jan 5, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Jan 5, 2018

Author Contributor

Happy to make a PR to add the ? 👍

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

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

@mqliutie

This comment has been minimized.

Copy link

commented Apr 27, 2018

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.