Custom reporter integration #3064

abdulhannanali · 2017-03-04T06:13:07Z

Summary
This PR adds the ability to specify custom reporter to be used with Jest. Right now, there's no way for the user to specify the custom reporter they want to use, without tinkering the internals. This let's them specify the Custom reporter(s) if any users want to use with their tests.

This PR relies heavily on the work @DmitriiAbramov did in #2115 for adding XUnitReporter. Right now, only the interface through which we can use different reporters has been developed. Reporters such as Nyan and XUnitReporter are work in progress.

This PR reuses

Test plan

Integration tests for custom reporters are work in progress.

abdulhannanali · 2017-03-04T06:15:19Z

packages/jest-config/src/validConfig.js

@@ -57,6 +57,9 @@ module.exports = ({
  noStackTrace: false,
  notify: false,
  preset: 'react-native',
+  reporters: [


@thymikee Would appreciate some feedback from you on this. There was ValidationError being thrown but after a certain number of tries when I added a sample reporters file here, it stopped throwing the error. Shouldn't adding it to types/Config.js be suffice

nope, types/Config.js holds types for Flow (which is stripped with babel for runtime). jest-validate checks for actual valid JS object, which in our case is validConfig.js.

Cool makes sense thanks

abdulhannanali · 2017-03-04T06:16:11Z

packages/jest-config/src/normalize.js

@@ -354,6 +354,9 @@ function normalize(config: InitialConfig, argv: Object = {}) {
      case 'persistModuleRegistryBetweenSpecs':
      case 'preset':
      case 'replname':
+      case 'reporters':
+        value = _replaceRootDirTags(config.rootDir, config[key]);


The previous PR #2115 adds a validation here in order to check the type of the options passed. But after jest-validate that doesn't seem to be necessary.

It is necessary, jest-validate only checks for your config to be valid against an example and throws otherwise, it doesn't modify anything.

@thymikee So, If you take a look at the #2115 it adds in a type validation here, which is doing Array.isArray to type check if it's an array, but do we really need that here. As I am getting a ValidationError just based on what's there in validConfig.js.

Here's the ValidationError, I think this error should be suffice. Do we need a custom error here.

● Validation Error: Option "reporters" must be of type: array but instead received: string Example: { "reporters": [["./here-it-goes.js", {"option1": true}]] } Configuration Documentation: https://facebook.github.io/jest/docs/configuration.html

abdulhannanali · 2017-03-04T06:18:15Z

The first custom reporter is going to be a Nyan Cat, as it's trivial to implement

abdulhannanali · 2017-03-04T07:37:31Z

@thymikee I am interested in knowing how we can add a better validation check for the Reporters being imported,

thymikee · 2017-03-04T07:31:52Z

packages/jest-cli/src/TestRunner.js

+            Unexpected custom reporter configuration.
+            Expected to get either a path string or an array of [path, confg]
+            got: ${JSON.stringify(entry)}
+          `);


This will be quite heavily indented

Yes I can see this dirsrupting the output badly. Instead of using a Custom Error message, Are there any functions in jest-validate specifically for this purpose?

i actually always use that kind of error messages and i don't mind them being so heavily indented.
maybe we can add a shared utility function that trims indentation to the minimum value of the block?
probably shouldn't be addressed in this PR tho

Yes we can have a function similar to what we are doing with createConfigError maybe. I'll open up another PR for this purpose, after some time.

@thymikee @DmitriiAbramov Ideally will this be added to jest-validate or I guess Validate can be used here, since it's kind of a validation error.

It would be great if we could extend validation possibilities. Right now it's indeed pretty limited.

There is a module on npm for this purpose called dedent. The code style Jest uses here is to separate multiline strings by using string concatenation. Please follow this style here.

@cpojer Will do that however, it's going to be a temporary fix, as we need this to be in jest-validate ideally.

thymikee · 2017-03-04T07:35:10Z

packages/jest-cli/src/TestRunner.js

@@ -379,6 +379,10 @@ class TestRunner {
  _setupReporters() {
    const config = this._config;

+    if (config.reporters) {
+      return this._addCustomReporters(config.reporters);


So when you setup any custom reporter, you're getting rid of all default ones, like DefaultReporter, CoverageReporter, SummaryReporter? I'm not happy with that

Ooopsie! Let me change this. It was added there for testing purposes. Thanks for pointing it out.

although in some cases that might be a desired behavior. I'm not really sure how to go about that. maybe add another option noDefaultReporters: true?

@DmitriiAbramov Makes sense will add this.

@DmitriiAbramov The configuration open noDefaultReporters: true has been added. One concern here is, Should this option only perform the desired behavior when it's used in tandem with config.reporters and otherwise give a warning maybe? Or should it work regardless of config.reporters?

I have added the first iteration of this which used regardless of reporters option specified or not.

thymikee · 2017-03-04T07:42:48Z

packages/jest-config/src/validConfig.js

@@ -57,6 +57,9 @@ module.exports = ({
  noStackTrace: false,
  notify: false,
  preset: 'react-native',
+  reporters: [


nope, types/Config.js holds types for Flow (which is stripped with babel for runtime). jest-validate checks for actual valid JS object, which in our case is validConfig.js.

abdulhannanali · 2017-03-04T21:49:38Z

@DmitriiAbramov In a Test Suite, Can we consider the the fullPath of the test as the value for the classname and package attribute within tescase and testsuite tag.?

abdulhannanali · 2017-03-04T23:20:17Z

Not adding it in this PR as @DmitriiAbramov suggested, but here's the initial version of XUnitReporter written to be used a Custom Reporter. Any feedback is welcomed. https://gist.github.com/abdulhannanali/6560aa5f53e65ca3cf92fb9ad0836d02 . I was going to push this too, but thanks to @DmitriiAbramov for pointing it out that it should be a separate PR.

noDefaultReporters option let's user turn off all the reporters set by default

abdulhannanali · 2017-03-06T04:39:09Z

packages/jest-cli/src/__tests__/TestRunner-test.js

+// Checking for the method _addCustomReporters
+// Custom reporters used here are the reporters within the package
+// No extra reporters are included to be used
+describe('_addCustomReporters', () => {


@thymikee Hey! I am new to testing. Wrote some unit tests to check the integration. Would you mind giving it a review?

abdulhannanali · 2017-03-06T04:43:16Z

packages/jest-cli/src/__tests__/TestRunner-test.js

+      runner._addCustomReporters(reporters);    
+    };
+
+    expect(addInvalidReporters).toThrow();


Calling this passes the test, but at the same time throws an error too, because of the require calls that are done. Is there any way I can avoid this?

you can create a mock file and require it

@DmitriiAbramov But then the error which is currently thrown won't be thrown. We need to check if the error is thrown. I am doubtful at this point, if really need to test for this, as here we are checking if the require is going to throw invalid error.

@DmitriiAbramov Sorry! I think this test is way too ambiguous these aren't invalid but non existent reporters. Should be more clearer. Removing this test for now.

However, I still would like to clarify, if Mock file is going to be helpful in testing for the case where we are checking if file exists or not.

abdulhannanali · 2017-03-06T04:55:09Z

packages/jest-cli/src/__tests__/TestRunner-test.js

@@ -38,6 +38,115 @@ test('.addReporter() .removeReporter()', () => {
  expect(runner._dispatcher._reporters).not.toContain(reporter);
 });

+describe('noDefaultReporters option', () => {


Adding test for noDefaltReporters option. As I am using this same option for testing the reporters in the next describe function block too.

abdulhannanali · 2017-03-06T04:55:36Z

packages/jest-cli/src/__tests__/TestRunner-test.js

+  let runner;
+
+  beforeEach(() => {
+    runner = new TestRunner({}, {noDefaultReporters: true}, {});


Using noDefualtReporters here to make the addition of tests easier.

abdulhannanali · 2017-03-06T06:03:19Z

One of the major concern I need to clear out is how the path resolution for the reporters will work. I am currently working on Integration Tests for Custom reporters and for this purpose I am writing a simple reporter, which just checks if the whole integration of reporters work fine.

abdulhannanali · 2017-03-06T07:33:00Z

@DmitriiAbramov The Reporters in the Jest extend from a BaseReporter, but in Mocha the biggest difference is that the BaseReporter is responsible for keeping track of the whole progress of the tests within it's stats object while the BaseReporter within the Jest is just exposing two more methods and removing the preRunMessage.

We can even remove the BaseReporter. However, it has a few advantages, as the code in the TestRunner would break if the reporters don't implement all of the methods on the base reporter. Also, we can additionally check in the code if the reports are inheriting from BaseReporter, adding kind of a validation check, but we definitely need a way to expose BaseReporter to the application.

cpojer · 2017-03-06T13:07:48Z

packages/jest-cli/src/TestRunner.js

-    this.addReporter(
-      config.verbose ? new VerboseReporter(config) : new DefaultReporter(),
-    );
+


please don't add more than two newlines at a time :)

Maybe worth adding? http://eslint.org/docs/rules/no-multiple-empty-lines

+1 to no multiple empty lines in a separate PR!

👍 for no-multiple-empty-lines like in StandardJS..

@abdulhannanali mind sending a separate PR that adds that lint rule? :)

@cpojer Definitely thanks a lot! 👍

cpojer · 2017-03-06T13:08:09Z

.vscode/settings.json

-  ]
+],
+"eslint.enable": true,
+"javascript.validate.enable": false


hmmm.. should we really add this here? Shall we do it in a separate PR?

This is a big mistake. I get this committed with each PR I do, as this is part of my development workflow. Sorry! Will remove it.

cpojer · 2017-03-06T13:08:32Z

packages/jest-cli/src/TestRunner.js

+
+    // Default Reporters are setup when
+    // noDefaultReporters is false, which is false by default
+    // and can be set to true in configuration.


I think this comment is superfluous as it describes what the if statement does. Mind removing it?

Yes, need to get over this habit.

Comments that describe what statements does are superfluous. Noted! Will remove them!

cpojer · 2017-03-06T13:11:46Z

packages/jest-cli/src/TestRunner.js

+            ${JSON.stringify(reporterPath)}
+          Config:
+            ${JSON.stringify(reporterConfig)}
+        `);


Can we print this similarly to how we are already printing warnings/errors, with colors etc? I don't think you need JSON.stringify for the reporterPath, for example.

this error is not letting me lint 100% safe

abdulhannanali · 2017-04-05T01:48:27Z

website/core/center.js

@@ -9,11 +9,11 @@ const assign = require('object-assign');

 const center = React.createClass({
  render() {
-    let {style, ...props} = this.props;
-    style = assign({}, style, {textAlign: 'center'});
+    const {style, ...props} = this.props;


Doing this to get all the code linted, this is an eslint error as we were previously reassigning to style.

abdulhannanali · 2017-04-05T02:25:33Z

@DmitriiAbramov @cpojer I have pushed the working version containing the tests. The custom reporters right now are reusing much of the Reporter functionality Jest already has for it's own reporters. The API to implement custom reporters is exactly the same. It's kind of really providing the hook with which you can add your own custom reporter.

In order for the reporters to confirm to the standard, I implemented a _validateReporters method which validates the reporter by checking if various methods are implemented for sure, but since this makes it very unflexible I removed it.

I would like to know any other suggestions, I have removed the [WIP] prefix from this PR too.

Thank you for your help everyone ❤️

abdulhannanali · 2017-04-05T02:25:57Z

Circle CI build is failed for some weird yarn reason, I have nothing to do with

bookman25 · 2017-04-05T23:17:00Z

Does this allow for reporters being passed in via the CLI? It didn't look like it, but I might have missed it. For jest-editor-support it would a big benefit to have CLI support for the reports arg.

abdulhannanali · 2017-04-11T10:16:42Z

@bookman25 As discussed before, this PR will only add the capability for the jest-config. A separate PR should be opened if we want to add the capability to CLI too. However, I haven't given a lot of thought to it, right now, even though Karma and Ava already does it.. Will think about it once this one is merged.

…config

aaronabramov · 2017-04-22T06:24:10Z

closing in favor of #3349

github-actions · 2021-05-13T18:15:32Z

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.

abdulhannanali added 4 commits March 4, 2017 02:29

add custom reporters option in TestRunner

98d01b5

add reporters option in jest-cli config

1215c0b

add flowtype for reporters option

b3196ac

add key for reporters in validConfig

44f6550

facebook-github-bot added the CLA Signed ✔️ label Mar 4, 2017

abdulhannanali commented Mar 4, 2017

View reviewed changes

thymikee reviewed Mar 4, 2017

View reviewed changes

abdulhannanali added 3 commits March 5, 2017 05:12

add noDefaultReporters option

aa548c9

noDefaultReporters option let's user turn off all the reporters set by default

Lint

268a640

add unit tests for _addCustomReporters

e79ae0c

nickytonline mentioned this pull request Mar 6, 2017

Update Jest Reporter to output single line report coryhouse/react-slingshot#361

Open

abdulhannanali added 2 commits March 6, 2017 09:30

separate default reporters in method in TestRunner

2ee53ce

add tests for errors which are thrown

073d6e9

abdulhannanali commented Mar 6, 2017

View reviewed changes

add tests for .noDefaultReporters

6a430de

abdulhannanali commented Mar 6, 2017

View reviewed changes

Merge branch 'master' into reporter-config

329df49

cpojer reviewed Mar 6, 2017

View reviewed changes

abdulhannanali added 11 commits April 5, 2017 02:26

remove comments

7d903c9

correct types and change method signatures

1aa5d54

remove bug from reporterValidationErrors.js

599c6ed

make custom_reporters tests more concise

b5aa966

fix lint error in website

d1cf92e

this error is not letting me lint 100% safe

finalize types for reporters

23a7610

yarn prettier

dd2d652

remove .vscode folder

33640be

all integration_tests are prettier now

937a3c9

Merge branch 'master' into reporter-config

6dc65fb

remove validateReporters call

492e0de

abdulhannanali commented Apr 5, 2017

View reviewed changes

abdulhannanali changed the title ~~[WIP] Custom reporter integration~~ Custom reporter integration Apr 5, 2017

abdulhannanali added 4 commits April 10, 2017 08:19

remove usage of \t in reporter validation errors

8c486e7

change spread operator with usage of .apply

e328f93

modify custom_reporters integration_tests to suit node 4

bfad068

prettier validations

f4ed436

abdulhannanali added 4 commits April 11, 2017 16:04

Merge remote-tracking branch 'remotes/upstream/master' into reporter-…

3b154d2

…config

prettier ❤️

f6fa7bb

pretty lint

9ac6de0

update lock file

e093125

aaronabramov mentioned this pull request Apr 22, 2017

Custom reporters #3349

Merged

aaronabramov closed this Apr 22, 2017

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Custom reporter integration #3064

Custom reporter integration #3064

Conversation

abdulhannanali commented Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulhannanali commented Mar 4, 2017

abdulhannanali commented Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulhannanali commented Mar 4, 2017 • edited Loading

abdulhannanali commented Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulhannanali Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulhannanali commented Mar 6, 2017 • edited Loading

abdulhannanali commented Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulhannanali commented Apr 5, 2017

abdulhannanali commented Apr 5, 2017

bookman25 commented Apr 5, 2017

abdulhannanali commented Apr 11, 2017

aaronabramov commented Apr 22, 2017

github-actions bot commented May 13, 2021

abdulhannanali commented Mar 4, 2017 •

edited

Loading

abdulhannanali commented Mar 4, 2017 •

edited

Loading

abdulhannanali commented Mar 4, 2017 •

edited

Loading

abdulhannanali commented Mar 4, 2017 •

edited

Loading

abdulhannanali Mar 8, 2017 •

edited

Loading

abdulhannanali commented Mar 6, 2017 •

edited

Loading

abdulhannanali commented Mar 6, 2017 •

edited

Loading