Added compact print option for default reporter #7845

doniyor2109 · 2019-02-10T08:35:43Z

Summary

Added compact option to DefaultReporter so that it will prints only paths and titles.
Resolves #3322

Test plan

SimenB

Thanks! We can simplify how options are passed other than that this looks great!

We need an update to the changelog and an e2e test as well

packages/jest-cli/src/reporters/base_reporter.js

doniyor2109 · 2019-02-10T10:33:21Z

@SimenB It turns out verbose reporter extends from default reporter. I was thinking that should verbose reporter receive compact option too?

packages/jest-message-util/src/index.ts

SimenB · 2019-02-10T10:46:39Z

I was thinking that should verbose reporter receive compact option too?

Yeah, makes sense!

jeysal · 2019-02-10T11:01:45Z

When this lands, we should think about offering a way to activate this option via the CLI (not currently possible afaik) and/or add it to jest-watch-toggle-config.

SimenB · 2019-02-10T12:15:20Z

Currently no way of passing reporter options from the cli. Can do jest --config '{ "reporters": [ ["default", {"some":"option"}] ] }', but that's not really ergonomic

jeysal · 2019-02-10T12:16:57Z

I'll add this to #7185

SimenB

We need to document these options somewhere

packages/jest-cli/src/reporters/utils.js

packages/jest-cli/src/reporters/default_reporter.js

sunfit · 2019-02-11T16:44:55Z

I think this will not work properly after there are >20 test suites. Summary reporter will kick in and print all the diffs anyway.

Also what will the user need to do if he wants to know why those tests failed?
Is configuration like this possible?

  reporters: [
    "default",
    ["default", {compact: true}]
  ],

Or will rerunning tests without the compact option be required?

@cpojer denied a very similar PR by @jeysal a year ago because it required the user to rerun tests #5676 (comment)

@rickhanlonii was there an overview of proposed changes as you suggesetd in #3322 (comment)?

IMHO this should be implemented in SummaryReporter by removing the 20 test suite threshold and then printing failed test names without the diffs. Probably enabled by some option?

That way the order of output would be:

Default output which includes failed test diffs - so you can see why the tests failed
SummaryReporter output with a list of failed test names - quick overview of failed tests (what we want from this feature)
Little summary with number of failed tests and suites, elapsed time etc.

Currently SummaryReporter duplicates the diffs from DefaultReporter output, not sure how useful this is.

SimenB · 2019-02-11T17:46:10Z

I think a config option makes more sense than a CLI flag. We should pass the option to both verbose and default reporter.

doniyor2109 · 2019-02-14T16:01:41Z

@SimenB Currently there is no any built-in reporter that receives options. So shall we create separate page for them for documentation?

SimenB · 2019-02-14T16:45:59Z

A separate page for reporters would be awesome! Then we can go a bit in depth about the API and link to some cool community-made reporters (neither of which are needed for a first iteration, of course!)

SimenB · 2019-02-14T16:51:10Z

