feat(jest-reporters): pass reporterContext to custom reporter constructors as third argument
#12657
Conversation
docs/Configuration.md
Outdated
|
|
||
| ```json | ||
| { | ||
| "reporters": [ | ||
| "default", | ||
| ["<rootDir>/my-custom-reporter.js", {"banana": "yes", "pineapple": "no"}] | ||
| ["jest-junit", {"outputName": "junit-report.xml", "suiteName": "some-name"}] |
There was a problem hiding this comment.
Just to convince Prettier to keep this as separate line.
| return {options: this._options, path: reporter}; | ||
| return {options: {}, path: reporter}; |
There was a problem hiding this comment.
startRun() was passed here. Perhaps for notify reporter?.
There was a problem hiding this comment.
Hm.. Possibly this branch is never called, because reporters are already normalised:
There was a problem hiding this comment.
let's remove it, probably for legacy reasons
| @@ -49,6 +47,7 @@ | |||
| "@types/istanbul-lib-source-maps": "^4.0.0", | |||
| "@types/istanbul-reports": "^3.0.0", | |||
| "@types/node-notifier": "^8.0.0", | |||
| "jest-resolve": "^28.0.0-alpha.8", | |||
There was a problem hiding this comment.
Needed for tests only.
| export type OnTestStart = (test: Test) => Promise<void>; | ||
| export type OnTestFailure = ( | ||
| test: Test, | ||
| error: SerializableError, | ||
| ) => Promise<unknown>; | ||
| export type OnTestSuccess = ( | ||
| test: Test, | ||
| result: TestResult, | ||
| ) => Promise<unknown>; |
There was a problem hiding this comment.
Not used and not public exports.
| export type TestRunnerOptions = { | ||
| serial: boolean; | ||
| }; | ||
|
|
||
| export type TestRunData = Array<{ | ||
| context: Context; | ||
| matches: {allTests: number; tests: Array<Test>; total: number}; | ||
| }>; |
| export type { | ||
| AggregatedResult, | ||
| SnapshotSummary, |
There was a problem hiding this comment.
Fixed. Types for all public classes and utilities are exported. SnapshotSummary type is only used by getSnapshotSummary() util. This one is not exposed, but perhaps it should be exported together with getSnapshotStatus()? Looks helpful.
mrazauskas
changed the title
reporterContext to custom reporter constructors as third argumentreporterContext to custom reporter constructors as third argument
|
@SimenB Documentation got improved to address your last comments. User defined I was thinking if options and context could be combined into one object and passed liked this from test scheduler: new Reporter(this._globalConfig, {...options, _context: this._context})Could work, but extra nesting is somewhat clumsy. Alternative: new Reporter(this._globalConfig, {...options, ...this._context})Oh, that would be dangerous. Another attempt: new Reporter(
this._globalConfig,
{
...options,
_firstRun: this._context.firstRun,
_previousSuccess: this._context.previousSuccess,
// and the rest of context in similar fashion
}
)Hm.. That’s alright, but what about separation of concerns? These are not user defined options. Also might not work in long term. It seemed that this signature would be the most optimal: constructor(globalConfig, reporterOptions, reporterContext)Will not break with older reporters. Perhaps |
| } { | ||
| if (typeof reporter === 'string') { | ||
| return {options: this._options, path: reporter}; | ||
| return {options: {}, path: reporter}; | ||
| } else if (Array.isArray(reporter)) { | ||
| const [path, options] = reporter; |
There was a problem hiding this comment.
add a check that tuple length is 2?
There was a problem hiding this comment.
Can we leave this for another PR? Currently this method is called twice, that's nothing significant, but still unnecessary. I have an idea how to rework it, but that's another story (;
| @@ -411,7 +411,7 @@ export type GlobalConfig = { | |||
| passWithNoTests: boolean; | |||
| projects: Array<string>; | |||
| replname?: string; | |||
| reporters?: Array<string | ReporterConfig>; | |||
There was a problem hiding this comment.
This should type normalized config, right? I have added tests jest-config, which show that reporters are always normalized to a tuple. Having more precise type here, makes it possible to have less logic inside TestScheduler. Last commits are added just to illustrate the solution. Perhaps it would be better to send separate PR? Feel free to revert them.
|
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. |
Summary
Promise, I was refactoring
jest-worker, but noticed strange details in types ofjest-reporters. Just could not leave it.What if constructors of custom reporters alongside with their config options would also receive
reporterContext?Similarly default reporters all could receive unified
reporterContextobject. Currently context is passed to reporters, but is typed somewhat differently. That feels redundant.Also this change would allow writing custom reporters similar to coverage or notify. Why not? (;
Test plan
Green CI.