-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Changes from 4 commits
98d01b5
1215c0b
b3196ac
44f6550
aa548c9
268a640
e79ae0c
2ee53ce
073d6e9
6a430de
329df49
7cc7aea
138bec3
6da8dad
352380b
1a5ac39
124ee97
d067e59
302b672
a9e9fce
31b51e3
75e39d1
12e5382
08bd0cf
8bf1bb8
06e7103
32dcff4
45656ad
15abe4e
76ebadb
e158496
80c88db
4a33e50
352f219
03c2045
8dd5ef3
27c5533
cebb62e
e36d442
593040c
c94c993
fc8a4d3
cfdcccf
552d0e2
7d903c9
1aa5d54
599c6ed
b5aa966
d1cf92e
23a7610
dd2d652
33640be
937a3c9
6dc65fb
492e0de
8c486e7
e328f93
bfad068
f4ed436
3b154d2
f6fa7bb
9ac6de0
e093125
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -379,6 +379,10 @@ class TestRunner { | |
_setupReporters() { | ||
const config = this._config; | ||
|
||
if (config.reporters) { | ||
return this._addCustomReporters(config.reporters); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DmitriiAbramov Makes sense will add this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DmitriiAbramov The configuration open There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bookman25 Thanks for pointing it out, this will be used by the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah. this should be called from the constructor (or There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)} | ||
`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be quite heavily indented There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thymikee @DmitriiAbramov Ideally will this be added to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a module on npm for this purpose called There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)} | ||
`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -354,6 +354,9 @@ function normalize(config: InitialConfig, argv: Object = {}) { | |
case 'persistModuleRegistryBetweenSpecs': | ||
case 'preset': | ||
case 'replname': | ||
case 'reporters': | ||
value = _replaceRootDirTags(config.rootDir, config[key]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is necessary, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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': | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,9 @@ module.exports = ({ | |
noStackTrace: false, | ||
notify: false, | ||
preset: 'react-native', | ||
reporters: [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thymikee Would appreciate some feedback from you on this. There was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: '/', | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.