[WIP] Fix Console Logging in jsdom Env

[dairyisscary]

Summary
Jest currently supports great buffered output for printing to console (console.log, console.warn, etc) but this only works in the official node env. This PR aims to add this functionality to jsdom env too (very useful when debugging tests).

Current master monkey patches in the jest console with this line, but this does not work on the jsdom object for whatever reason:
const testConsole = env.global.console = new TestConsole(

This PR consists of two commits, the first being a (broken) test demonstrating the desired behavior with jsdom env. The second commit is a proposed solution which fixes the failing test.

I have changed it so the test environment is responsible for constructing the console rather than the higher context with the above assignment. I have provided a factory function to the test environment's constructor since it seemed unreasonable to me for the environment to know how to and to duplicate this configuration logic. I was looking for some design input on this since this would break existing API, which probably wouldn't be a big deal except that its currently a supported use case for user to configure their own test environment.

Test plan
I have broken a boatload of tests because of the API change but I didn't want to waste time fixing them all if there are disagreements on the design.

[thymikee]

I think we could avoid breaking change by making createJestConsole optional (I'd rename it to customConsole). You could extract createJestConsole to a separate module and use it as a default.

[dairyisscary]

I was more thinking of the higher level context being dependent on env.console. This change would require TestEnvironments to have a .console.getBuffer() if they want their output included with the results.

[thymikee]

You're assigning to this.console. Be sure to remove the references to this field in dispose() method, as this constructor is called for every test file (same for jest-environment-node) and may cause memory leak.

[dairyisscary]

Nice! Thanks. I've tightened up the types too.

[thymikee]

Thanks for this PR, looks really nice! Left some comments on things I would adjust. We'll need to wait for @cpojer to review it though.

[dairyisscary]

I would love to move this creator function out to its own module, allowing me to remove it from the signature of the environment constructor (why have it at all then?). But when I attempted to do this, the package lines/dependencies got in my way. The creator function is here created as part of jest-cli and has lots of dependencies on internal jest-cli stuff (like the BufferedConsole), so I would have to make this creator utility function there, but then I can't import it from the environment packages; users wishing to create their own environment would likely want public access to this utility function too.

For me to do this, I'd have to shuffle a lot of these other dependencies around. :)

[cpojer]

the variable should be "console". No abbreviations please!

[cpojer]

I think createConsole should be good enough of a name, it implies that it is "custom".

[cpojer]

I think I'd prefer moving this out of jest-cli into the appropriate packages so that the creator function is a stateless module rather than something we pass around.

Just updated to node 7.3 and it seems indeed that logging is broken in that version using jsdom. Ugh, that's annoying. Maybe it is a regression in node that needs to be fixed?

[italoacasas]

Hi everyone,

@cpojer if you have the time, can you create an Issue in the node core?, if no one finds what happen I will take care of the Issue tonight.

[cpojer]

@italoacasas thanks. I created an issue here: nodejs/node#10492 but I'm on vacation right now and don't have time to create a more succinct and detailed repro. :(

[italoacasas]

it's ok, don't worry, I think I have enough information. Thanks

[MylesBorins]

it looks like the commit that broke things was nodejs/node@524f693

the PR was nodejs/node#10227

While people are digging into this I'm going to look into adding jest to citgm so we can catch breakages like this earlier

edit: currently it does not look like there is any easy way for us to include jest in citgm due to a variety of things mostly inherited from the monorepo approach. Let me know if you want to go more in depth and we can discuss.

[domenic]

It seems like the Node fix is moving in the direction of a more correct vm module though, so either jest or jsdom is doing something weird. Let me know if it ends up being jsdom...

[dairyisscary]

I tried to figure out why jsdomObject.defaultView.console was non-assignable. In the jsdom source, its just a regular assignment, so nothing weird there I think.

When I ran this minimal script, assignment went just fine:

const jsdom = require('jsdom');
const ctx = jsdom.jsdom(undefined, {});
const global = ctx.defaultView;
console.log(global.console);
global.console = 'test';
console.log(global.console); // this prints test

BUT as soon as a I added 'use strict'; to the above, it printed the original object twice (non-assignable). For whatever reason, strict mode seems to break this.

Also, Object.getOwnPropertyDescriptor(global, 'console') reported that the property was writable in both jest source and in script above.

Also I see that the node commit above is for the vm module in node, but jest only uses this module for running a test with node environment, yes?

Edit: Oh I see, it looks like the jsdom function returns the document, not the window object and .defaultView is a proxy back to the window object. Maybe this is why node doesn't want to assign in strict mode?

[MylesBorins]

in v7.3.0 the re-assignment fails. This is definitely what is causing the issue. /cc @domenic

edit: we can alternatively set each of the individual methods in the console object

var jsdom = require('jsdom');
const ctx = jsdom.jsdom(undefined, {});
const global = ctx.defaultView;
console.log(global.console.log.toString());
global.console.log = 'test';
console.log(global.console.log); // this prints test

but I am not sure that is the exact functionality we want, we are also not guaranteed that other bits in jsdom are not broken by this change as well

[domenic]

So it sounds like the issue is that now, in strict mode code, assigning someGlobalObjectCreatedByVM.prop = value will make prop non-writable? That seems like a separate vm/Node bug then, probably one revealed by nodejs/node@524f693 (since before that commit it might be non-writable but vm was just ignoring that non-writability).

[cpojer]

I put up an alternative fix that's a bit simpler based on @dairyisscary's comments. It simply uses a non-strict module to set the console. This is a bit hacky and I expect this to break again in the future but it seems to me like this is a node/vm or jsdom issue as console should definitely be assignable or at least writable through Object.defineProperty, no?

[cpojer]

I realized this doesn't just expand to console but also applies to setTimeout, setImmediate and all other timer globals. I applied the same hacky fix for now to make sure Jest works again and will publish 18.1 shortly.

[cpojer]

@dairyisscary thank you so much for this PR and figuring out all these issues. I went ahead merging my own PR, which also fixes this issue for timers in node 7.3 and is a bit more minimal. Publishing 18.1 now but I'm hoping that this can be fixed in node or jsdom properly soon.

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