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

Print todo and skip descriptions when in verbose mode #8038

Merged
merged 12 commits into from Mar 19, 2019

Conversation

natealcedo
Copy link
Contributor

@natealcedo natealcedo commented Mar 4, 2019

Summary

Prints the text in test.todo and test.skip calls when in verbose mode

Fixes #7999

Test plan

Added some integration integration tests under testTodo

summedTests.todo.forEach(todoTest => {
this._logTodoTest(
'todo',
this._getIcon('todo'),
Copy link
Member

Choose a reason for hiding this comment

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

seems silly to pass in the icons and title, but I see it's consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Didn't want to stray too far from what was being done. The same could be said for printing skipped tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I think given that we are explicitly pulling this behaviour out into _logTodoTest we should just move the concrete data into the function definition.

 private _logTodoTest(
    todoTest: TestResult.AssertionResult,
    indentLevel: number,
  ) {
    const text = chalk.dim(`todo ${todoTest.title}`);
    this._logLine(`${this._getIcon('todo')} ${text}`, indentLevel);
  }

Then if you want to be fancy you could curry this to:

 private _logTodoTest = (indentLevel: number) => (todoTest: TestResult.AssertionResult) => {
    const text = chalk.dim(`todo ${todoTest.title}`);
    this._logLine(`${this._getIcon('todo')} ${text}`, indentLevel);
  }

and then you can just do the following in the forEach:

summedTests.todo.forEach(this._logTodoTest(indentLevel));

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 like this approach. Should we do a similar approach to skipped tests then since their functions are very similar?

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! Just missing changelog

@SimenB SimenB requested a review from thymikee March 4, 2019 13:35
@SimenB SimenB requested a review from mattphillips March 4, 2019 14:44
@natealcedo
Copy link
Contributor Author

natealcedo commented Mar 5, 2019

I've just pushed in some changes where skipped tests get the same treatment as per the original PR. One thing to note is that I wrap the description in quotes now since some descriptions might be long.
Let me know what you guys think.

Edit: I'll revert adding the quotes since I just noticed that they're not there for passing tests as well

@natealcedo natealcedo changed the title Print todo descriptions when in verbose mode Print todo and skip descriptions when in verbose mode Mar 7, 2019
@natealcedo
Copy link
Contributor Author

Hey just following up. What else does this PR need to get merged?

@SimenB
Copy link
Member

SimenB commented Mar 19, 2019

What else does this PR need to get merged?

Nothing!

@SimenB SimenB merged commit 809ab98 into jestjs:master Mar 19, 2019
@Akswii
Copy link

Akswii commented Apr 10, 2019

I know this is merged/closed, but is there anyway to list just the todos? When you have a lot of tests then showing it in verbose will be kind of hard to pickup.

@SimenB
Copy link
Member

SimenB commented Apr 10, 2019

We could probably pass an option to the reporter enabling this. Wanna open a PR? If not, please open up an issue

@natealcedo
Copy link
Contributor Author

I can pick this up. How would this work from a user’s point of view though? I imagine this would need another configuration option?

@SimenB
Copy link
Member

SimenB commented Apr 11, 2019

Yeah, something like reporters: [['default', {showTodoTitleWhenVerbose: true}]] or something similar, without the horrible name

@natealcedo
Copy link
Contributor Author

So I can foresee 2 things here.

  1. We only show the 'showTodoTitleWhenVerbose' when it's passed as an option in
  2. This behavior is not enabled by default when running jest with --verbose or when verbose: true is set.

That right?

@SimenB
Copy link
Member

SimenB commented Apr 11, 2019

Default to showing it, I think? /cc @scotthovestadt @thymikee @jeysal @mattphillips

@natealcedo
Copy link
Contributor Author

Ok I think I may have misread @Akswii 's request. Right now, in verbose mode. We print all test names. I think what he's asking is for the ability to show only the test names of the todo tests and everything else will be as per before. i.e. "3 skipped", Correct me if I'm wrong @Akswii

@natealcedo natealcedo deleted the feature/add-todo-test-description branch April 11, 2019 09:00
@Akswii
Copy link

Akswii commented Apr 11, 2019

@natealcedo Exactly! I wish to use test.todo to outline my future work load, and it would be a nice feature to be able to list only the todos. I'm working on a project with a ton of tests, and it's impossible to see what the todos are when the tests are running in verbose mode.

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

Show todo name/description in verbose mode
6 participants