Custom reporters #3349

aaronabramov · 2017-04-22T06:23:39Z

i rebased/resolved/squashed #3064 which grew uncontrollably big.
also fixed a few issues and resolved conflicts with multi runner feature

cc @abdulhannanali

codecov-io · 2017-04-22T17:22:39Z

Codecov Report

Merging #3349 into master will decrease coverage by 0.21%.
The diff coverage is 51.4%.

@@            Coverage Diff             @@
##           master    #3349      +/-   ##
==========================================
- Coverage   64.02%   63.81%   -0.22%     
==========================================
  Files         177      179       +2     
  Lines        6580     6649      +69     
  Branches        5        5              
==========================================
+ Hits         4213     4243      +30     
- Misses       2365     2404      +39     
  Partials        2        2

Impacted Files	Coverage Δ
packages/jest-cli/src/reporters/NotifyReporter.js	`25.92% <ø> (ø)`	⬆️
packages/jest-config/src/index.js	`26.08% <ø> (ø)`	⬆️
packages/jest-cli/src/reporters/BaseReporter.js	`66.66% <ø> (ø)`	⬆️
packages/jest-config/src/validConfig.js	`100% <ø> (ø)`	⬆️
packages/jest-cli/src/reporters/DefaultReporter.js	`16.12% <0%> (ø)`	⬆️
packages/jest-cli/src/reporters/VerboseReporter.js	`30.61% <0%> (ø)`	⬆️
packages/jest-config/src/constants.js	`100% <100%> (ø)`	⬆️
packages/jest-config/src/normalize.js	`86.93% <100%> (-3.01%)`	⬇️
...ckages/jest-config/src/reporterValidationErrors.js	`33.33% <33.33%> (ø)`
packages/jest-cli/src/TestRunner.js	`30.85% <35.71%> (-0.29%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62d29f2...f2eb376. Read the comment docs.

cpojer · 2017-04-25T16:23:05Z

packages/jest-config/src/reporterValidationErrors.js

+ */
+function createReporterError(
+  reporterIndex: number,
+  reporterValue: any,


any? Shouldn't it demand at least "Object"?

cpojer · 2017-04-25T16:24:22Z

packages/jest-config/src/reporterValidationErrors.js

+function createArrayReporterError(
+  reporterIndex: number,
+  valueIndex: number,
+  value: any,


cpojer · 2017-04-25T16:24:43Z

packages/jest-config/src/normalize.js

@@ -32,6 +32,7 @@ const DEPRECATED_CONFIG = require('./deprecated');
 const JSON_EXTENSION = '.json';
 const PRESET_NAME = 'jest-preset' + JSON_EXTENSION;
 const ERROR = `${BULLET}Validation Error`;
+const {validateReporters} = require('./reporterValidationErrors');


this require should be in the block above.

cpojer · 2017-04-25T16:26:00Z

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


-  constructor(options: Options) {
+  constructor(options: GlobalConfig) {


I deliberately changed this in the past to not include all config options. But I guess it is fine – can we change this variable name to _globalConfig and globalConfig?

cpojer · 2017-04-25T16:26:29Z

integration_tests/__tests__/__snapshots__/custom-reporters-test.js.snap

+    "path": false,
+  },
+  "options": Object {
+    "christop": "pojer",


cpojer · 2017-04-25T16:27:25Z

integration_tests/__tests__/__snapshots__/custom-reporters-test.js.snap

+exports[`Custom Reporters Integration invalid format for adding reporters 1`] = `
+"●  Reporter Validation Error:
+
+Unexpected value for Path at index 0 of reporter at index 0


This message is so not nice. index 0 of reporter at index 0. Can we maybe print actual reporters value along with it?

cpojer · 2017-04-25T16:27:56Z

integration_tests/__tests__/custom-reporters-test.js

+    const {stdout, status, stderr} = runJest('custom_reporters', [
+      'add-test.js',
+    ]);
+    const parsedJSON = JSON.parse(stdout);


what about runJest.json?

but we're specifically testing a custom reporter :)

Ok, sure then :)

cpojer · 2017-04-25T16:28:45Z

integration_tests/custom_reporters/add.js

