Skip to content

Commit

Permalink
Merge pull request #2149 from mprobst/source-map-fixes
Browse files Browse the repository at this point in the history 
feat(reporter): improve source map handling and reporting.
  • Loading branch information
@dignifiedquire
dignifiedquire committed May 25, 2016
2 parents 9a972b1 + cf0be47 commit 13368e6
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 20 deletions.
17 changes: 9 additions & 8 deletions lib/reporter.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var util = require('util')
var resolve = require('url').resolve
var log = require('./logger').create('reporter')
var MultiReporter = require('./reporters/multi')
var baseReporterDecoratorFactory = require('./reporters/base').decoratorFactory
Expand All @@ -23,7 +24,7 @@ var createErrorFormatter = function (basePath, emitter, SourceMapConsumer) {
}

var URL_REGEXP = new RegExp('(?:https?:\\/\\/[^\\/]*)?\\/?' +
'(base|absolute)' + // prefix
'(base/|absolute)' + // prefix, including slash for base/ to create relative paths.
'((?:[A-z]\\:)?[^\\?\\s\\:]*)' + // path
'(\\?\\w*)?' + // sha
'(\\:(\\d+))?' + // line
Expand Down Expand Up @@ -53,11 +54,8 @@ var createErrorFormatter = function (basePath, emitter, SourceMapConsumer) {
// remove domain and timestamp from source files
// and resolve base path / absolute path urls into absolute path
var msg = input.replace(URL_REGEXP, function (_, prefix, path, __, ___, line, ____, column) {
if (prefix === 'base') {
path = basePath + path
}

var file = findFile(path)
// Find the file using basePath + path, but use the more readable path down below.
var file = findFile(prefix === 'base/' ? basePath + '/' + path : path)

if (file && file.sourceMap && line) {
line = parseInt(line || '0', 10)
Expand All @@ -72,9 +70,12 @@ var createErrorFormatter = function (basePath, emitter, SourceMapConsumer) {
var original = getSourceMapConsumer(file.sourceMap)
.originalPositionFor({line: line, column: (column || 0), bias: bias})

// Source maps often only have a local file name, resolve to turn into a full path if
// the path is not absolute yet.
var sourcePath = resolve(path, original.source)
var formattedColumn = column ? util.format(':%s', column) : ''
return util.format('%s:%d%s <- %s:%d:%d', path, line, formattedColumn, original.source,
original.line, original.column)
return util.format('%s:%d:%d <- %s:%d%s', sourcePath, original.line, original.column,
path, line, formattedColumn)
} catch (e) {
log.warn('SourceMap position not found for trace: %s', msg)
// Fall back to non-source-mapped formatting.
Expand Down
41 changes: 29 additions & 12 deletions test/unit/reporter.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,18 @@ describe('reporter', () => {
})

it('should remove domain from files', () => {
expect(formatError('file http://localhost:8080/base/usr/a.js and http://127.0.0.1:8080/base/home/b.js')).to.be.equal('file /usr/a.js and /home/b.js\n')
expect(formatError('file http://localhost:8080/base/usr/a.js and http://127.0.0.1:8080/absolute/home/b.js')).to.be.equal('file usr/a.js and /home/b.js\n')
})

// TODO(vojta): enable once we serve source under urlRoot
it.skip('should handle non default karma service folders', () => {
formatError = m.createErrorFormatter('', '/_karma_/')
expect(formatError('file http://localhost:8080/_karma_/base/usr/a.js and http://127.0.0.1:8080/_karma_/base/home/b.js')).to.be.equal('file /usr/a.js and /home/b.js\n')
expect(formatError('file http://localhost:8080/_karma_/base/usr/a.js and http://127.0.0.1:8080/_karma_/base/home/b.js')).to.be.equal('file usr/a.js and home/b.js\n')
})

it('should remove shas', () => {
var ERROR = 'file http://localhost:8080/base/usr/file.js?6e31cb249ee5b32d91f37ea516ca0f84bddc5aa9 and http://127.0.0.1:8080/absolute/home/file.js?6e31cb249ee5b32d91f37ea516ca0f84bddc5aa9'
expect(formatError(ERROR)).to.be.equal('file /usr/file.js and /home/file.js\n')
expect(formatError(ERROR)).to.be.equal('file usr/file.js and /home/file.js\n')
})

it('should indent all lines', () => {
Expand All @@ -65,7 +65,7 @@ describe('reporter', () => {

it('should restore base paths', () => {
formatError = m.createErrorFormatter('/some/base', emitter)
expect(formatError('at http://localhost:123/base/a.js?123')).to.equal('at /some/base/a.js\n')
expect(formatError('at http://localhost:123/base/a.js?123')).to.equal('at a.js\n')
})

it('should restore absolute paths', () => {
Expand All @@ -90,10 +90,11 @@ describe('reporter', () => {

describe('source maps', () => {
var originalPositionForCallCount = 0
var sourceMappingPath = null

class MockSourceMapConsumer {
constructor (sourceMap) {
this.source = sourceMap.content.replace('SOURCE MAP ', '/original/')
this.source = sourceMap.content.replace('SOURCE MAP ', sourceMappingPath)
}

originalPositionFor (position) {
Expand All @@ -112,6 +113,7 @@ describe('reporter', () => {

beforeEach(() => {
originalPositionForCallCount = 0
sourceMappingPath = '/original/'
})

MockSourceMapConsumer.GREATEST_LOWER_BOUND = 1
Expand All @@ -127,7 +129,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://localhost:123/base/b.js:2:6'
expect(formatError(ERROR)).to.equal('at /some/base/b.js:2:6 <- /original/b.js:4:8\n')
expect(formatError(ERROR)).to.equal('at /original/b.js:4:8 <- b.js:2:6\n')
done()
})
})
Expand All @@ -142,7 +144,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://localhost:123/base/b.js:2'
expect(formatError(ERROR)).to.equal('at /some/base/b.js:2 <- /original/b.js:4:2\n')
expect(formatError(ERROR)).to.equal('at /original/b.js:4:2 <- b.js:2\n')
done()
})
})
Expand All @@ -157,7 +159,22 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at /base/b.js:2:6'
expect(formatError(ERROR)).to.equal('at /some/base/b.js:2:6 <- /original/b.js:4:8\n')
expect(formatError(ERROR)).to.equal('at /original/b.js:4:8 <- b.js:2:6\n')
done()
})
})

it('should resolve relative urls from source maps', (done) => {
sourceMappingPath = 'original/' // Note: relative path.
formatError = m.createErrorFormatter('/some/base', emitter, MockSourceMapConsumer)
var servedFiles = [new File('/some/base/path/a.js')]
servedFiles[0].sourceMap = {content: 'SOURCE MAP a.fancyjs'}

emitter.emit('file_list_modified', {served: servedFiles})

_.defer(() => {
var ERROR = 'at /base/path/a.js:2:6'
expect(formatError(ERROR)).to.equal('at path/original/a.fancyjs:4:8 <- path/a.js:2:6\n')
done()
})
})
Expand All @@ -172,7 +189,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://localhost:123/base/b.js:0:0'
expect(formatError(ERROR)).to.equal('at /some/base/b.js\n')
expect(formatError(ERROR)).to.equal('at b.js\n')
done()
})
})
Expand All @@ -187,7 +204,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://localhost:123/base/b.js'
expect(formatError(ERROR)).to.equal('at /some/base/b.js\n')
expect(formatError(ERROR)).to.equal('at b.js\n')
expect(originalPositionForCallCount).to.equal(0)
done()
})
Expand All @@ -208,7 +225,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://localhost:123/absoluteC:/a/b/c.js:2:6'
expect(formatError(ERROR)).to.equal('at C:/a/b/c.js:2:6 <- /original/b.js:4:8\n')
expect(formatError(ERROR)).to.equal('at c:/original/b.js:4:8 <- C:/a/b/c.js:2:6\n')
done()
})
})
Expand All @@ -218,7 +235,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://localhost:123/absoluteC:/a/b/c.js?da39a3ee5e6:2:6'
expect(formatError(ERROR)).to.equal('at C:/a/b/c.js:2:6 <- /original/b.js:4:8\n')
expect(formatError(ERROR)).to.equal('at c:/original/b.js:4:8 <- C:/a/b/c.js:2:6\n')
done()
})
})
Expand Down

0 comments on commit 13368e6

Please sign in to comment.