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

[Feature] Add Summary Flag to reduce output #2703

Closed
wants to merge 1 commit into from

Conversation

kwelch
Copy link

@kwelch kwelch commented Jan 26, 2017

In effort to address #1872 and the need for minimal output, I suggest using a --summary option.

This will turn off the DefaultReporter leaving just the SummaryReporter. Error messages are maintained, this just removes the RUN | FAIL | PASS messages.

Here is a sample output using the summary flag with the built in tests.
screen shot 2017-01-26 at 12 08 28 am

I am not sure about those warnings, as I didn't touch any disconnect listeners I am unsure where that came from.

@facebook-github-bot
Copy link
Contributor

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 up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

@kwelch kwelch changed the title Added summary flag option to turn off DefaultReporter [Feature] Add Summary Flag to reduce output Jan 26, 2017
@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!

@@ -197,6 +197,11 @@ const options = {
description: 'Prevent tests from printing messages through the console.',
type: 'boolean',
},
summary: {
default: false,
description: 'Limits console output to only the test summary.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Limits console output to only include the test summary" maybe?

@@ -55,7 +55,7 @@ const NPM_EVENTS = new Set([
'postrestart',
]);

class SummareReporter extends BaseReporter {
Copy link
Member

Choose a reason for hiding this comment

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

lol seriously? :D

Copy link
Contributor

Choose a reason for hiding this comment

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

was that me again? :'(

Copy link
Contributor

Choose a reason for hiding this comment

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

Суммаре! 😆

@cpojer
Copy link
Member

cpojer commented Jan 26, 2017

Thanks for the PR. You need to make sure that Flow is happy, please.

I'm not sure if this is really helpful. This way you don't know what is going on at all. I'd prefer the --summary reporter to show the currently running tests for example or a progress bar or something. We could have a dot reporter that prints one "." for every passed test and a red "e" or "f" (or something else) for a failed test.

@kwelch
Copy link
Author

kwelch commented Jan 26, 2017

IMO, this is more valid as a CI reporter. I agreed that if you are watching the output in real time this is not helpful as you could wait several seconds before any output.

I can make an attempt at a progress reporter, but I would recommend exposing it through a different config value such as --reporter progress.

As for flow, I will look into that, this is the first time I used it and was not sure how errors would surface or how to fix them. I will reach out if needed.

@cpojer
Copy link
Member

cpojer commented Jan 26, 2017

I don't really think this PR provides value for CI. Jest only shows the failed test summary if more than 20 assertions failed. With your change, if less than 20 tests fail, the user will not be able to know which tests those were.

@kwelch
Copy link
Author

kwelch commented Jan 26, 2017

@cpojer

I did not have the understanding and agree this is no longer valid and I will close.

I will start heading down the path of a progress reporter.

Are you good with exposing it as a --reporter option? Valid options including: default, progress, or verbose.

Should this allow the ability of passing in a custom reporter?

@kwelch kwelch closed this Jan 26, 2017
@cpojer
Copy link
Member

cpojer commented Jan 26, 2017

@DmitriiAbramov knows a lot about reporters. I'd like to hear his perspective on this.

@aaronabramov
Copy link
Contributor

what i was thinking:

  1. we definitely need custom reporters, because every project usually has different requirements (because of their build pipeline, CI integration, etc...). and there's no universal way to report.
  2. Adding a cli flag for every variation seems too much.

i think the best way to approach it is to have a config option with a default value:

reporters: [
  'default',
  'summary'
]

later we can configure reporters:

reporters: [
  'default',
  'nyan-cat',
  ['jest-xunit-reporter', {outputFile: '/reports/jest.xml'}]
]

i wrote a quick prototype a while ago, but never had a chance to finish it #2115

cc @wanderley

@kwelch
Copy link
Author

kwelch commented Jan 26, 2017

I can look into getting it to the point of the former option and I will use the prototype as reference. I will also add the progress/dots reporter.

I think have the ability to select from built in reporters is a good start and custom reporters is that much simpler to put in place.

Edit
That PR looks to be in a agnostic place. I can try to pick that up and add the progress reporter there.

@Daniel15
Copy link
Contributor

Daniel15 commented Jan 26, 2017

IMO, this is more valid as a CI reporter.

For CI, it would be good to have machine-readable output. JUnit's XML format is the de-facto standard for this, and most CI systems have functionality to parse it. A pluggable way of configuring reporters (as @DmitriiAbramov alluded to above) would be the best way of handling it.

For what it's worth, Jasmine's reporters do seem to work with Jest today. We're actually using an AppVeyor reporter for Jest itself: #1602. I know @cpojer had plans to remove Jasmine one day, so I guess this wouldn't continue working forever, and we'd want to have something that's configurable before removing Jasmine from Jest.

@kwelch
Copy link
Author

kwelch commented Jan 26, 2017

I have the jasmine junit reporter working in my enterprise project and it reads in Jenkins with no issue. Next time I am on desktop I will post source.

It doesn't change the console output thought. my intent is to minimize the console output.

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

7 participants