+
+'use strict';
+
+module.exports = function add(x, y) {


have you heard of arrow functions?

cpojer · 2017-04-25T16:28:58Z

integration_tests/custom_reporters/reporters/IncompleteReporter.js

+ * Reporter to test for the flexibility of the interface we implemented.
+ * The reporters shouldn't be required to implement all the methods
+ *
+ * This only implements one mehtod onRunComplete which should be called


cpojer · 2017-04-25T16:29:48Z

integration_tests/custom_reporters/reporters/TestReporter.js

+    const onRunStart = this._statsCollected.onRunStart;
+
+    onRunStart.called = true;
+    onRunStart.config = typeof config;


is this typeof call useful?

cpojer · 2017-04-25T16:29:59Z

packages/jest-cli/src/ReporterDispatcher.js

+import type BaseReporter from './reporters/BaseReporter';
+import type {ReporterOnStartOptions} from 'types/Reporters';
+
+export type RunOptions = {


should this be an exact type?

cpojer · 2017-04-25T16:30:48Z

packages/jest-cli/src/TestRunner.js

@@ -67,7 +69,7 @@ class TestRunner {
    this._setupReporters();
  }

-  addReporter(reporter: BaseReporter) {
+  addReporter(reporter: Object) {


Can this be a type for Reporter instances in types/TestRunner.js or something?

cpojer · 2017-04-25T17:06:05Z

packages/jest-cli/src/TestRunner.js

@@ -273,14 +275,22 @@ class TestRunner {
    return Promise.race([runAllTests, onInterrupt]).then(cleanup, cleanup);
  }

+  _shouldAddDefaultReporters(reporters?: Array<ReporterConfig>): boolean {
+    return !reporters || reporters.indexOf(DEFAULT_REPORTER_LABEL) !== -1;


What happens when reporters.length is 0?

i'd expect it to give no output, i think

I think we should throw in that case, tbh.

cpojer · 2017-04-25T17:11:09Z

packages/jest-cli/src/TestRunner.js

  _setupReporters() {
    const config = this._config;
+    const reporters = config.reporters;
+    const isDefault: boolean = this._shouldAddDefaultReporters(reporters);


does this really need a type annotation inline? Flow should be able to figure it out.

cpojer · 2017-04-25T17:11:58Z

Bunch of comments but feel free to merge it after one more iteration.

cpojer · 2017-05-02T17:47:35Z

Please don't forget to document the reporters option in the docs and adding "Available in Jest 20.0.0" like we do for other things (Like the expect.resolves API).

cpojer

I'll request changes because you are doing some bigger rewrites and I'll take a look at it tomorrow :)

this error is not letting me lint 100% safe Reporter config 3 (#2) * add custom reporters option in TestRunner * add reporters option in jest-cli config * add flowtype for reporters option * add key for reporters in validConfig * add noDefaultReporters option noDefaultReporters option let's user turn off all the reporters set by default * Lint * add unit tests for _addCustomReporters * separate default reporters in method in TestRunner * add tests for errors which are thrown * add tests for .noDefaultReporters * modify Error thrown for _addCustomReporters * remove superfluous comment from TestRunner.js * remove reporter tests from TestRunner-test.js * add new custom reporters format in TestRunner.js * update the format for adding customReporter * add descriptive validations for reporters * add reporters attibute in normalize.js * add prettier to types * Seperate out ReporterDispatcher in a file * add elaborate messages for errors * add Facebook Copyright header to ReporterDispatcher.js * typecheck and lint properly * correcting a condition in ReporterDispatcher * rename method to `_shouldAddDefaultReporters` * add integration tests for custom_reporters * add more complete integration tests for reporters * remove AggregatedResults.js * remove any methods to be validated * correct _addDefaultReporters call * remove "reporters" validations from TestRunner.js * add pretty validations for custom reporters * remove comment * add reporter validation in normalize.js * keep comments precise remove unwanted * check if reporters exist before validation * pretty custom reporters * prettier integration_tests * prettier * yarn prettier * prettier * Remove unnecessary comments from TestRunner.js * make ReporterConfig type in types/Config simpler * remove comments * correct types and change method signatures * remove bug from reporterValidationErrors.js * make custom_reporters tests more concise * fix lint error in website this error is not letting me lint 100% safe * finalize types for reporters * yarn prettier * remove .vscode folder * all integration_tests are prettier now * remove validateReporters call * remove usage of \t in reporter validation errors * change spread operator with usage of .apply * modify custom_reporters integration_tests to suit node 4 * prettier validations * prettier ❤️ * pretty lint * update lock file

cpojer · 2017-05-02T21:12:07Z

integration_tests/custom_reporters/reporters/IncompleteReporter.js

+ * This only implements one method onRunComplete which should be called
+ */
+class IncompleteReporter {
+  constructor(options) {


this seems unused.

cpojer · 2017-05-02T21:13:22Z

integration_tests/__tests__/__snapshots__/custom-reporters-test.js.snap

+ Got:
+   number
+ Reporters config:
+   [      3243242    ]


This whitespace is really ugly. Why can we not just print this using prettyFormat like we always do?

i think i missed join('\n') somewhere :)

cpojer · 2017-05-02T21:14:21Z

packages/jest-cli/src/TestRunner.js

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


this is passing config once and this._globalConfig on the other, even though they are the same.

cpojer · 2017-05-02T21:14:42Z

packages/jest-cli/src/TestRunner.js

+  }
+
+  _addCustomReporters(reporters: Array<ReporterConfig>) {
+    const customReporter = reporters.filter(reporter => reporter !== 'default');


this should be customReporters, plural.

cpojer · 2017-05-02T21:17:02Z

packages/jest-cli/src/TestRunner.js

+          chalk.red(
+            'An error occured while adding the reporter at path ' + path,
+          ),
+        );


why does this write to console.error and then throw? Shouldn't it be:

throw new Error( 'An error occured while adding the reporter at path "' + path + '".' + error.message );

cpojer · 2017-05-02T21:17:32Z

packages/jest-cli/src/TestRunner.js

+   * Get properties of a reporter in an object
+   * to make dealing with them less painful.
+   */
+  _getReporterProps(reporter: ReporterConfig): Object {


I think you can remote Object here because it is a private field. If not, then please type it properly :)

cpojer · 2017-05-02T21:17:40Z

packages/jest-cli/src/TestRunner.js

+   * to make dealing with them less painful.
+   */
+  _getReporterProps(reporter: ReporterConfig): Object {
+    let props = {};


Object.create(null).

cpojer · 2017-05-02T21:18:46Z

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

 import type {PathPattern} from '../SearchSource';
 import type {ReporterOnStartOptions} from 'types/Reporters';

+type SummaryReporterOptions = {


exact type please.

cpojer · 2017-05-02T21:19:00Z

packages/jest-cli/src/reporters/__tests__/CoverageReporter-test.js

@@ -4,8 +4,6 @@
 * This source code is licensed under the BSD-style license found in the
 * LICENSE file in the root directory of this source tree. An additional grant
 * of patent rights can be found in the PATENTS file in the same directory.
- *
- * @emails oncall+jsinfra


we should remove this from all tests in this repo, actually.

cpojer · 2017-05-02T21:19:27Z

packages/jest-config/src/normalize.js

@@ -255,6 +256,10 @@ function normalize(options: InitialOptions, argv: Object = {}) {
    exampleConfig: VALID_CONFIG,
  });

+  if (options.reporters && Array.isArray(options.reporters)) {


If it is not an array, shouldn't we also validate it?

cpojer · 2017-05-02T21:20:49Z

I left a few comments inline. The one missing piece is that the <rootDir> replacement and proper resolution using jest-resolve (we do this in normalize.js for all other fields) must be added, otherwise resolving reporters works differently from every other config option. Mind adding that?

cpojer · 2017-05-02T23:39:22Z

Alright, good to go.

* Custom Reporters this error is not letting me lint 100% safe Reporter config 3 (jestjs#2) * add custom reporters option in TestRunner * add reporters option in jest-cli config * add flowtype for reporters option * add key for reporters in validConfig * add noDefaultReporters option noDefaultReporters option let's user turn off all the reporters set by default * Lint * add unit tests for _addCustomReporters * separate default reporters in method in TestRunner * add tests for errors which are thrown * add tests for .noDefaultReporters * modify Error thrown for _addCustomReporters * remove superfluous comment from TestRunner.js * remove reporter tests from TestRunner-test.js * add new custom reporters format in TestRunner.js * update the format for adding customReporter * add descriptive validations for reporters * add reporters attibute in normalize.js * add prettier to types * Seperate out ReporterDispatcher in a file * add elaborate messages for errors * add Facebook Copyright header to ReporterDispatcher.js * typecheck and lint properly * correcting a condition in ReporterDispatcher * rename method to `_shouldAddDefaultReporters` * add integration tests for custom_reporters * add more complete integration tests for reporters * remove AggregatedResults.js * remove any methods to be validated * correct _addDefaultReporters call * remove "reporters" validations from TestRunner.js * add pretty validations for custom reporters * remove comment * add reporter validation in normalize.js * keep comments precise remove unwanted * check if reporters exist before validation * pretty custom reporters * prettier integration_tests * prettier * yarn prettier * prettier * Remove unnecessary comments from TestRunner.js * make ReporterConfig type in types/Config simpler * remove comments * correct types and change method signatures * remove bug from reporterValidationErrors.js * make custom_reporters tests more concise * fix lint error in website this error is not letting me lint 100% safe * finalize types for reporters * yarn prettier * remove .vscode folder * all integration_tests are prettier now * remove validateReporters call * remove usage of \t in reporter validation errors * change spread operator with usage of .apply * modify custom_reporters integration_tests to suit node 4 * prettier validations * prettier ❤️ * pretty lint * update lock file * Custom Reporters (merge/fix) * Use jest-resolve to resolve reporters * Minor cleanups

github-actions · 2021-05-13T17:24:41Z

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.

facebook-github-bot added the CLA Signed ✔️ label Apr 22, 2017

aaronabramov mentioned this pull request Apr 22, 2017

Custom reporter integration #3064

Closed

aaronabramov force-pushed the reporter-config-4 branch 2 times, most recently from 9b77ab1 to 4d93360 Compare April 22, 2017 16:54

cpojer reviewed Apr 25, 2017

View reviewed changes

cpojer approved these changes Apr 25, 2017

View reviewed changes

cpojer added this to the Jest 20 milestone Apr 28, 2017

cpojer requested changes May 2, 2017

View reviewed changes

aaronabramov force-pushed the reporter-config-4 branch from 4d93360 to d0c55f8 Compare May 2, 2017 20:44

aaronabramov force-pushed the reporter-config-4 branch from d0c55f8 to deb2790 Compare May 2, 2017 21:05

cpojer reviewed May 2, 2017

View reviewed changes

aaronabramov force-pushed the reporter-config-4 branch from deb2790 to 57385b5 Compare May 2, 2017 21:17

cpojer reviewed May 2, 2017

View reviewed changes

Custom Reporters (merge/fix)

8dec8ba

aaronabramov force-pushed the reporter-config-4 branch from 57385b5 to 8dec8ba Compare May 2, 2017 21:33

aaronabramov and others added 2 commits May 2, 2017 15:50

Use jest-resolve to resolve reporters

19168b9

Minor cleanups

f2eb376

cpojer merged commit 4a99428 into jestjs:master May 2, 2017

aaronabramov deleted the reporter-config-4 branch May 2, 2017 23:47

This was referenced May 31, 2017

Add CI reporters (AppVeyor, CircleCI, etc) and automatically enable when appropriate #3536

Closed

Replace Jasmine with jest-circus #3668

Merged

SimenB mentioned this pull request Jun 27, 2017

No custom test reporters #862

Closed

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


		constructor(options: Options) {
		constructor(options: GlobalConfig) {

Custom reporters #3349

Custom reporters #3349

Conversation

aaronabramov commented Apr 22, 2017

codecov-io commented Apr 22, 2017 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Apr 25, 2017

cpojer commented May 2, 2017

cpojer left a 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

cpojer commented May 2, 2017

cpojer commented May 2, 2017

github-actions bot commented May 13, 2021

codecov-io commented Apr 22, 2017 •

edited

Loading