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

WIP: Make console.error and warn log a stack trace and codeframe like thrown errors do #9741

Merged
merged 31 commits into from
Apr 12, 2020

Conversation

ychi
Copy link
Contributor

@ychi ychi commented Apr 1, 2020

Summary

Make console.error and console.warn log a stack trace like thrown errors do. As requested in this issue. Visual examples:

Error used to be like this:
Screenshot 2020-04-04 at 18 11 19
It will become like this:
Screenshot 2020-04-04 at 18 06 22

What warn used to look like:
Screenshot 2020-04-04 at 18 10 46
And will become:
Screenshot 2020-04-04 at 18 05 57

There won't be stack trace for console.log, but for consistency, it's proposed to print like this:
Screenshot 2020-04-04 at 18 06 12
Instead of this:
Screenshot 2020-04-04 at 18 11 01

In the original feature request, there were discussion possibility to opt out. This can be achieved by exposing a new cli option --noCodeFrame.

Closes #8819

Test plan

To pass CI, some snapshots will need to be updated.

@facebook-github-bot
Copy link
Contributor

Hi @ychi!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@ychi ychi changed the title Put full stack trace in console buffer WIP: Make console.error and warn log a stack trace and codeframe like thrown errors do Apr 1, 2020
@ychi ychi force-pushed the stack-for-error-n-warn branch 2 times, most recently from 8c6c2d5 to 7fe4f36 Compare April 3, 2020 16:49
packages/jest-console/src/formatStackTrace.ts Outdated Show resolved Hide resolved
packages/jest-runner/src/runTest.ts Outdated Show resolved Hide resolved
packages/jest-source-map/src/getCallsite.ts Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ychi ychi requested a review from SimenB April 4, 2020 18:32
@ychi
Copy link
Contributor Author

ychi commented Apr 7, 2020

Hi @SimenB,

Can you take a look at the proposed output in the Summary above when you are available? I think many snapshot tests need to be updated accordingly. Please let me know which areas I can improve the code, too. Thank you very much!

@SimenB
Copy link
Member

SimenB commented Apr 8, 2020

Oh yeah, this is looking good! I like it 👍

Let's drop the --noCodeFrame option for now - easier to add if somebody complains than removing if nobody uses it 🙂

@SimenB
Copy link
Member

SimenB commented Apr 8, 2020

@gaearon thoughts on the output here?

@SimenB
Copy link
Member

SimenB commented Apr 9, 2020

I think this is shaping up great @ychi! 👍 Could you add some tests, update the snapshots and add an entry to the changelog?

@ychi ychi force-pushed the stack-for-error-n-warn branch from a4f6158 to 6d4322e Compare April 9, 2020 21:14
"chalk": "^3.0.0",
"jest-util": "^25.3.0",
"jest-message-util": "^25.2.6",
"jest-types": "^25.2.6",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"jest-types": "^25.2.6",
"@jest/types": "^25.3.0",

and at the top.

jest-util is also the wrong version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reminding about this! dependencies in package.json should always use package name while references in tsconf.json would be relative path.

By the way, what's the reason some package names are scoped while others are not? Is it because packages published in the early days and not wanting to change package name?

Copy link
Member

Choose a reason for hiding this comment

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

We got the jest namespace pretty late, and to avoid confusion we haven't renamed any packages. So in general old packages are not scoped, but new ones are scoped

@SimenB SimenB requested a review from jeysal April 12, 2020 16:41
@SimenB SimenB merged commit c830517 into jestjs:master Apr 12, 2020
@SimenB
Copy link
Member

SimenB commented Apr 12, 2020

Thanks @ychi!

@ychi
Copy link
Contributor Author

ychi commented Apr 12, 2020

Oh! So much going on in one day. Thank you very much @SimenB for helping with so many things in this PR!

@ychi ychi deleted the stack-for-error-n-warn branch April 12, 2020 19:59
@kentcdodds
Copy link
Contributor

Hooray!!! Thank you @ychi and @SimenB and everyone else 👏👏👏👏

@Hypnosphi
Copy link

@SimenB Sorry, but I'm not interested in those stacktraces, they only add noise in my case. Can you please reconsider adding noCodeFrame CLI and config option?

@SimenB
Copy link
Member

SimenB commented Apr 20, 2020

We have a --noStackTrace flag, but I guess you still want the stacks for actual errors?

@Hypnosphi
Copy link

Hypnosphi commented Apr 20, 2020

Generally, I do, but I think I'll try to live with this option a bit, thanks.

UPD: it doesn't seem to affect these console stacktraces

@SimenB
Copy link
Member

SimenB commented Apr 20, 2020

Oh, that's a bug I think

@ychi
Copy link
Contributor Author

ychi commented Apr 21, 2020

I can look into the bug later today :)

@ychi
Copy link
Contributor Author

ychi commented Apr 21, 2020

It's because getStackTraceLines() always return at least the first line, if noCodeFrame is not explicitly set to true, this if condition will still be evaluated to true, therefore rendering the whole callsite.

One way I can see would be like this:

if ((!options.noStackTrace) && (!options.noCodeFrame)) {
    ...
}

In which --noStackTrace flag will disable code frame.

Should we go with this route? Would it be suitable for the next minor release?

@SimenB
Copy link
Member

SimenB commented Apr 21, 2020

It should respect noStackTrace as well, yeah

@FredCoen
Copy link

hi,

in my case it adds noise as well. How do i get rid of console errors and warn?

Thanks

@kentcdodds
Copy link
Contributor

If there are errors/warn in your console, you should find where those are appearing (which this new feature should help with) and why, and change your code so those don't show up. It's dangerous to simply hide errors and warnings. They probably exist for a reason and are indicative of a problem.

@FredCoen
Copy link

Im aware of that but i want to have the option to not include them in my tests. dont want to change my source code for that..

@kentcdodds
Copy link
Contributor

If you want to get rid of the stack trace, then it sounds like the next version will support that with noStackTrace. Or is it that you don't want to see any of the error or warn logs at all? If that's the case, you can add this to the top of your tests or in a setupFilesAfterEnv file:

console.error = () => {}
console.warn = () => {}

I don't advise this for the reasons I mentioned earlier :)

@FredCoen
Copy link

thanks!

@Hypnosphi
Copy link

Why does it also apply to console.log and console.group?
https://github.com/facebook/jest/pull/9741/files#diff-b701d764ec6d4ad18067c4124271cd75

@ychi
Copy link
Contributor Author

ychi commented May 11, 2020

Call sites were moved to a different line for all console methods, while warn and error prints code frame and stack trace. Probably caused console.group to behave strangely, I can take a look this week.

@ychi
Copy link
Contributor Author

ychi commented May 16, 2020

Before v25.4.0, console.log and console group print like this:
Screenshot 2020-05-16 at 16 42 11
After v25.4.0, they print like this:
Screenshot 2020-05-16 at 16 35 05

The motivation was to make different console methods print in similar ways regardless of stack trace.

@Hypnosphi
Copy link

Hypnosphi commented May 16, 2020

I'd say that this IS a stacktrace, just reduced to one line. And it definitely doesn't help readability

@oleksii-burda
Copy link

Hi guys,
Is there any updates with --noCodeFrame option?
Actually I'm not interested in stack trace for console output, but I want the stacks for actual errors, that's why I cannot use --noStackTrace flag. And I see I'm not the only one :)

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

Make console.error (and warn?) log a stack trace and codeframe like thrown errors do
9 participants