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

Optimized ranges (fix for issue #150) #173

Merged
merged 6 commits into from
Jan 9, 2022
Merged
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}]
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
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
33 changes: 33 additions & 0 deletions lib/range.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* ...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, inclusive = false) => {
let start = 0
let end = lines.length - 1

/**
* I consider this a temporary solution until I find an alternaive way to fix the "off by one issue"
*/
const extStartCol = inclusive ? startCol - 1 : startCol

while (start < end) {
const mid = (start + end) >> 1
if (lines[mid].startCol <= startCol && lines[mid].endCol > extStartCol) {
start = mid
end = start
} else if (lines[mid].startCol > startCol) {
end = mid - 1
} else {
start = mid + 1
}
}
if (start === end) {
while (end < lines.length && extStartCol < lines[end].endCol && endCol >= lines[end].startCol) {
++end
}
return lines.slice(start, end)
} else {
return []
}
}
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, true)
if (!lines.length) return {}

const start = originalPositionTryBoth(
sourceMap,
lines[0].line,
Math.max(0, startCol - lines[0].startCol)
)
if (!(start && start.source)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we might not need to change this logic, if we figure out what's going on with the range detection logic.

Copy link
Contributor Author

@glromeo glromeo Jan 3, 2022

Choose a reason for hiding this comment

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

I rearranged those ifs to fail fast and avoid the costly originalEndPositionFor...but admittedly it's a nano-optimization...

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
28 changes: 16 additions & 12 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 } = require('./range')
const compatError = Error(`requires Node.js ${require('../package.json').engines.node}`)
let readFile = () => { throw compatError }
try {
Expand Down Expand Up @@ -112,22 +113,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)') {
let lines
if (block.functionName === '(empty-report)') {
// (empty-report), this will result in a report that has all lines zeroed out.
lines = covSource.lines.filter((line) => {
line.count = 0
this.all = true
return true
}
})
this.all = lines.length > 0
} else {
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 +142,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 +155,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 Down
29 changes: 0 additions & 29 deletions tap-snapshots/test/v8-to-istanbul.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -1877,38 +1877,9 @@ Object {
"0": Array [
1,
],
"1": Array [
1,
],
},
"branchMap": Object {
"0": Object {
"line": 2,
"loc": Object {
"end": Object {
"column": 7,
"line": 8,
},
"start": Object {
"column": -1,
"line": 2,
},
},
"locations": Array [
Object {
"end": Object {
"column": 7,
"line": 8,
},
"start": Object {
"column": -1,
"line": 2,
},
},
],
"type": "branch",
},
"1": Object {
"line": 2,
"loc": Object {
"end": Object {
Expand Down
35 changes: 35 additions & 0 deletions test/range.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/* global describe, it */
const { sliceRange } = require('../lib/range')

require('tap').mochaGlobals()
require('should')

describe('range', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Thaniks for starting to flesh out this suite 👌

describe('slice range', () => {
it('can deal with empty arrays', () => {
sliceRange([], 0, 1).should.eql([])
})
it('can find lines that match exactly', () => {
const THREE_LINES = [
{ startCol: 0, endCol: 10 },
{ startCol: 11, endCol: 20 },
{ startCol: 21, endCol: 30 }
]
sliceRange(THREE_LINES, 0, 10).should.eql([THREE_LINES[0]])
sliceRange(THREE_LINES, 11, 20).should.eql([THREE_LINES[1]])
sliceRange(THREE_LINES, 21, 30).should.eql([THREE_LINES[2]])
})
it('can cover a range that spans two lines', () => {
const SIX_LINES = [
{ startCol: 0, endCol: 3 },
{ startCol: 4, endCol: 10 },
{ startCol: 11, endCol: 14 },
{ startCol: 15, endCol: 20 },
{ startCol: 21, endCol: 24 },
{ startCol: 25, endCol: 30 }
]
sliceRange(SIX_LINES, 5, 14).should.eql(SIX_LINES.slice(1, 3))
sliceRange(SIX_LINES, 15, 21).should.eql(SIX_LINES.slice(3, 5))
})
})
})
25 changes: 25 additions & 0 deletions test/source.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* global describe, it */

const CovSource = require('../lib/source')
const { SourceMapConsumer } = require('source-map')

require('tap').mochaGlobals()
require('should')
Expand Down Expand Up @@ -38,6 +39,30 @@ describe('Source', () => {
const source = new CovSource(sourceRaw, 0)
source.offsetToOriginalRelative(undefined, Infinity, Infinity).should.deepEqual({})
})

it('range crossing two sourcemaps', async () => {
const sourceRaw = `\
(() => {
// hello.ts
function hello() {
console.log("hello world");
}

// greet.ts
hello();
})();
//# sourceMappingURL=greet.js.map\
`
const source = new CovSource(sourceRaw, 0)
const sourceMap = await new SourceMapConsumer({
version: 3,
sources: ['../hello.ts', '../greet.ts'],
sourcesContent: ['export function hello() {\r\n console.log("hello world")\r\n}', 'import {hello} from "./hello"\r\n\r\nhello()\r\n'],
mappings: ';;AAAO,mBAAiB;AACtB,YAAQ,IAAI;AAAA;;;ACCd;',
names: []
})
source.offsetToOriginalRelative(sourceMap, 25, 97).should.deepEqual({})
})
})

describe('ignore', () => {
Expand Down