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

- NodeJS Console initialization fix #4157

Closed
wants to merge 2 commits into from

Conversation

nowherenone
Copy link

Summary
This is a fix for the problem when NodeJS is "swallowing" console output.
BufferedConsole is initializing Console module with constructor parameters that don't match with the expected ones.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4157   +/-   ##
=======================================
  Coverage   60.55%   60.55%           
=======================================
  Files         196      196           
  Lines        6777     6777           
  Branches        6        6           
=======================================
  Hits         4104     4104           
  Misses       2670     2670           
  Partials        3        3
Impacted Files Coverage Δ
packages/jest-cli/src/lib/buffered_console.js 7.69% <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 5b2b647...70fc59d. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented Jul 30, 2017

Can you go into more detail of what this issue is aimed at fixing, and also providing a test that this change fixes? I don't see right now how this PR makes sense, I'll need more context. Thank you.

@nowherenone
Copy link
Author

nowherenone commented Jul 31, 2017

@cpojer Sure. I've referenced the original issue 3 days ago - Jest 20.0.0.4 swallows the console output from an app that is being tested. Here is one more issue about the same problem, and here is another one

That is really frustrating problem reported by many users, but all those issues were just closed without any fixes. So I've proposed a fix that definitely solves it.

A test that covers this update should prove that console.log calls from a tested app (not from the Jest itself) are really make output to stdout. To be honest - I don't know how to makes this test without running Jest inside of another process that logs all stdout calls from it.

@Pyrolistical
Copy link

I wrote an infinite loop and jest doesn't output to console, but this should solve my issue as well

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

I'm fine with this change. Looks like stubbing stdout in constructor is unnecessary here. And console.logs et al. still call BufferedConsole.write

@cpojer
Copy link
Member

cpojer commented Aug 24, 2017

Alright, not so fast. Passing process.stdout can cause the console implementation in node to write directly to the output stream which we explicitly do not want. The constructor call isn't wrong, it is intentionally pushing data into the buffered stream. If you want everything to be printed in a test that is an infinite loop, for example, use --verbose.

@cpojer cpojer closed this Aug 24, 2017
@nowherenone
Copy link
Author

Well, @cpojer then I don't understand how it isn't wrong.

When you are extending Console - the first parameter of super should be a stream that takes the output. And in our case this buffered stream should bring the output to the console after all. But since there is a BufferedConsole.write call that has nothing to do with stdout - all console calls just getting pushed into this._buffer and nothing else happens after that.

So, basically, you are creating a Console, that is detached from stdout at all.

@cpojer
Copy link
Member

cpojer commented Aug 24, 2017

The buffer gets sent back to the Jest parent process, which will print it when it prints the test result.

@nowherenone
Copy link
Author

Thank you for the explanation, --verbose key had solved my case.
Please excuse me for the noise. :)

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants