Skip to content
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

Fixed startCol/endCol vs startOffset/endOffset mismatch due to greediness and whitespaces #172

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[*.{.js,.json}]
glromeo marked this conversation as resolved.
Show resolved Hide resolved
end_of_line = lf
indent_size = 2
indent_style = space
5 changes: 4 additions & 1 deletion .gitattributes
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
test/fixtures/scripts/needs-compile.js eol=crlf
test/fixtures/scripts/needs-compile.compiled.js eol=crlf
test/fixtures/scripts/needs-compile.compiled.js.map eol=crlf
test/fixtures/scripts/needs-compile.compiled.js.map eol=crlf
test/fixtures/scripts/branches.covered.js eol=lf
glromeo marked this conversation as resolved.
Show resolved Hide resolved
test/fixtures/scripts/branches.js eol=lf
test/fixtures/scripts/functions.js eol=lf
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@ coverage
node_modules
.idea
.vscode
*.iml
yarn.lock
2 changes: 2 additions & 0 deletions lib/line.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ module.exports = class CovLine {
// note that startCol and endCol are absolute positions
// within a file, not relative to the line.
this.startCol = startCol
this.minCol = startCol + lineStr.length - lineStr.trimStart().length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trimming lines may actually be the cause of the remapping issues I saw in c8, since we specifically see breakage with tests that exercise SourceMap files.

The source map lookup logic is quite finicky, and trimming before performing it may need to be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcoe the investigation I did suggests me that the tests are broken but that the accuracy of the coverage improves. As I posted here and in the node-fetch comparisons the existing logic seems to often miss the first line of a range. I look forward to your feedback on those examples


// the line length itself does not include the newline characters,
// these are however taken into account when enumerating absolute offset.
const matchedNewLineChar = lineStr.match(/\r?\n$/u)
const newLineLength = matchedNewLineChar ? matchedNewLineChar[0].length : 0
this.endCol = startCol + lineStr.length - newLineLength
this.maxCol = startCol + lineStr.trimEnd().length

// we start with all lines having been executed, and work
// backwards zeroing out lines based on V8 output.
Expand Down
40 changes: 40 additions & 0 deletions lib/range.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* ...something resembling a binary search, to find the lowest line within the range.
* And then you could break as soon as the line is longer than the range...
*/
module.exports.sliceRange = (lines, startCol, endCol) => {
let lower = 0
let upper = lines.length - 1

let s
while (lower <= upper) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate that you pulled this into a helper file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, I actually spent some time asking myself: is it better to do 2 binary searches or not? At the end I preferred to opt for only 1 binary search and then linear scan because it's unlikely that in a large file a range would be too big anyway so I guess that the linear search should perform better

s = (lower + upper) >> 1
if (startCol < lines[s].startCol) {
upper = s - 1
} else if (startCol >= lines[s].endCol) {
lower = s + 1
} else {
let e = s + 1
while (e < lines.length && endCol > lines[e].startCol) {
++e
}
return lines.slice(s, e)
}
}
if (s >= 0 && lines[s].startCol >= startCol) {
let e = s + 1
while (e < lines.length && endCol > lines[e].startCol) {
++e
}
return lines.slice(s, e)
}
return []
}

module.exports.trimRange = (rawSource, range) => {
const slice = rawSource.slice(range.startOffset, range.endOffset)
return {
startOffset: range.startOffset + (slice.length - slice.trimStart().length),
endOffset: range.endOffset - (slice.length - slice.trimEnd().length)
}
}
16 changes: 7 additions & 9 deletions lib/source.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const CovLine = require('./line')
const { sliceRange } = require('./range')
const { GREATEST_LOWER_BOUND, LEAST_UPPER_BOUND } = require('source-map').SourceMapConsumer