As an aside, on the "standard vs verbose" reporter thing - I think at some point we want to only allow a single reporter to actually print to stdout. And custom reporters would have to either implement the whole thing (e.g. by extending the built-in one) or export React components. The dream is to rewrite reporters to Ink (I have a working PoC here: https://github.com/SimenB/jest/tree/ink-reporter), but that doesn't work well with multiple reporters wanting control of process.stdout.

So at that point, "default jest reporter" would decide whether or not do print a summary, it wouldn't be decided by Jest by plugging in a different reporter (standard vs verbose).

However, that's future (and undecided) stuff!

EDIT: Since I was on a roll: #7900

sunfit · 2019-02-14T18:29:43Z

packages/jest-cli/src/TestScheduler.js

+        : new DefaultReporter(
+            this._globalConfig,
+            this._getReporterOption('default'),
+          ),


SummaryReporter (line 304) will still print diffs when there will be more than 20 test suites.

sunfit · 2019-02-14T18:49:29Z

@SimenB the previous PR was denied by @cpojer mainly because user had to rerun tests in order to see why they failed, not because it was implemented through CLI flag.

If I understand correctly it's is the same situation here, maybe even worse, because now in order to see the diffs you will need to rerun tests with a different jest config (without 'compact' enabled on default reporter). So you will need a separate npm script with jest --config='' and an additional jest config file.

If I am wrong here please explain why.

@doniyor2109 the full failure messages will still be printed by SummaryReporter when there will be more than 20 test suites. So your implementation fails to achieve its main goal which is to stop the failureMessage spam which makes it hard to understand what tests actually failed.

@rickhanlonii was there a proposed changes overview like you wanted in #3322 (comment)?

SimenB · 2019-02-15T12:35:09Z

Sorry about the conflict 😬 I've extracted the reporters to their own package instead of living inside jest-cli

I think the behavior we want is to get some sort of minimal output showing the errors. Maybe instead of the full expected blah, received blah with a stack, we just present the assertion in the normal reporter, and the summary reporter just printed the paths?

I'd like to take a small step back and frame this in a way that we only think about a single reporter (that behaves like the current way default + summary behaves), not separate reporters. How would we expect it to print the errors (compact or not)?

@pedrottimark you've worked a lot on clearer error messages lately, any thoughts on this?

packages/jest-reporters/src/verbose_reporter.ts

Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>

packages/jest-reporters/src/default_reporter.ts

packages/jest-core/src/TestScheduler.ts

Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>

SimenB

Sorry about the random driveby comments

CHANGELOG.md

e2e/default-reporter/package.json

packages/jest-test-result/src/index.ts

SimenB · 2019-03-20T10:53:43Z

We talked about this now and the way we want to do it is basically remove the error details from the summary reporter. So the summary reporter should just print names of the failed tests instead of the error details.

So this part should essentially not print failureMessage, just the ResultHeader.

https://github.com/facebook/jest/blob/afe22419327d2470bab9187e10fa153e2056bed9/packages/jest-reporters/src/summary_reporter.ts#L159-L185

IMHO this should be implemented in SummaryReporter by removing the 20 test suite threshold and then printing failed test names without the diffs. Probably enabled by some option?

I think the 20 limit still makes sense? No need to use space for a summary if we don't have a lot of output

I think this solves the issue of duplication and a lot of extra output that's basically duplication. Not sure if it makes sense as a default option since there might be thousands and thousands of tests, and having to scroll past the passing ones to find it might be annoying? Behind an option makes sense, though (compact is fine as a name).

@jeysal @thymikee @mattphillips agreed?

kolesan · 2019-03-20T12:15:00Z

As a user I would definitely want the 20 suite limit removed. Even 1 suite can have enough failing tests to blow up the output. And I believe the space would be used minimally and for relative information.

Honestly I came to a conclusion that just adding a new list of failing test names that would always be printed above the summary (the one with the numbers) and not changing anything else would be a good simple solution. Like @pedrottimark mentioned in his comments. It would solve all the cases without losing any usability and there would be no need for any extra config options.

EDIT: This is @sunfit, forgot i was logged in with a different github acc, sorry.

SimenB · 2019-03-20T13:29:09Z

As a user I would definitely want the 20 suite limit removed. Even 1 suite can have enough failing tests to blow up the output. And I believe the space would be used minimally and for relative information.

The 20 limit is for printing a summary at all, if it's less than 20 we output nothing (in the summary).

So I think we're in agreement? Summary should just be a list without details as the details will be in the normal output. Sorry if I misunderstood something

sunfit · 2019-03-20T13:39:45Z

The 20 limit is for printing a summary at all, if it's less than 20 we output nothing (in the summary).

That is the problem. If we output nothing then we won't output the list of failed test names.

For example I wished for this feature long before my project had 20 suites. About 10 diffs of failing tests already bloat the output so that a lot of scrolling is required to see the "big picture" - all the failing tests.

SimenB · 2019-03-20T13:50:22Z

Right, you always want the list? Gotcha. Maybe a listSummary: boolean option then?

sunfit · 2019-03-20T14:11:27Z

I think this behaviour is essential enough to be available by default, also the less options there are the better is for the user.

However I would be content with having it behind some option as well.

henhan · 2020-03-09T11:38:45Z

What happened with the work on this PR? Is it still relevant to complete? Or has this feature been completed in some other PR?

sunfit · 2020-03-09T18:51:28Z

What happened with the work on this PR? Is it still relevant to complete? Or has this feature been completed in some other PR?

No idea. Did you read the whole thread? The implementation should probably differ from what is in this PR.

rickhanlonii · 2020-03-24T19:45:21Z

Should this be a custom reporter instead, like jest-reporter-compact?

sunfit · 2020-03-24T20:12:45Z

The discussion about this went across a few tickets. Mainly here and in #3322

I was hoping for this behaviour to be available out of the box in jest.
Amount of interest and custom-reporters written for this suggests it is a popular desire.
Desire to have a compact list of failures at the end of test run, without so much visual noise from diffs.

Prefered order of things (as seen in the screenshot) is:

Diffs
Short summary (as a list)
Jests default summary with numbers

Screenshot is just an example to illustrate the order, some improvements are in order for the actual output. Failed tests should be at the bottom of the list for example (in case there are hundreds of passing suites), maybe passing suites should not be shown at all, or shown before the diffs, some kind of heading for the list summary, etc.

I could try to do a PR with this implemented behind an option or as default behaviour.
If the team would support this.

nickserv · 2020-11-10T20:06:59Z

Alternatively, would it be simpler to use the existing reporter but with a --dry-run option that doesn't run the tests?

github-actions · 2022-05-11T00:07:54Z

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.

Added compact print option for default reporter

f9f6c1a

facebook-github-bot added the cla signed label Feb 10, 2019

doniyor2109 added 2 commits February 10, 2019 13:36

Merge remote-tracking branch 'jest/master' into tests_summary

56094dc

Fix linter errors

55ec7a5

SimenB requested changes Feb 10, 2019

View reviewed changes

packages/jest-cli/src/reporters/base_reporter.js Outdated Show resolved Hide resolved

packages/jest-cli/src/reporters/base_reporter.js Outdated Show resolved Hide resolved

Minor fixes

96cf325

SimenB reviewed Feb 10, 2019

View reviewed changes

packages/jest-message-util/src/index.ts Outdated Show resolved Hide resolved

Added e2e tests

c2d41c7

doniyor2109 added 6 commits February 10, 2019 16:06

Fix bug when reporters is null

87816d1

Changelog

88a464c

Minor tweaks

e64bc67

Extends default reporter options for verbose reporter

d7158dd

Missing snapshot file

12cb996

Typo in test folder

6f594e2

SimenB requested changes Feb 10, 2019

View reviewed changes

packages/jest-cli/src/reporters/utils.js Outdated Show resolved Hide resolved

packages/jest-cli/src/reporters/default_reporter.js Outdated Show resolved Hide resolved

jeysal mentioned this pull request Feb 10, 2019

Simplifying configuration #7185

Open

Minor tweaks

24ab5d3

sunfit reviewed Feb 14, 2019

View reviewed changes

SimenB reviewed Mar 8, 2019

View reviewed changes

packages/jest-reporters/src/verbose_reporter.ts Outdated Show resolved Hide resolved

SimenB and others added 4 commits March 8, 2019 16:45

Tweaks

6e2edcd

Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>

Tweaks

509fdf1

Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>

Tweaks

503e85e

Tweaks

41fd5ad

SimenB reviewed Mar 8, 2019

View reviewed changes

packages/jest-reporters/src/default_reporter.ts Outdated Show resolved Hide resolved

SimenB reviewed Mar 8, 2019

View reviewed changes

packages/jest-core/src/TestScheduler.ts Outdated Show resolved Hide resolved

SimenB and others added 4 commits March 8, 2019 16:57

Tweak

ddf293b

Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>

private method

00a9f1c

Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>

Fix

eed3d51

Merge branch 'master' into tests_summary

f4e1d47

SimenB reviewed Mar 10, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

e2e/default-reporter/package.json Show resolved Hide resolved

packages/jest-test-result/src/index.ts Outdated Show resolved Hide resolved

doniyor2109 and others added 2 commits March 11, 2019 19:28

Tweaks

9c722a4

Merge branch 'master' into tests_summary

5d5eddb

thymikee requested a review from SimenB March 18, 2019 15:22

SimenB added 2 commits March 20, 2019 10:16

Merge branch 'master' into tests_summary

ed98e6c

move changelog entry

9143bd4

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added compact print option for default reporter #7845

Added compact print option for default reporter #7845

doniyor2109 commented Feb 10, 2019

SimenB left a comment

doniyor2109 commented Feb 10, 2019 •

edited

Loading

SimenB commented Feb 10, 2019

jeysal commented Feb 10, 2019

SimenB commented Feb 10, 2019

jeysal commented Feb 10, 2019

SimenB left a comment

sunfit commented Feb 11, 2019 •

edited

Loading

SimenB commented Feb 11, 2019

doniyor2109 commented Feb 14, 2019 •

edited

Loading

SimenB commented Feb 14, 2019

SimenB commented Feb 14, 2019 •

edited

Loading

sunfit Feb 14, 2019

sunfit commented Feb 14, 2019

SimenB commented Feb 15, 2019

SimenB left a comment

SimenB commented Mar 20, 2019

kolesan commented Mar 20, 2019 •

edited

Loading

SimenB commented Mar 20, 2019

sunfit commented Mar 20, 2019

SimenB commented Mar 20, 2019

sunfit commented Mar 20, 2019

henhan commented Mar 9, 2020

sunfit commented Mar 9, 2020

rickhanlonii commented Mar 24, 2020

sunfit commented Mar 24, 2020 •

edited

Loading

nickserv commented Nov 10, 2020

github-actions bot commented May 11, 2022

Added compact print option for default reporter #7845

Added compact print option for default reporter #7845

Conversation

doniyor2109 commented Feb 10, 2019

Summary

Test plan

SimenB left a comment

Choose a reason for hiding this comment

doniyor2109 commented Feb 10, 2019 • edited Loading

SimenB commented Feb 10, 2019

jeysal commented Feb 10, 2019

SimenB commented Feb 10, 2019

jeysal commented Feb 10, 2019

SimenB left a comment

Choose a reason for hiding this comment

sunfit commented Feb 11, 2019 • edited Loading

SimenB commented Feb 11, 2019

doniyor2109 commented Feb 14, 2019 • edited Loading

SimenB commented Feb 14, 2019

SimenB commented Feb 14, 2019 • edited Loading

sunfit Feb 14, 2019

Choose a reason for hiding this comment

sunfit commented Feb 14, 2019

SimenB commented Feb 15, 2019

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Mar 20, 2019

kolesan commented Mar 20, 2019 • edited Loading

SimenB commented Mar 20, 2019

sunfit commented Mar 20, 2019

SimenB commented Mar 20, 2019

sunfit commented Mar 20, 2019

henhan commented Mar 9, 2020

sunfit commented Mar 9, 2020

rickhanlonii commented Mar 24, 2020

sunfit commented Mar 24, 2020 • edited Loading

nickserv commented Nov 10, 2020

github-actions bot commented May 11, 2022

doniyor2109 commented Feb 10, 2019 •

edited

Loading

sunfit commented Feb 11, 2019 •

edited

Loading

doniyor2109 commented Feb 14, 2019 •

edited

Loading

SimenB commented Feb 14, 2019 •

edited

Loading

kolesan commented Mar 20, 2019 •

edited

Loading

sunfit commented Mar 24, 2020 •

edited

Loading