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

getConsoleOutput receives global noStackTrace #10081

Merged
merged 11 commits into from
Jun 1, 2020

Conversation

ychi
Copy link
Contributor

@ychi ychi commented May 24, 2020

Summary

Although formatStackTrace acts according to StackTraceOptions, getConsoleOutput needs to take noStackTrace value in global config and use it to override the options passed to formatStackTrace. This will resolve the bug mentioned here

Test plan

Pass existing test cases. Adding new test case to ensure getConsoleOutput does override corresponding configuration.

@ychi
Copy link
Contributor Author

ychi commented May 26, 2020

@SimenB can you have a look when you are available 😃

@@ -14,15 +14,16 @@ import {
import type {ConsoleBuffer} from './types';

export default (
// TODO: remove in 26
// TODO: remove in 27
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed 26, I can follow closer and create PR when 27 is near.

@@ -121,6 +121,7 @@ async function runTestInternal(
// 4 = the console call is buried 4 stack frames deep
BufferedConsole.write([], type, message, 4),
config,
globalConfig.noStackTrace,
Copy link
Member

Choose a reason for hiding this comment

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

can we pass the whole globalConfig instead?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

Could you add an integration test as well? That way we can be 100% sure it works 🙂

@ychi ychi force-pushed the noStackTrace-for-getConsoleOutput branch from 96c4714 to ed13e4d Compare June 1, 2020 08:37
config: StackTraceConfig = {
rootDir: root,
testMatch: [],
},
globalConfig: Config.GlobalConfig,
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be optional until next major ☹️ Should create an issue we can add to the Jest 27 milestone so we don't forget (again)

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 created this one and can create a PR for it, please let me know if anything is missing.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect 👍

@@ -40,12 +43,12 @@ export default (
if (type === 'warn') {
message = chalk.yellow(message);
typeMessage = chalk.yellow(typeMessage);
noStackTrace = false;
noStackTrace = false || globalConfig.noStackTrace;
Copy link
Member

Choose a reason for hiding this comment

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

false || will always fallback

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 catching! updated in a following commit.

@@ -40,12 +44,12 @@ export default (
if (type === 'warn') {
message = chalk.yellow(message);
typeMessage = chalk.yellow(typeMessage);
noStackTrace = false;
noStackTrace = globalConfig ? globalConfig.noStackTrace : false;
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
noStackTrace = globalConfig ? globalConfig.noStackTrace : false;
noStackTrace = globalConfig?.noStackTrace ?? false;

import {formatStackTrace} from 'jest-message-util';
import getConsoleOutput from '../getConsoleOutput';
import BufferedConsole from '../BufferedConsole';
import {LogType} from '../types';
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
import {LogType} from '../types';
import type {LogType} from '../types';

'warn',
];

cases.forEach(logType => {
Copy link
Member

Choose a reason for hiding this comment

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

it.each?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks!

@SimenB SimenB merged commit 790fe2a into jestjs:master Jun 1, 2020
@ychi ychi deleted the noStackTrace-for-getConsoleOutput branch June 1, 2020 19:59
@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.

3 participants