Skip to content

Commit b3ad434

Browse files
bnoordhuisbnoordhuis
committed
Make download() function testable.
Move the function around so it can be tested and add a regression test. As a policy vs. mechanism thing, change the control flow to handle exceptions at the call site, not inside the download function. PR-URL: #837 Reviewed-By: Rod Vagg <[email protected]>
1 parent 89692c9 commit b3ad434

2 files changed

Lines changed: 94 additions & 47 deletions

File tree

‎lib/install.js‎

Lines changed: 57 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11

22
module.exports = exports = install
33

4+
module.exports.test = { download: download }
5+
46
exports.usage = 'Install node development files for the specified node version.'
57

68
/**
@@ -110,45 +112,6 @@ function install (gyp, argv, callback) {
110112
go()
111113
}
112114

113-
function download (url) {
114-
log.http('GET', url)
115-
116-
var req = null
117-
var requestOpts = {
118-
uri: url
119-
, headers: {
120-
'User-Agent': 'node-gyp v' + gyp.version + ' (node ' + process.version + ')'
121-
}
122-
}
123-
124-
// basic support for a proxy server
125-
var proxyUrl = gyp.opts.proxy
126-
|| process.env.http_proxy
127-
|| process.env.HTTP_PROXY
128-
|| process.env.npm_config_proxy
129-
if (proxyUrl) {
130-
if (/^https?:\/\//i.test(proxyUrl)) {
131-
log.verbose('download', 'using proxy url: "%s"', proxyUrl)
132-
requestOpts.proxy = proxyUrl
133-
} else {
134-
log.warn('download', 'ignoring invalid "proxy" config setting: "%s"', proxyUrl)
135-
}
136-
}
137-
try {
138-
// The "request" constructor can throw sometimes apparently :(
139-
// See: https://github.com/nodejs/node-gyp/issues/114
140-
req = request(requestOpts)
141-
} catch (e) {
142-
cb(e)
143-
}
144-
if (req) {
145-
req.on('response', function (res) {
146-
log.http(res.statusCode, url)
147-
})
148-
}
149-
return req
150-
}
151-
152115
function getContentSha(res, callback) {
153116
var shasum = crypto.createHash('sha256')
154117
res.on('data', function (chunk) {
@@ -218,8 +181,11 @@ function install (gyp, argv, callback) {
218181
return
219182
}
220183

221-
var req = download(release.tarballUrl)
222-
if (!req) return
184+
try {
185+
var req = download(gyp, process.env, release.tarballUrl)
186+
} catch (e) {
187+
return cb(e)
188+
}
223189

224190
// something went wrong downloading the tarball?
225191
req.on('error', function (err) {
@@ -311,8 +277,12 @@ function install (gyp, argv, callback) {
311277
var shasumsPath = path.resolve(devDir, 'SHASUMS256.txt')
312278

313279
log.verbose('checksum url', release.shasumsUrl)
314-
var req = download(release.shasumsUrl)
315-
if (!req) return
280+
try {
281+
var req = download(gyp, process.env, release.shasumsUrl)
282+
} catch (e) {
283+
return cb(e)
284+
}
285+
316286
req.on('error', done)
317287
req.on('response', function (res) {
318288
if (res.statusCode !== 200) {
@@ -358,8 +328,12 @@ function install (gyp, argv, callback) {
358328
if (err) return done(err)
359329
log.verbose('streaming 32-bit ' + release.name + '.lib to:', libPath32)
360330

361-
var req = download(release.libUrl32)
362-
if (!req) return
331+
try {
332+
var req = download(gyp, process.env, release.libUrl32, cb)
333+
} catch (e) {
334+
return cb(e)
335+
}
336+
363337
req.on('error', done)
364338
req.on('response', function (res) {
365339
if (res.statusCode !== 200) {
@@ -384,8 +358,12 @@ function install (gyp, argv, callback) {
384358
if (err) return done(err)
385359
log.verbose('streaming 64-bit ' + release.name + '.lib to:', libPath64)
386360

387-
var req = download(release.libUrl64)
388-
if (!req) return
361+
try {
362+
var req = download(gyp, process.env, release.libUrl64, cb)
363+
} catch (e) {
364+
return cb(e)
365+
}
366+
389367
req.on('error', done)
390368
req.on('response', function (res) {
391369
if (res.statusCode !== 200) {
@@ -444,3 +422,35 @@ function install (gyp, argv, callback) {
444422
}
445423

446424
}
425+
426+
function download (gyp, env, url) {
427+
log.http('GET', url)
428+
429+
var requestOpts = {
430+
uri: url
431+
, headers: {
432+
'User-Agent': 'node-gyp v' + gyp.version + ' (node ' + process.version + ')'
433+
}
434+
}
435+
436+
// basic support for a proxy server
437+
var proxyUrl = gyp.opts.proxy
438+
|| env.http_proxy
439+
|| env.HTTP_PROXY
440+
|| env.npm_config_proxy
441+
if (proxyUrl) {
442+
if (/^https?:\/\//i.test(proxyUrl)) {
443+
log.verbose('download', 'using proxy url: "%s"', proxyUrl)
444+
requestOpts.proxy = proxyUrl
445+
} else {
446+
log.warn('download', 'ignoring invalid "proxy" config setting: "%s"', proxyUrl)
447+
}
448+
}
449+
450+
var req = request(requestOpts)
451+
req.on('response', function (res) {
452+
log.http(res.statusCode, url)
453+
})
454+
455+
return req
456+
}

‎test/test-download.js‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
'use strict'
2+
3+
var http = require('http')
4+
var test = require('tape')
5+
var install = require('../lib/install')
6+
7+
test('download over http', function (t) {
8+
t.plan(2)
9+
10+
var server = http.createServer(function (req, res) {
11+
t.strictEqual(req.headers['user-agent'],
12+
'node-gyp v42 (node ' + process.version + ')')
13+
res.end('ok')
14+
server.close()
15+
})
16+
17+
var host = '127.0.0.1'
18+
server.listen(0, host, function () {
19+
var port = this.address().port
20+
var gyp = {
21+
opts: {},
22+
version: '42',
23+
}
24+
var url = 'http://' + host + ':' + port
25+
var req = install.test.download(gyp, {}, url)
26+
req.on('response', function (res) {
27+
var body = ''
28+
res.setEncoding('utf8')
29+
res.on('data', function(data) {
30+
body += data
31+
})
32+
res.on('end', function() {
33+
t.strictEqual(body, 'ok')
34+
})
35+
})
36+
})
37+
})

0 commit comments

Comments
 (0)