-
Notifications
You must be signed in to change notification settings - Fork 3k
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
chore(code coverage): enable coverage remapping to source via sourcemap #557
Conversation
"build_cjs": "rm -rf dist/cjs && babel dist/es6 --out-dir dist/cjs --modules common --sourceMaps --loose all && node lib/copy_dts.js", | ||
"build_es6": "rm -rf dist/es6 && tsc src/Rx.ts src/Rx.KitchenSink.ts --outDir dist/es6 --target ES6 -d", | ||
"build_cjs": "rm -rf dist/cjs && babel dist/es6 --out-dir dist/cjs --modules common --source-maps --loose all && node lib/copy_dts.js", | ||
"build_es6": "rm -rf dist/es6 && tsc src/Rx.ts src/Rx.KitchenSink.ts --outDir dist/es6 --sourceMap --target ES6 -d", |
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.
Found cjs / es6 modules didn't have sourcemap until now..
It is expected to see coverage number is decreased with this PR, now coverage is completely recalculated with ES6 output due to remapping. When it comes to original typescript source, I could observe coverage is around ~85%. |
This looks great so far. Unsure when I'll get a chance to merge this though. Might be a few days. |
Take your time, PR'll be there when you're back :) If I could find solutions of converting sourcemaps, I'll update PR meanwhile |
Also we'll have to research getting coveralls to be more lax until we're out of alpha |
@Blesh I could find this in coverall repo setting, maybe this is way to control? (Seems only repo owner can change those values) |
PR's updated including one additional commit to close #558, now I was originally tried to suggest move into different build system other than simple npm scripts, but found out that changes will include amount of out-of-scope changes not related with this specific issue. Those can be discussed in further as long term goal, focused to resolve immediate issue first. |
Okay, I've temporarily increased thresholds of Coveralls to allow this to merge without breaking Travis. We'll see if it works. |
@kwonoj so I pulled down this PR, rebased on master, removed node_modules, reinstalled via npm, and ran
Error:
I'll look into it, but not for too long, I have a LOT of PRs to catch up on. :) |
Seems I mistakenly miss one of dependencies. I'll try to resolve on my side and update PR. |
@kwonoj great! Notify me when you've got it. (Both here and Twitter today, because my GH inbox is dead from last week). |
- update script enables test coverage remapping to original (es6) source - minor update to document, code
15300cd
to
73b9989
Compare
@@ -20,4 +20,4 @@ script: | |||
- npm run build_cover | |||
|
|||
after_script: | |||
- cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js | |||
- cat ./coverage/coverage-remapped.lcov | ./node_modules/coveralls/bin/coveralls.js |
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.
Travis doesn't seem to be running the check now... this line is suspect.
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.
Yes, I'm looking those part now. On my forked branch this works, failed here, continuing investigation.
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.
Also, it seems that remap-istanbul
isn't writing a coverage-remapped.lcov
file at all.
I will push some dummy commits to analyze logs, will flatten them once issue is resolved. Will notify you @Blesh separately once it's done. |
…inal typescript code closes ReactiveX#558
07efbb3
to
4dd49b0
Compare
OK, travis & coveralls both happy. Reason of failures was |
This PR introduces
remap-istanbul
module allows to remap test coverage into original source code instead of transpiled results. (istanbul
side discussion is ongoing at gotwarlost/istanbul#212) Due to current build processes, this PR does not resolves remapping issue fully though.