Pass “Test”s to reporters; update Test to contain contexts. #3289
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3289 +/- ##
==========================================
- Coverage 63.89% 63.89% -0.01%
==========================================
Files 176 176
Lines 6473 6472 -1
Branches 4 4
==========================================
- Hits 4136 4135 -1
Misses 2336 2336
Partials 1 1
Continue to review full report at Codecov.
|
| @@ -228,17 +232,17 @@ class TestRunner { | |||
|
|
|||
| // Send test suites to workers continuously instead of all at once to track | |||
| // the start time of individual tests. | |||
| const runTestInWorker = ({config, path}) => | |||
| const runTestInWorker = test => | |||
There was a problem hiding this comment.
const runTestInWorker = (test: Test) =>There was a problem hiding this comment.
Flow isn't asking me to annotate this which means it infers the proper type. Why should I change it? I don't want to add superfluous types.
| @@ -439,14 +443,13 @@ class ReporterDispatcher { | |||
| ); | |||
| } | |||
|
|
|||
| onTestResult(config, testResult, results) { | |||
| onTestResult(test, testResult, results) { | |||
There was a problem hiding this comment.
Types
onTestResult (test: Test, testResult: TestResult, results: AggregatedResult)There was a problem hiding this comment.
Same, Flow isn't asking for it. Is this about readability? If Flow isn't asking it means the type isn't ambiguous to flow.
There was a problem hiding this comment.
Ok, sorry about that, I'm checking it through GH, so I don't know if Flow infers something :D
| constructor(config: Config) { | ||
| this._config = config; | ||
| constructor(context: Context) { | ||
| this._context = context; | ||
| this._cache = {}; |
There was a problem hiding this comment.
I think it would be nice to type perf cache too
| @@ -84,11 +84,13 @@ const runJest = async ( | |||
|
|
|||
| if ( | |||
| data.paths.length === 1 && | |||
| config.silent !== true && | |||
| config.verbose !== false | |||
| hasteContext.config.silent !== true && | |||
There was a problem hiding this comment.
Shouldn't this be renamed to context for consistency?
There was a problem hiding this comment.
That is happening somewhere in the larger PR, yes. It creates more merge conflicts if I do it in this one.
|
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
Pulling out commit #2 from #3156 to make the other diff easier to review as I add more stuff on top of it. This is changing how reporters work a little bit but there may be some more changes later.
cc @abdulhannanali
Test plan
jest