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

Custom reporter integration #3064

Closed
wants to merge 63 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
98d01b5
add custom reporters option in TestRunner
abdulhannanali Mar 3, 2017
1215c0b
add reporters option in jest-cli config
abdulhannanali Mar 4, 2017
b3196ac
add flowtype for reporters option
abdulhannanali Mar 4, 2017
44f6550
add key for reporters in validConfig
abdulhannanali Mar 4, 2017
aa548c9
add noDefaultReporters option
abdulhannanali Mar 5, 2017
268a640
Lint
abdulhannanali Mar 5, 2017
e79ae0c
add unit tests for _addCustomReporters
abdulhannanali Mar 5, 2017
2ee53ce
separate default reporters in method in TestRunner
abdulhannanali Mar 6, 2017
073d6e9
add tests for errors which are thrown
abdulhannanali Mar 6, 2017
6a430de
add tests for .noDefaultReporters
abdulhannanali Mar 6, 2017
329df49
Merge branch 'master' into reporter-config
abdulhannanali Mar 6, 2017
7cc7aea
modify Error thrown for _addCustomReporters
abdulhannanali Mar 8, 2017
138bec3
remove superfluous comment from TestRunner.js
abdulhannanali Mar 8, 2017
6da8dad
remove reporter tests from TestRunner-test.js
abdulhannanali Apr 1, 2017
352380b
add new custom reporters format in TestRunner.js
abdulhannanali Apr 1, 2017
1a5ac39
update the format for adding customReporter
abdulhannanali Apr 1, 2017
124ee97
Merge branch 'master' into reporter-config
abdulhannanali Apr 1, 2017
d067e59
add descriptive validations for reporters
abdulhannanali Apr 1, 2017
302b672
add reporters attibute in normalize.js
abdulhannanali Apr 1, 2017
a9e9fce
add prettier to types
abdulhannanali Apr 1, 2017
31b51e3
Seperate out ReporterDispatcher in a file
abdulhannanali Apr 1, 2017
75e39d1
add elaborate messages for errors
abdulhannanali Apr 1, 2017
12e5382
add Facebook Copyright header to ReporterDispatcher.js
abdulhannanali Apr 1, 2017
08bd0cf
typecheck and lint properly
abdulhannanali Apr 1, 2017
8bf1bb8
correcting a condition in ReporterDispatcher
abdulhannanali Apr 2, 2017
06e7103
rename method to `_shouldAddDefaultReporters`
abdulhannanali Apr 2, 2017
32dcff4
add integration tests for custom_reporters
abdulhannanali Apr 2, 2017
45656ad
add more complete integration tests for reporters
abdulhannanali Apr 2, 2017
15abe4e
remove AggregatedResults.js
abdulhannanali Apr 2, 2017
76ebadb
remove any methods to be validated
abdulhannanali Apr 2, 2017
e158496
correct _addDefaultReporters call
abdulhannanali Apr 2, 2017
80c88db
remove "reporters" validations from TestRunner.js
abdulhannanali Apr 3, 2017
4a33e50
add pretty validations for custom reporters
abdulhannanali Apr 3, 2017
352f219
remove comment
abdulhannanali Apr 3, 2017
03c2045
add reporter validation in normalize.js
abdulhannanali Apr 3, 2017
8dd5ef3
keep comments precise remove unwanted
abdulhannanali Apr 3, 2017
27c5533
check if reporters exist before validation
abdulhannanali Apr 3, 2017
cebb62e
pretty custom reporters
abdulhannanali Apr 4, 2017
e36d442
prettier integration_tests
abdulhannanali Apr 4, 2017
593040c
prettier
abdulhannanali Apr 4, 2017
c94c993
yarn prettier
abdulhannanali Apr 4, 2017
fc8a4d3
prettier
abdulhannanali Apr 4, 2017
cfdcccf
Remove unnecessary comments from TestRunner.js
abdulhannanali Apr 4, 2017
552d0e2
make ReporterConfig type in types/Config simpler
abdulhannanali Apr 4, 2017
7d903c9
remove comments
abdulhannanali Apr 4, 2017
1aa5d54
correct types and change method signatures
abdulhannanali Apr 4, 2017
599c6ed
remove bug from reporterValidationErrors.js
abdulhannanali Apr 5, 2017
b5aa966
make custom_reporters tests more concise
abdulhannanali Apr 5, 2017
d1cf92e
fix lint error in website
abdulhannanali Apr 5, 2017
23a7610
finalize types for reporters
abdulhannanali Apr 5, 2017
dd2d652
yarn prettier
abdulhannanali Apr 5, 2017
33640be
remove .vscode folder
abdulhannanali Apr 5, 2017
937a3c9
all integration_tests are prettier now
abdulhannanali Apr 5, 2017
6dc65fb
Merge branch 'master' into reporter-config
abdulhannanali Apr 5, 2017
492e0de
remove validateReporters call
abdulhannanali Apr 5, 2017
8c486e7
remove usage of \t in reporter validation errors
abdulhannanali Apr 10, 2017
e328f93
change spread operator with usage of .apply
abdulhannanali Apr 10, 2017
bfad068
modify custom_reporters integration_tests to suit node 4
abdulhannanali Apr 11, 2017
f4ed436
prettier validations
abdulhannanali Apr 11, 2017
3b154d2
Merge remote-tracking branch 'remotes/upstream/master' into reporter-…
abdulhannanali Apr 11, 2017
f6fa7bb
prettier :heart:
abdulhannanali Apr 11, 2017
9ac6de0
pretty lint
abdulhannanali Apr 11, 2017
e093125
update lock file
abdulhannanali Apr 11, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,7 @@
"jest.pathToJest": "npm run jest --",
"editor.rulers": [
80
]
],
"eslint.enable": true,
"javascript.validate.enable": false
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
55 changes: 55 additions & 0 deletions packages/jest-cli/src/TestRunner.js
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,10 @@ class TestRunner {
_setupReporters() {
const config = this._config;

if (config.reporters) {
return this._addCustomReporters(config.reporters);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DmitriiAbramov Makes sense will add this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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 have added the first iteration of this which used regardless of reporters option specified or not.

}

this.addReporter(
config.verbose
? new VerboseReporter(config)
Expand All @@ -396,6 +400,57 @@ class TestRunner {
if (config.notify) {
this.addReporter(new NotifyReporter(this._startRun));
}

return undefined;
}

/**
* Add Custom reporters to Jest
* Custom reporters can be added to Jest using the reporters option in Jest
* Config. The format for adding a custom reporter is following:
*
* Format for the custom reporters
*
* "reporters": [
* ["reporterPath/packageName", { option1: 'fasklj' }],
* // Format if we want to specify options
*
* "reporterName"
* // Format if we don't want to specify any options
* ]
*/
_addCustomReporters(reporters: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like this method is being called by anything except tests right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bookman25 Thanks for pointing it out, this will be used by the _setupReporters, based on my conversation with Christop and Dmitrii, I am making a few changes on how the reporters options are passed in the configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah. this should be called from the constructor (or setupReporters() method).
i also think it should probably take the entire config as an argument, since it's aware of the configuration and it's better to encapsulate this logic in once place (i mean the logic of where to get the config from and how to validate it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DmitriiAbramov Yes that helps will make this change

let reporterPath, reporterConfig;

reporters.forEach(entry => {
if (Array.isArray(entry)) {
[reporterPath, reporterConfig] = entry;
} else {
if (typeof entry !== 'string') {
throw new Error(`
Unexpected custom reporter configuration.
Expected to get either a path string or an array of [path, confg]
got: ${JSON.stringify(entry)}
`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be quite heavily indented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

reporterPath = entry;
}

try {
const reporter = require(reporterPath);
this.addReporter(new reporter(reporterConfig || {}));
} catch (error) {
console.error(`
Failed to set up reporter:
${JSON.stringify(reporterPath)}
Config:
${JSON.stringify(reporterConfig)}
`);
Copy link
Member

Choose a reason for hiding this comment

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

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.


throw error;
}
});
}

_bailIfNeeded(aggregatedResults: AggregatedResult, watcher: TestWatcher) {
Expand Down
3 changes: 3 additions & 0 deletions packages/jest-config/src/normalize.js
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,9 @@ function normalize(config: InitialConfig, argv: Object = {}) {
case 'persistModuleRegistryBetweenSpecs':
case 'preset':
case 'replname':
case 'reporters':
value = _replaceRootDirTags(config.rootDir, config[key]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

break;
case 'resetMocks':
case 'resetModules':
case 'rootDir':
Expand Down
3 changes: 3 additions & 0 deletions packages/jest-config/src/validConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ module.exports = ({
noStackTrace: false,
notify: false,
preset: 'react-native',
reporters: [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool makes sense thanks

['./here-it-goes.js', {option1: true}]
],
resetMocks: false,
resetModules: false,
rootDir: '/',
Expand Down
2 changes: 2 additions & 0 deletions types/Config.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export type Config = {|
noStackTrace: boolean,
notify: boolean,
preset: ?string,
reporters: Array<string | Array<string | Object>>,
resetMocks: boolean,
resetModules: boolean,
rootDir: Path,
Expand Down Expand Up @@ -132,6 +133,7 @@ export type InitialConfig = {|
forceExit?: boolean,
globals?: ConfigGlobals,
haste?: HasteConfig,
reporters?: Array<string | Array<string | Object>>,
logHeapUsage?: boolean,
logTransformErrors?: ?boolean,
moduleDirectories?: Array<string>,
Expand Down