module.exports = class CovSource {
Expand Down Expand Up @@ -75,27 +76,24 @@ module.exports = class CovSource {
// given a start column and end column in absolute offsets within
// a source file (0 - EOF), returns the relative line column positions.
offsetToOriginalRelative (sourceMap, startCol, endCol) {
const lines = this.lines.filter((line, i) => {
return startCol <= line.endCol && endCol >= line.startCol
})
const lines = sliceRange(this.lines, startCol, endCol)
if (!lines.length) return {}

const start = originalPositionTryBoth(
sourceMap,
lines[0].line,
Math.max(0, startCol - lines[0].startCol)
)
if (!(start && start.source)) {
glromeo marked this conversation as resolved.
Show resolved Hide resolved
return {}
}

let end = originalEndPositionFor(
sourceMap,
lines[lines.length - 1].line,
endCol - lines[lines.length - 1].startCol
)

if (!(start && end)) {
return {}
}

if (!(start.source && end.source)) {
if (!(end && end.source)) {
return {}
}

Expand Down
38 changes: 24 additions & 14 deletions lib/v8-to-istanbul.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const { fileURLToPath } = require('url')
const CovBranch = require('./branch')
const CovFunction = require('./function')
const CovSource = require('./source')
const { sliceRange, trimRange } = require('./range')
const compatError = Error(`requires Node.js ${require('../package.json').engines.node}`)
let readFile = () => { throw compatError }
try {
Expand Down Expand Up @@ -42,6 +43,7 @@ module.exports = class V8ToIstanbul {

async load () {
const rawSource = this.sources.source || await readFile(this.path, 'utf8')
this.rawSource = rawSource
this.rawSourceMap = this.sources.sourceMap ||
// if we find a source-map (either inline, or a .map file) we load
// both the transpiled and original source, both of which are used during
Expand Down Expand Up @@ -112,22 +114,25 @@ module.exports = class V8ToIstanbul {
if (this.excludePath(path)) {
return
}
const lines = covSource.lines.filter(line => {
// Upstream tooling can provide a block with the functionName
// (empty-report), this will result in a report that has all
// lines zeroed out.
if (block.functionName === '(empty-report)') {

// (empty-report), this will result in a report that has all
// lines zeroed out.
if (block.functionName === '(empty-report)') {
this.all = true
covSource.lines.forEach(line => {
line.count = 0
this.all = true
return true
}
})
}

const lines = this.all ? covSource.lines : sliceRange(covSource.lines, startCol, endCol)
if (!lines.length) {
return
}

return startCol < line.endCol && endCol >= line.startCol
})
const startLineInstance = lines[0]
const endLineInstance = lines[lines.length - 1]

if (block.isBlockCoverage && lines.length) {
if (block.isBlockCoverage) {
this.branches[path] = this.branches[path] || []
// record branches.
this.branches[path].push(new CovBranch(
Expand All @@ -138,7 +143,7 @@ module.exports = class V8ToIstanbul {
range.count
))

// if block-level granularity is enabled, we we still create a single
// if block-level granularity is enabled, we still create a single
// CovFunction tracking object for each set of ranges.
if (block.functionName && i === 0) {
this.functions[path] = this.functions[path] || []
Expand All @@ -151,7 +156,7 @@ module.exports = class V8ToIstanbul {
range.count
))
}
} else if (block.functionName && lines.length) {
} else if (block.functionName) {
this.functions[path] = this.functions[path] || []
// record functions.
this.functions[path].push(new CovFunction(
Expand All @@ -164,6 +169,11 @@ module.exports = class V8ToIstanbul {
))
}

const {
startCol: minCol,
endCol: maxCol
} = this._maybeRemapStartColEndCol(trimRange(this.rawSource, range))

// record the lines (we record these as statements, such that we're
// compatible with Istanbul 2.0).
lines.forEach(line => {
Expand All @@ -176,7 +186,7 @@ module.exports = class V8ToIstanbul {
// set to 0, and is set if the special comment /* c8 ignore next */
// is used.

if (startCol <= line.startCol && endCol >= line.endCol && !line.ignore) {
if (minCol <= line.minCol && maxCol >= line.maxCol && !line.ignore) {
line.count = range.count
}
})
Expand Down
Loading