-
Notifications
You must be signed in to change notification settings - Fork 79
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
Avoid reading sourcemaps in js file if it is already in fileCoverage.inputSourceMap #125
Conversation
@MichaReiser any chance you have a moment to review/test this proposed follow-up to #87? |
@mttcr note this is failing with our tests, see https://travis-ci.org/SitePen/remap-istanbul/builds/190977909 |
Probably not to test, but I can review it. Let me know when the tests are fixed. |
…has an inlined sourcemap
I restored the codeIsArray logic and I provided a test that checks that the CoverageTransformer correctly uses the provided inputSourceMap and not the one inlined in the source. To check that, the inputSourceMap is a bit different from the inlined one: it has a custom sourceRoot. The test passes with my branch, and doesn't with yours. @dylans @MichaReiser your tests are fixed and I also provided a new one to check the new functionality. Hope it's good for you! |
Current coverage is 98.60% (diff: 100%)@@ master #125 diff @@
==========================================
Files 15 15
Lines 573 573
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 564 565 +1
+ Misses 9 8 -1
Partials 0 0
|
Looks good from my side... |
Thanks @mttcr and @MichaReiser . I'll let this sit for a few days in case anyone else has thoughts or feedback, and then land it. |
From #123
PR #87 allowed to use the inputSourceMap provided in fileCoverage. The problem is that we still parse the js file to override the sourcemap if there is one inlined. This PR skips the reading of the js file if we already have the sourcemap.