Repository: karma-runner/karma
Pull request #3721 (2021-10-31 12:24:11)

fix(reporter): do not print warning if stack trace contains generated code invocation
For some projects, a preprocessor like TypeScript may run to downlevel
certain features to a lower ECMAScript target. e.g. a project may
consume Angular for example, which ships ES2020.

If TypeScript, ESBuild, Babel etc. is used to do this, they may inject
generated code which does not map to any original source. If any of
these helpers (the generated code) is then part of a stack trace, Karma
will incorrectly report an error for an unresolved source map position.

Generated code is valid within source maps and can be denoted as
mappings with a 1-variable-length mapping. See the source map spec:
https://sourcemaps.info/spec.html.

The warning for generated code is especially bad when the majority of
file paths, and the actually relevant-portions in the stack are
resolved properly. e.g.

Errors initially look like this without the source mapping processing
of Karma:

```js
Expected false to be true.
<Jasmine>
runHarnessTests/</<@http://localhost:9877/base/angular_material/src/material/select/testing/unit_tests_bundle_spec.js:72191:40
fulfilled@http://localhost:9877/base/angular_material/src/material/select/testing/unit_tests_bundle_spec.js:26:26
invoke@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-evergreen.js:372:26
ProxyZoneSpec.prototype.onInvoke@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-testing.js:301:43
invoke@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-evergreen.js:371:52
run@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-evergreen.js:134:43
scheduleResolveOrReject/<@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-evergreen.js:1276:36
invokeTask@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-evergreen.js:406:31
ProxyZoneSpec.prototype.onInvokeTask@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-testing.js:332:43
invokeTask@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-evergreen.js:405:60
runTask@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-evergreen.js:178:47
drainMicroTaskQueue@http://localhost:9877/base/npm/node_modules/zone.js/dist/zone-evergreen.js:582:35
```

A helper function shows up in the stacktrace but has no original mapping as it is
purely generated by TypeScript/ESbuild etc. The following warning is
printed and pollutes the test output while the remaining stack trace
paths (as said before), have been remapped properly:

```
SourceMap position not found for trace: http://localhost:9877/base/angular_material/src/material/select/testing/unit_tests_bundle_spec.js:26:26
```

The expected resolved stacktrace looks like this after the transformation:

```js
        Expected false to be true.
        <Jasmine>
        runHarnessTests/</<@../../src/material/select/testing/shared.spec.ts:123:38 <- angular_material/src/material/select/testing/unit_tests_bundle_spec.js:72191:40
        fulfilled@angular_material/src/material/select/testing/unit_tests_bundle_spec.js:26:26
        invoke@npm/node_modules/zone.js/dist/zone-evergreen.js:372:26
        ProxyZoneSpec.prototype.onInvoke@npm/node_modules/zone.js/dist/zone-testing.js:301:43
        invoke@npm/node_modules/zone.js/dist/zone-evergreen.js:371:52
        run@npm/node_modules/zone.js/dist/zone-evergreen.js:134:43
        scheduleResolveOrReject/<@npm/node_modules/zone.js/dist/zone-evergreen.js:1276:36
        invokeTask@npm/node_modules/zone.js/dist/zone-evergreen.js:406:31
        ProxyZoneSpec.prototype.onInvokeTask@npm/node_modules/zone.js/dist/zone-testing.js:332:43
        invokeTask@npm/node_modules/zone.js/dist/zone-evergreen.js:405:60
        runTask@npm/node_modules/zone.js/dist/zone-evergreen.js:178:47
        drainMicroTaskQueue@npm/node_modules/zone.js/dist/zone-evergreen.js:582:35
```

More details on the scenario here:
https://gist.github.com/devversion/549d25915c2dc98a8896ba4408a1e27c.

jginsburgn: @devversion thanks for this PR. Can you please correct your commit message to make sure our CI passes?
devversion: @jginsburgn rebased and fixed the commit message header (the limit is quite low tbh)
jginsburgn: > @jginsburgn rebased and fixed the commit message header (the limit is quite low tbh)

I agree! Can you file an issue to address this concern there?
devversion: @jginsburgn created #3722. hope that works
devversion: @devoto13 thanks for reviewing. Is there anything I can do to move that forward? 
devoto13: @devversion I think @jginsburgn needs to run qualifications tests against the Google monorepo and then merge this PR.
jginsburgn: > @devversion I think @jginsburgn needs to run qualification tests against the Google monorepo and then merge this PR.

@devversion I am on it :)

Patch:
diff --git a/lib/reporter.js b/lib/reporter.js
index de5ae7f9..88fd00c4 100644
--- a/lib/reporter.js
+++ b/lib/reporter.js
@@ -48,7 +48,7 @@ function createErrorFormatter (config, emitter, SourceMapConsumer) {
       input = JSON.stringify(input, null, indentation)
     }
 
