Update Source Map Handling

[pr1sm]

Changes

Update how source maps are handled

Instead of combining source maps manually, this is handled automatically by istanbul-lib-instrument when passing the input source map in as an argument in the instrument call.

Remove source-map dependency

With the removal of combining source maps, the source-map package is not longer needed.

Fix test mocks

The preprocessor test mocks have been updated to include the necessary mocked functions. The updated mocks don't change what is being tested.

---

I've created a playground to test karma-coverage here. The master branch (at the time of creating this PR) is using my patch and the generated reports are correct for the few languages I tested (available here).

The playground has really simple code/tests, but if anyone wants to test it on a bigger codebase, I'd appreciate it!

fixes #393

[johnjbarton]

Suppose we have JS input that is compiled from TS or coffeescript or etc. That JS will have a sourecemap point back to the original sources. If the instrumenter does not consider that sourcemap, it will generate a map from instrumentedJS back to JS. The deleted code here would have combined the maps to create instrumentedJS back to eg TS. What changed to allow us to delete this combiner code?

[pr1sm]

@johnjbarton Sorry for the late reply -- I was away this weekend and didn't have time to respond.

If the instrumenter does not consider that sourcemap, it will generate a map from instrumentedJS back to JS.

I believe the instrumented JS explicitly creates a source map only if an incoming source map is detected. This is the current code on master (my changes don't affect this code):

karma-coverage/lib/preprocessor.js

Lines 128 to 139 in d3f53e3

	if (file.sourceMap) {
	log.debug('Enabling source map generation for "%s".', file.originalPath)
	codeGenerationOptions = Object.assign({}, {
	format: {
	compact: !constructOptions.noCompact
	},
	sourceMap: file.sourceMap.file,
	sourceMapWithCode: true,
	file: file.path
	}, constructOptions.codeGenerationOptions || {})
	options.produceSourceMap = true
	}

If no source map is detected (file.sourceMap is undefined), then a source map is only created if the instrumenter options are set to generate one (either by default options or by an instrumenter override in the karma config). Maybe this is a bug and it should be changed to if (!file.sourceMap)?

What changed to allow us to delete this combiner code?

The source map from the instrumented JS back to the transpiled JS is no longer necessary since I believe the newer istanbul packages use the transpiled JS as the starting point for remapping coverage back to the original source. This means that only the source map from transpiled JS back to the original source (TS, coffeescript, etc.) would be necessary to get correct reports.

---

EDIT: Disregard the last part of this comment, I was unaware of the unintended side effects my changes could cause per #394 (comment). While it is true that istanbul doesn't need the merged source maps, karma itself might for reporting mapped stacktraces (at the very least).

Adding in a test exception to ts (as an example), I can confirm my changes cause broken stacktrace mapping to occur. The stack trace outputs a location corresponding to the instrumented JS and does not map it back to the original source code location:

Relevant Output

19 11 2019 22:22:49.703:WARN [reporter]: SourceMap position not found for trace: Error: testing...
    at Object.add (base/src/addts.js?cd950d72a848e5f042ccf27d669049e0462a3118:1:2769)
HeadlessChrome 78.0.3904 (Mac OS X 10.14.6) test add (ts) adds equal numbers FAILED
        Error: testing...
            at Object.add (src/addts.js:1:2769)
            at Context. (test/addts.test.ts:12:17 <- test/addts.test.js:11:27)
  

I'll fix this and confirm it does create correctly mapped stacktraces when an error is thrown.

[pr1sm]

I've fixed the stack trace source mapping issue and have updated my test repo to throw exceptions on purpose and confirm stack traces work correctly:

Relevant Output

19 11 2019 23:35:49.235:INFO [HeadlessChrome 78.0.3904 (Mac OS X 10.14.6)]: Connected on socket w4Uhk4jkRi4pExFLAAAA with id 60605994
HeadlessChrome 78.0.3904 (Mac OS X 10.14.6) test add (es5) should fail this test with an error FAILED
        Error: testing...
            at window.throwEs5Error (/Users/dhanwada/Repositories/karma-coverage-test/src/add.es5.js:16:9 <- src/add.es5.js:1:2621)
            at Context. (test/add.es5.test.js:569:657)
HeadlessChrome 78.0.3904 (Mac OS X 10.14.6) test add (babel) should fail this test with an error FAILED
        Error: testing...
            at l (test/add.webpack.test.js:1:3175)
            at Context. (test/add.webpack.test.js:569:680)
HeadlessChrome 78.0.3904 (Mac OS X 10.14.6) test add (coffee) should fail this test with an error FAILED
        Error: testing...
            at window.throwCoffeeError (/Users/dhanwada/Repositories/karma-coverage-test/src/addcoffee.coffee:18:9 <- src/addcoffee.js:1:3305)
            at Context. (test/addcoffee.test.coffee:9:5 <- test/addcoffee.test.js:9:12)
HeadlessChrome 78.0.3904 (Mac OS X 10.14.6) test add (ts) should fail this test with an error FAILED
        Error: testing...
            at Object.throwError (/Users/dhanwada/Repositories/karma-coverage-test/src/addts.ts:18:11 <- src/addts.js:1:4232)
            at Context. (test/addts.test.ts:17:5 <- test/addts.test.js:15:17)
HeadlessChrome 78.0.3904 (Mac OS X 10.14.6): Executed 12 of 12 (4 FAILED) (0.011 secs / 0.003 secs)

It's important to note the webpack example doesn't properly report them. This is because the webpack files are being instrumented by babel/webpack and NOT karma-coverage. All other examples that are being instrumented by karma-coverage properly remap the stacktraces

The fix for this issues involves relying on the istanbul-lib-instrument api accepting an optional input source map that gets merged into the source map generated from the instrumentation code. Since the default instrumenter supports this, most cases should be fine.

The older instrumenters (istanbul 0.x and ibrik) do not do this, so this change would drop support for them. I would argue that support was already dropped in #377, since the change to use istanbul-lib-report/istanbul-reports broke use of coverage maps generated by the older instrumenters (the coverage map structure changed quite a bit).

[xirzec]

@johnjbarton Any chance we can get a release with these changes soon?

I'm currently hitting the exact issue solved by this PR. 😅

[karmarunnerbot]

🎉 This PR is included in version 2.0.2 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