-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 9 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,11 +379,16 @@ class TestRunner { | |
_setupReporters() { | ||
const config = this._config; | ||
|
||
this.addReporter( | ||
config.verbose | ||
? new VerboseReporter(config) | ||
: new DefaultReporter(), | ||
); | ||
if (config.reporters) { | ||
this._addCustomReporters(config.reporters); | ||
} | ||
|
||
// Default Reporters are setup when | ||
// noDefaultReporters is false, which is false by default | ||
// and can be set to true in configuration. | ||
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 think this comment is superfluous as it describes what the if statement does. Mind removing it? 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, need to get over this habit. 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. Comments that describe what statements does are superfluous. Noted! Will remove them! |
||
if (!config.noDefaultReporters) { | ||
this._setupDefaultReporters(); | ||
} | ||
|
||
if (config.collectCoverage) { | ||
// coverage reporter dependency graph is pretty big and we don't | ||
|
@@ -392,12 +397,74 @@ class TestRunner { | |
this.addReporter(new CoverageReporter()); | ||
} | ||
|
||
this.addReporter(new SummaryReporter()); | ||
if (config.notify) { | ||
this.addReporter(new NotifyReporter(this._startRun)); | ||
} | ||
} | ||
|
||
/** | ||
* _setupDefaultReporters | ||
* Method for setting up the default reporters | ||
*/ | ||
_setupDefaultReporters() { | ||
const config = this._config; | ||
|
||
this.addReporter( | ||
config.verbose | ||
? new VerboseReporter(config) | ||
: new DefaultReporter() | ||
); | ||
|
||
this.addReporter(new SummaryReporter()); | ||
} | ||
|
||
/** | ||
* 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 | ||
* | ||
* "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) { | ||
if (this._config.bail && aggregatedResults.numFailedTests !== 0) { | ||
if (watcher.isWatchMode()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,100 @@ test('.addReporter() .removeReporter()', () => { | |
expect(runner._dispatcher._reporters).not.toContain(reporter); | ||
}); | ||
|
||
// 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', () => { | ||
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 Hey! I am new to testing. Wrote some unit tests to check the integration. Would you mind giving it a review? 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 method is prefixed with Jest integration tests run Jest agains an existing example directory and verify that 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 Thanks a lot that works, based on this I have removed additional tests I added for private method |
||
// Paths for the given reporters | ||
const SUMMARY_REPORTER_PATH = './reporters/SummaryReporter.js'; | ||
const VERBOSE_REPORTER_PATH = './reporters/VerboseReporter.js'; | ||
const DEFAULT_REPORTER_PATH = './reporters/DefaultReporter.js'; | ||
|
||
// Requiring constructors of the given reporters | ||
// to check against the reporters added | ||
const summaryReporter = require('.' + SUMMARY_REPORTER_PATH); | ||
const verboseReporter = require('.' + VERBOSE_REPORTER_PATH); | ||
const defaultReporter = require('.' + DEFAULT_REPORTER_PATH); | ||
|
||
let runner; | ||
|
||
beforeEach(() => { | ||
runner = new TestRunner({}, {}, {}); | ||
|
||
// Removing all the reporters we previously have in the | ||
// Dispatcher. Helps in removing inconsistencies in Tests. | ||
runner._dispatcher._reporters = []; | ||
}); | ||
|
||
test('adds reporter using 2D Array format', () => { | ||
const reporters = [ | ||
[SUMMARY_REPORTER_PATH, {}], | ||
]; | ||
|
||
expect(runner._dispatcher._reporters).toHaveLength(0); | ||
expect(runner._dispatcher._reporters[0]).toBeUndefined(); | ||
|
||
runner._addCustomReporters(reporters); | ||
|
||
expect(runner._dispatcher._reporters).toHaveLength(1); | ||
expect(runner._dispatcher._reporters.pop()).toBeInstanceOf(summaryReporter); | ||
}); | ||
|
||
test('adds reporter using 2D syntax with no configuration object', () => { | ||
const reporters = [ | ||
[SUMMARY_REPORTER_PATH], | ||
]; | ||
|
||
runner._addCustomReporters(reporters); | ||
|
||
expect(runner._dispatcher._reporters).toHaveLength(1); | ||
expect(runner._dispatcher._reporters.pop()).toBeInstanceOf(SummaryReporter); | ||
}); | ||
|
||
test('adds reporter using string syntax (no custom configuration)', () => { | ||
const reporters = [ | ||
SUMMARY_REPORTER_PATH, | ||
]; | ||
|
||
runner._addCustomReporters(reporters); | ||
|
||
expect(runner._dispatcher._reporters).toHaveLength(1); | ||
expect(runner._dispatcher._reporters.pop()).toBeInstanceOf(summaryReporter); | ||
}); | ||
|
||
test('adds two reporters with variable format', () => { | ||
const reporters = [ | ||
VERBOSE_REPORTER_PATH, | ||
[DEFAULT_REPORTER_PATH, {}], | ||
]; | ||
runner._addCustomReporters(reporters); | ||
|
||
expect(runner._dispatcher._reporters).toHaveLength(2); | ||
|
||
expect(runner._dispatcher._reporters[0]).toBeInstanceOf(verboseReporter); | ||
expect(runner._dispatcher._reporters[1]).toBeInstanceOf(defaultReporter); | ||
}); | ||
|
||
test('throws on invalid file path', () => { | ||
const reporters = [ | ||
['ohthisisnotgoingtobearealpath.sadfslj', {}], | ||
]; | ||
|
||
const addInvalidReporters = () => { | ||
runner._addCustomReporters(reporters); | ||
}; | ||
|
||
expect(addInvalidReporters).toThrow(); | ||
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. 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? 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. you can create a mock file and require it 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 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. 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 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. 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. 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. |
||
expect(addInvalidReporters).toThrow(/Cannot find module/); | ||
expect(runner._dispatcher._reporters).toHaveLength(0); | ||
}); | ||
|
||
test('throws on invalid argument to reporter', () => { | ||
expect(() => { | ||
runner._addCustomReporters('This should be an array obviously'); | ||
}).toThrow(); | ||
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. you should avoid using 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. always use a regular expression or a string to match the message 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 will add a regular expression, once I can finalize the type of test that's going to be used for this type checking 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. Thanks a lot for clarifying, I can definitely see what a disaster it can be |
||
}); | ||
}); | ||
|
||
describe('_createInBandTestRun()', () => { | ||
test('injects the rawModuleMap to each the worker in watch mode', () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -349,11 +349,15 @@ function normalize(config: InitialConfig, argv: Object = {}) { | |
case 'moduleLoader': | ||
case 'modulePaths': | ||
case 'name': | ||
case 'noDefaultReporters': | ||
case 'noStackTrace': | ||
case 'notify': | ||
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 |
---|---|---|
|
@@ -54,9 +54,13 @@ module.exports = ({ | |
modulePathIgnorePatterns: ['<rootDir>/build/'], | ||
modulePaths: ['/shared/vendor/modules'], | ||
name: 'string', | ||
noDefaultReporters: false, | ||
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'm unsure about this configuration option. Alternatively we could use
which gets overwritten when
Not sure if this is necessarily better and maybe my problem is just with the name of this boolean config, though. cc @DmitriiAbramov What do you think? 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 think it's a really good idea. "reporters": [
"default",
["custom-jest-reporter-package", {configValue: true}]
}
? 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. Ah, if the name of the default reporters is simply "default" (instead of "summary, default") then honestly I think we are fine with "reporters" just overwriting the default: // Includes the default reporters:
reporters: ["default", "custom-reporter"],
// or excludes them:
reporters: ["custom-reporter"],
// or includes some of them:
reporters: ["summary", "custom-reporter"], This basically would make the default reporter an aggregate of other reporters (jest, summary). We can also change this in the code so that the DefaultReporter wraps the jest (new name for current default reporter?) and summary reporter. 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 I like the style @DmitriiAbramov included for adding the default reporters. I can remove 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. Sounds good. Let's start with Dmitrii's suggestion and only implement "reporters" and then see what that looks like and take it from there :) |
||
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.