-    let msg = input.replace(URL_REGEXP, function (_, prefix, path, __, ___, line, ____, column) {
+    let msg = input.replace(URL_REGEXP, function (stackTracePath, prefix, path, __, ___, line, ____, column) {
       const normalizedPath = prefix === 'base/' ? `${basePath}/${path}` : path
       const file = lastServedFiles.find((file) => file.path === normalizedPath)
 
@@ -64,12 +64,21 @@ function createErrorFormatter (config, emitter, SourceMapConsumer) {
           const zeroBasedColumn = Math.max(0, (column || 1) - 1)
           const original = getSourceMapConsumer(file.sourceMap).originalPositionFor({ line, column: zeroBasedColumn, bias })
 
+          // If there is no original position/source for the current stack trace path, then
+          // we return early with the formatted generated position. This handles the case of
+          // generated code which does not map to anything, see Case 1 of the source-map spec.
+          // https://sourcemaps.info/spec.html.
+          if (original.source === null) {
+            return PathUtils.formatPathMapping(path, line, column)
+          }
+
           // Source maps often only have a local file name, resolve to turn into a full path if
           // the path is not absolute yet.
           const oneBasedOriginalColumn = original.column == null ? original.column : original.column + 1
           return `${PathUtils.formatPathMapping(resolve(path, original.source), original.line, oneBasedOriginalColumn)} <- ${PathUtils.formatPathMapping(path, line, column)}`
         } catch (e) {
-          log.warn(`SourceMap position not found for trace: ${input}`)
+          log.warn(`An unexpected error occurred while resolving the original position for: ${stackTracePath}`)
+          log.warn(e)
         }
       }
 


Test patch:
diff --git a/test/unit/reporter.spec.js b/test/unit/reporter.spec.js
index c88c0114..3d69bd06 100644
--- a/test/unit/reporter.spec.js
+++ b/test/unit/reporter.spec.js
@@ -5,6 +5,7 @@ const loadFile = require('mocks').loadFile
 const path = require('path')
 const _ = require('lodash')
 const sinon = require('sinon')
+const logger = require('../../lib/logger')
 
 const File = require('../../lib/file')
 
@@ -127,6 +128,8 @@ describe('reporter', () => {
     describe('source maps', () => {
       let originalPositionForCallCount = 0
       let sourceMappingPath = null
+      let log
+      let logWarnStub
 
       class MockSourceMapConsumer {
         constructor (sourceMap) {
@@ -147,9 +150,36 @@ describe('reporter', () => {
         }
       }
 
+      class MockSourceMapConsumerWithParseError {
+        constructor () {
+          throw new Error('Fake parse error from source map consumer')
+        }
+      }
+
+      class MockSourceMapConsumerWithGeneratedCode {
+        constructor (sourceMap) {
+          this.source = sourceMap.content.replace('SOURCE MAP ', sourceMappingPath)
+        }
+
+        originalPositionFor () {
+          return {
+            source: null,
+            line: null,
+            column: null
+          }
+        }
+      }
+
       beforeEach(() => {
         originalPositionForCallCount = 0
         sourceMappingPath = '/original/'
+
+        log = logger.create('reporter')
+        logWarnStub = sinon.spy(log, 'warn')
+      })
+
+      afterEach(() => {
+        logWarnStub.restore()
       })
 
       MockSourceMapConsumer.GREATEST_LOWER_BOUND = 1
@@ -170,6 +200,51 @@ describe('reporter', () => {
         })
       })
 
+      // Regression test for cases like: https://github.com/karma-runner/karma/pull/1098.
+      // Note that the scenario outlined in the PR should no longer surface due to a check
+      // ensuring that the line always is non-zero, but there could be other parsing errors.
+      it('should handle source map errors gracefully', (done) => {
+        formatError = m.createErrorFormatter({ basePath: '', hostname: 'localhost', port: 123 }, emitter,
+          MockSourceMapConsumerWithParseError)
+
+        const servedFiles = [new File('/a.js'), new File('/b.js')]
+        servedFiles[0].sourceMap = { content: 'SOURCE MAP a.js' }
+        servedFiles[1].sourceMap = { content: 'SOURCE MAP b.js' }
+
+        emitter.emit('file_list_modified', { served: servedFiles })
+
+        _.defer(() => {
+          const ERROR = 'at http://localhost:123/base/b.js:2:6'
+          expect(formatError(ERROR)).to.equal('at b.js:2:6\n')
+          expect(logWarnStub.callCount).to.equal(2)
+          expect(logWarnStub).to.have.been.calledWith('An unexpected error occurred while resolving the original position for: http://localhost:123/base/b.js:2:6')
+          expect(logWarnStub).to.have.been.calledWith(sinon.match({ message: 'Fake parse error from source map consumer' }))
+          done()
+        })
+      })
+
+      // Generated code can be added by e.g. TypeScript or Babel when it transforms
+      // native async/await to generators. Calls would then be wrapped with a helper
+      // that is generated and does not map to anything, so-called generated code that
+      // is allowed as case #1 in the source map spec.
+      it('should not warn for trace file portion for generated code', (done) => {
+        formatError = m.createErrorFormatter({ basePath: '', hostname: 'localhost', port: 123 }, emitter,
+          MockSourceMapConsumerWithGeneratedCode)
+
+        const servedFiles = [new File('/a.js'), new File('/b.js')]
+        servedFiles[0].sourceMap = { content: 'SOURCE MAP a.js' }
+        servedFiles[1].sourceMap = { content: 'SOURCE MAP b.js' }
+
+        emitter.emit('file_list_modified', { served: servedFiles })
+
+        _.defer(() => {
+          const ERROR = 'at http://localhost:123/base/b.js:2:6'
+          expect(formatError(ERROR)).to.equal('at b.js:2:6\n')
+          expect(logWarnStub.callCount).to.equal(0)
+          done()
+        })
+      })
+
       it('should rewrite stack traces (when basePath is empty)', (done) => {
         formatError = m.createErrorFormatter({ basePath: '', hostname: 'localhost', port: 123 }, emitter, MockSourceMapConsumer)
         const servedFiles = [new File('/a.js'), new File('/b.js')]