Add summary_reporter.test.js #5389
Conversation
… into fix-update-command-npm
Codecov Report
@@ Coverage Diff @@
## master #5389 +/- ##
=========================================
+ Coverage 61.32% 62.23% +0.9%
=========================================
Files 205 205
Lines 6925 6925
Branches 3 3
=========================================
+ Hits 4247 4310 +63
+ Misses 2677 2614 -63
Partials 1 1
Continue to review full report at Codecov.
|
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Hey, thanks for improving Jest test coverage! Left some comments inlined
| process.env.npm_lifecycle_event = 'test'; | ||
| process.env.npm_lifecycle_script = 'jest'; | ||
| process.stderr.write = result => results.push(result); | ||
| SummaryReporter = require('../summary_reporter').default; |
There was a problem hiding this comment.
You can import SummaryReporter at the top of the file
| describe('onRunComplete', () => { | ||
| test('npm test', () => { | ||
| process.env.npm_config_user_agent = 'npm'; | ||
| const results = []; |
There was a problem hiding this comment.
Results are already defined, no need to shadow it
| expect(results[1]).toMatch( | ||
| '\u001b[1m\u001b[31m › 2 snapshot tests\u001b[39m\u001b[22m failed in 1 test suite. ' + | ||
| '\u001b[2mInspect your code changes or run `npm test -- -u` to update them.\u001b[22m', | ||
| ); |
There was a problem hiding this comment.
Can we join results with \n and store it as a snapshot? Or at least strip ansi. Same for the other test
| process.stderr.write = write; | ||
| }); | ||
|
|
||
| describe('onRunComplete', () => { |
There was a problem hiding this comment.
Can we remove the describe and just move its title to the tests? There's too little going on here to benefit from using it
kdnakt
changed the title
|
Thanks for the comments. I'll fix them in a few days. |
kdnakt
changed the title
|
@thymikee Thanks for the review comments. I've just updated the code so would you have a look? |
| process.stderr.write = result => results.push(result); | ||
| const testReporter = new SummaryReporter(globalConfig); | ||
| testReporter.onRunComplete(new Set(), aggregatedResults); | ||
| expect(results[0] + results[1]).toMatchSnapshot(); |
There was a problem hiding this comment.
How about results.join('')?
|
|
||
| test('snapshots needs update with npm test', () => { | ||
| process.env.npm_config_user_agent = 'npm'; | ||
| process.stderr.write = result => results.push(result); |
There was a problem hiding this comment.
This is already done in beforeEach
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ |
There was a problem hiding this comment.
Ah, forgot about adding @flow pragma here. See how it's done in other files.
There was a problem hiding this comment.
Adding @flow made the test fail ...
39: process.stderr.write = result => results.push(result);
^^^^^ property `write`. Covariant property `write` incompatible with contravariant use in
And other *.test.js files also don't have @flow pragma. So is it possible to skip adding it here?
There was a problem hiding this comment.
Oh, forgot we only added it to integration tests. Cool, we can skip it for now.
|
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. |
5 participants
Summary
I just added summary_reporter.test.js
Test plan