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

Jump to bottom

added fix for #181 but without any changes to test #183

Closed
wants to merge 2 commits into from
Closed

added fix for #181 but without any changes to test #183

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1db688
added fix for #181 but without any changes to test
dotnetCarpenter Oct 30, 2015
ead68ff
fix wrong assumption - that undefined is not traversed by Array.forEach
dotnetCarpenter Oct 31, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 32 additions & 28 deletions bin/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ var coverage = pipeToService
var coverageReport

var nycBin = require.resolve('nyc/bin/nyc.js')
var coverallsBin = require.resolve('coveralls/bin/coveralls.js')
var codecovBin = require.resolve('codecov.io/bin/codecov.io.js')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

redundant, since the introduction of the services array

for (var i = 0; i < args.length; i++) {
var arg = args[i]
Expand Down Expand Up @@ -228,39 +226,45 @@ if (coverageReport && !global.__coverage__ && files.length === 0) {
var args = [nycBin, 'report', '--reporter', coverageReport]
var child

// automatically hook into coveralls
// automatically hook into coveralls and/or codecov
if (coverageReport === 'text-lcov' && pipeToService) {
child = spawn(node, args)
var covBin, covName

if (process.env.COVERALLS_REPO_TOKEN) {
covBin = coverallsBin
covName = 'Coveralls'
} else if (process.env.CODECOV_TOKEN) {
covBin = codecovBin
covName = 'Codecov'
}

var ca = spawn(node, [covBin], {
stdio: [ 'pipe', 1, 2 ],
env: process.env
})
child.stdout.pipe(ca.stdin)
ca.on('close', function (code, signal) {
if (signal)
process.kill(process.pid, signal)
else if (code)
process.exit(code)
else
console.log('Successfully piped to ' + covName)
var services = [
process.env.COVERALLS_REPO_TOKEN && {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not needed since we have the services array

covBin: require.resolve('coveralls/bin/coveralls.js')
, covName: 'Coveralls'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the main issue of #181 that #183 is fixing. Here we say if else if, in #183 we say forEach

}
, process.env.CODECOV_TOKEN && {
covBin: require.resolve('codecov.io/bin/codecov.io.js')
, covName: 'Codecov'
}
].filter(function(s) {
return !!s // remove undefined services
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved inside the forEach

signalExit(function (code, signal) {
child.kill('SIGHUP')
ca.kill('SIGHUP')

services.forEach(function(s) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

forEach is new

var ca = spawn(node, [s.covBin], {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The content is pretty much the same as it has always been. Since the forEach argument is a function, we get a function scope environment. That is, each variable is stored in the environment which in turns is stored on the heap.
Given this code, we spawn a child process and assign it to ca in this environment. We call the child.stdout.pipe function with our instance of ca.stdin, basically telling the nyc/bin/nyc.js process to pipe its output to our ca.stdin. This happens for each service (maximum 2, minimum 0), since child.stdout.pipe can pipe to several functions, this is a non issue.
We then attach an on handler that is stored in the child_process.spawn function scope environment. Lastly, we call signalExit with a new function that has access to child and ca in this environment via a pointer/reference.
If we have 2 services, then we have 2 environments which each spawn a child process in parallel (var ca = spawn(node, [s.covBin], {...). We create new function scope environments for each of these processes, so there can be no conflict.

The only thing that can lead to concern is the question of when the signalExit function argument is run. If we got 2 environments and both have access to child and they both call child.kill('SIGHUP'), is the code then racy? I haven't seen it but I also don't know the internals of signal-exit.

See the "Environments: Managing Variables" section of Speaking JavaScript for an explanation of javascript environment.

stdio: [ 'pipe', 1, 2 ],
env: process.env
})
child.stdout.pipe(ca.stdin)
ca.on('close', function (code, signal) {
if (signal)
process.kill(process.pid, signal)
else if (code)
process.exit(code)
else
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved inside the forEach

console.log('Successfully piped to ' + s.covName)
})
signalExit(function (code, signal) {
child.kill('SIGHUP')
ca.kill('SIGHUP')
})
})
} else {
// otherwise just run the reporter
var child = fg(node, args)
child = fg(node, args)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why var when we do it on line 227

if (coverageReport === 'lcov') {
child.on('exit', function () {
opener('coverage/lcov-report/index.html')
Expand Down