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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Currently this only has the `console` which is the test console that Jest will
expose to tests. ([#5223](https://github.com/facebook/jest/issues/5223))
* `[jest-environment-jsdom]` pass the `options.console` to a custom
instance of `virtualConsole` so jsdom is using the same console as the
test. ([#5223](https://github.com/facebook/jest/issues/5223))

### Chore & Maintenance

Expand Down
18 changes: 18 additions & 0 deletions integration_tests/__tests__/__snapshots__/console.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,21 @@ Snapshots: 0 total
Time: <<REPLACED>>
"
`;

exports[`the jsdom console is the same as the test console 1`] = `""`;

exports[`the jsdom console is the same as the test console 2`] = `
"PASS __tests__/console.test.js
✓ can mock console.error calls from jsdom

"
`;

exports[`the jsdom console is the same as the test console 3`] = `
"Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Snapshots: 0 total
Time: <<REPLACED>>
Ran all test suites.
"
`;
11 changes: 11 additions & 0 deletions integration_tests/__tests__/console.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,14 @@ test('does not print to console with --silent', () => {
expect(rest).toMatchSnapshot();
expect(summary).toMatchSnapshot();
});

// issue: https://github.com/facebook/jest/issues/5223
test('the jsdom console is the same as the test console', () => {
const {stderr, stdout, status} = runJest('console-jsdom');
const {summary, rest} = extractSummary(stderr);

expect(status).toBe(0);
expect(stdout).toMatchSnapshot();
expect(rest).toMatchSnapshot();
expect(summary).toMatchSnapshot();
});
40 changes: 40 additions & 0 deletions integration_tests/console-jsdom/__tests__/console.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
/* eslint-env browser */
'use strict';

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

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

test('can mock console.error calls from jsdom', () => {
// copied and modified for tests from:
// https://github.com/facebook/react/blob/46b3c3e4ae0d52565f7ed2344036a22016781ca0/packages/shared/invokeGuardedCallback.js#L62-L147
const fakeNode = document.createElement('react');

const evt = document.createEvent('Event');
const evtType = 'react-invokeguardedcallback';
function callCallback() {
fakeNode.removeEventListener(evtType, callCallback, false);
throw new Error('this is an error in an event callback');
}

function onError(event) {}

window.addEventListener('error', onError);
fakeNode.addEventListener(evtType, callCallback, false);
evt.initEvent(evtType, false, false);
fakeNode.dispatchEvent(evt);
window.removeEventListener('error', onError);

expect(console.error).toHaveBeenCalledTimes(1);
Copy link
Member

@SimenB SimenB Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
5 changes: 5 additions & 0 deletions integration_tests/console-jsdom/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"jest": {
"testEnvironment": "jsdom"
}
}
8 changes: 6 additions & 2 deletions packages/jest-environment-jsdom/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@

import type {Script} from 'vm';
import type {ProjectConfig} from 'types/Config';
import type {EnvironmentOptions} from 'types/Environment';
import type {Global} from 'types/Global';
import type {ModuleMocker} from 'jest-mock';

import {FakeTimers, installCommonGlobals} from 'jest-util';
import mock from 'jest-mock';
import {JSDOM} from 'jsdom';
import {JSDOM, VirtualConsole} from 'jsdom';

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit: options?: EnvironmentOptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to make a PR to add the ? 👍

this.dom = new JSDOM(
'<!DOCTYPE html>',
Object.assign(
{
pretendToBeVisual: true,
runScripts: 'dangerously',
url: config.testURL,
virtualConsole: new VirtualConsole().sendTo(
options.console || console,
),
},
config.testEnvironmentOptions,
),
Expand Down
10 changes: 5 additions & 5 deletions packages/jest-runner/src/run_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,6 @@ async function runTestInternal(
RuntimeClass,
>);

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

const consoleOut = globalConfig.useStderr ? process.stderr : process.stdout;
const consoleFormatter = (type, message) =>
getConsoleOutput(
Expand All @@ -98,6 +93,11 @@ async function runTestInternal(
testConsole = new BufferedConsole();
}

const environment = new TestEnvironment(config, {console: testConsole});
const leakDetector = config.detectLeaks
? new LeakDetector(environment)
: null;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


Expand Down
6 changes: 5 additions & 1 deletion types/Environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,12 @@ import type {Global} from './Global';
import type {Script} from 'vm';
import type {ModuleMocker} from 'jest-mock';

export type EnvironmentOptions = {
console?: Object,
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

runScript(script: Script): any;
global: Global;
fakeTimers: {
Expand Down