Restore coverage in Travis #7628
Conversation
ghost
added
| var grunt = require('grunt'); | ||
| var path = require('path'); | ||
|
|
||
| var rootPath = path.resolve('.'); |
There was a problem hiding this comment.
All this code path was really weird and not needed anymore. We can just run jest --coverage to do the same thing.
There was a problem hiding this comment.
I don't think that's true. We want to exclude some files for sure, eg https://github.com/facebook/react/tree/355c49065386cbe33c026ba573c88c4e459ea328/src/shared/vendor/third_party (that file is showing up in the coverage report for this build). I'm also seeing coverage results for scripts/jest/test-framework-setup.js in the report.
There was a problem hiding this comment.
Jest has this annoying mode where you can either use the default where it runs coverage on all the files that are required by tests (the current mode) or you can manually specify all the files/folders you want to whitelist and blacklist.
My thinking was that outside of this thirty party component file, the rest is actually good. When I tried doing it on everything then it reported a bunch of useless stuff.
By the way, we don't need all that logic anymore, jest has a setting collectCoverageFrom in package.json that lets us configure it: https://github.com/facebook/jest/blob/master/package.json#L47
If you want to go through all the files and make it list only the things we care about, let me know.
| "gulp-util": "^3.0.7", | ||
| "gzip-js": "~0.3.2", | ||
| "jest": "^15.0.1", | ||
| "jest": "^15.0.2", |
There was a problem hiding this comment.
This fixed a bug with coverage and node 4
| ./node_modules/.bin/grunt jest:normal | ||
| fi | ||
| ./node_modules/.bin/grunt jest:coverage | ||
| cat ./coverage/lcov.info | ./node_modules/.bin/coveralls |
There was a problem hiding this comment.
Is coveralls better than codecov? For Jest we now use Codecov and it comments on every PR with the coverage difference. It is nice.
There was a problem hiding this comment.
coveralls should do that too https://coveralls.io/features
|
oh man, this is so much simpler. Does this run coverage on every PR or only on master? I think we should start collecting coverage on PRs too, possibly with codecov commenting on diffs. If this passes on travis just fine, shall we merge it and see if it works well on PRs over the course of a day and potentially revert if there is trouble? I don't expect there to be any, but you know, all libraries have different, crazy tests. Overall, the Jest tests are very similar to React though. |
|
Changes Unknown when pulling f51d0c9 on vjeux:fix_coverage into * on facebook:master*. |
|
Changes Unknown when pulling f51d0c9 on vjeux:fix_coverage into * on facebook:master*. |
|
omg it works. |
|
Changes Unknown when pulling e10e198 on vjeux:fix_coverage into * on facebook:master*. |
|
Do you guys know if it's normal that it says "Changes Unknown" but manages to get the correct 67%? Maybe it's because it's the first time it runs? |
|
Could be |
|
Yea, probably fine. I'm guessing it's looking at the branch point and trying to find coverage for that commit but since we haven't had coverage on for a while it cant find anything. |
|
Okay, here's my suggestion: we merge it in and see how it works in the next few prs that come in. |
|
Please make sure you exclude files appropriately (I think my comment above may be getting lost since it's inline). Edit: comment wasn't lost but I missed your response… We shouldn't include coverage results for things that aren't our code. Coverage should be reporting on React itself. |
| "<rootDir>/src", | ||
| "node_modules/fbjs" | ||
| ], | ||
| "collectCoverageFrom": [ |
There was a problem hiding this comment.
@zpao I put back the old coverage paths, should be good to review
|
Changes Unknown when pulling 6681ad6 on vjeux:fix_coverage into * on facebook:master*. |
|
Yay, 87% |
We disabled coverage in Travis because the implementation was crashing ( facebook#6290 ). Since we upgraded to Jest 15, the entire coverage implementation is brand new so we should give it another try.
|
Changes Unknown when pulling 17139d7 on vjeux:fix_coverage into * on facebook:master*. |
|
Thanks! |
We disabled coverage in Travis because the implementation was crashing ( facebook#6290 ). Since we upgraded to Jest 15, the entire coverage implementation is brand new so we should give it another try.
We disabled coverage in Travis because the implementation was crashing ( #6290 ). Since we upgraded to Jest 15, the entire coverage implementation is brand new so we should give it another try.