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.

Sign up for GitHub

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

feat: implement coverage details for process tree #416

Merged
merged 2 commits into from
Oct 30, 2016
Merged

feat: implement coverage details for process tree #416

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce440a9
feat: implement coverage details for process tree
addaleax Aug 27, 2016
6a3b4b9
make `loadReports` public, move coverage building logic to lib/process
addaleax Oct 18, 2016
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
23 changes: 20 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,24 @@ NYC.prototype.checkCoverage = function (thresholds) {
}

NYC.prototype._loadProcessInfoTree = function () {
return ProcessInfo.buildProcessTree(this._loadProcessInfos())
var _this = this
var processTree = ProcessInfo.buildProcessTree(this._loadProcessInfos())
Copy link
Member

Choose a reason for hiding this comment

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

I'd be tempted to encapsulate this logic inside process.js; and all that index.js would need to do would be:

console.log(processInfo.render()).


processTree.getCoverageMap(function (filenames, maps) {
var map = libCoverage.createCoverageMap({})

_this._loadReports(filenames).forEach(function (report) {
Copy link
Member

Choose a reason for hiding this comment

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

we can rename _loadReports to loadReports, and process.js can call this method on index.js.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bcoe I just updated this PR which what I think you had in mind – maybe you meant something totally different, this feels a bit icky now (as in, this doesn’t seem quite right from a separation of concerns pov?).

map.merge(report)
})

maps.forEach(function (otherMap) {
map.merge(otherMap)
})

return map
})

return processTree
}

NYC.prototype._loadProcessInfos = function () {
Expand All @@ -479,9 +496,9 @@ NYC.prototype._loadProcessInfos = function () {
})
}

NYC.prototype._loadReports = function () {
NYC.prototype._loadReports = function (filenames) {
var _this = this
var files = fs.readdirSync(this.tempDirectory())
var files = filenames || fs.readdirSync(this.tempDirectory())

var cacheDir = _this.cacheDirectory

Expand Down
22 changes: 21 additions & 1 deletion lib/process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ function ProcessInfo (defaults) {
this.coverageFilename = null
this.nodes = [] // list of children, filled by buildProcessTree()

this._coverageMap = null

for (var key in defaults) {
this[key] = defaults[key]
}
Expand All @@ -25,7 +27,11 @@ Object.defineProperty(ProcessInfo.prototype, 'label', {
return this._label
}

return this.argv.join(' ')
var covInfo = ''
if (this._coverageMap) {
covInfo = '\n ' + this._coverageMap.getCoverageSummary().lines.pct + ' % Lines'
}
return this.argv.join(' ') + covInfo
}
})

Expand Down Expand Up @@ -57,6 +63,20 @@ ProcessInfo.buildProcessTree = function (infos) {
return treeRoot
}

ProcessInfo.prototype.getCoverageMap = function (merger) {
Copy link
Member

Choose a reason for hiding this comment

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

This is cool! it works because we already output the coverage for each subprocess to a separate file named after the pid?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep – the coverage filename is included in the file containing the process info. I thought about magically inferring the filename from the PID, but I wanted to keep the flexibility – mostly because I’ve been hearing Bad Things™ about assuming uniqueness of PIDs on Windows. 😄

if (this._coverageMap) {
return this._coverageMap
}

var childMaps = this.nodes.map(function (child) {
return child.getCoverageMap(merger)
})

this._coverageMap = merger([this.coverageFilename], childMaps)

return this._coverageMap
}

ProcessInfo.prototype.render = function () {
return archy(this)
}
Expand Down
11 changes: 10 additions & 1 deletion test/src/nyc-bin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,14 +580,23 @@ describe('the nyc cli', function () {
stdout.should.match(new RegExp(
'nyc\n' +
'└─┬.*selfspawn-fibonacci.js 5\n' +
' │.* % Lines\n' +
' ├─┬.*selfspawn-fibonacci.js 4\n' +
' │ │.* % Lines\n' +
' │ ├─┬.*selfspawn-fibonacci.js 3\n' +
' │ │ │.* % Lines\n' +
' │ │ ├──.*selfspawn-fibonacci.js 2\n' +
' │ │ │.* % Lines\n' +
' │ │ └──.*selfspawn-fibonacci.js 1\n' +
' │ │ .* % Lines\n' +
' │ └──.*selfspawn-fibonacci.js 2\n' +
' │ .* % Lines\n' +
' └─┬.*selfspawn-fibonacci.js 3\n' +
' │.* % Lines\n' +
' ├──.*selfspawn-fibonacci.js 2\n' +
' └──.*selfspawn-fibonacci.js 1\n'
' │.* % Lines\n' +
' └──.*selfspawn-fibonacci.js 1\n' +
' .* % Lines\n'
))
done()
})
Expand Down