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

Fix: forever on command line #360

Merged
merged 18 commits into from
Apr 16, 2021
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
6 changes: 4 additions & 2 deletions .taprc
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
esm: false
ts: false
jsx: false
flow: false
timeout: 900
coverage: true
branches: 86
functions: 84
lines: 87
statements: 87
12 changes: 10 additions & 2 deletions autocannon.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,17 @@ function start (argv) {
})
})
} else {
initJob(argv).catch((err) => {
// if forever is true then a promise is not returned and we need to try ... catch errors
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: if rather than handling this in a special way we made the forever code path return a promise too, so that we don't have to handle special cases here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my original approach but I ran into problems with the reporting. I just tried again now and it is working 😕 . Would you like me to investigate further or shall I merge as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you. My personal preference would be to do it "properly", but I'm not a maintainer of the repo either so I leave that to you and the maintainers.

try {
const tracker = initJob(argv)
if (tracker.then) {
tracker.catch((err) => {
console.error(err.message)
})
}
} catch (err) {
console.error(err.message)
})
}
Comment on lines +242 to +252
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 actual fix for forever

}
}

Expand Down
13 changes: 10 additions & 3 deletions lib/validate.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const isValidFn = (opt) => (!opt || typeof opt === 'function' || typeof opt ===

const lessThanOneError = (label) => new Error(`${label} can not be less than 1`)
const greaterThanZeroError = (label) => new Error(`${label} must be greater than 0`)
const minIfPresent = (val, min) => val && val < min
const minIfPresent = (val, min) => val !== null && val < min
Copy link
Contributor Author

@skywickenden skywickenden Apr 13, 2021

Choose a reason for hiding this comment

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

This used to return 0 if val was 0 which resulted in a falsy result when it should be true.


function safeRequire (path) {
if (typeof path === 'string') {
Expand Down Expand Up @@ -83,8 +83,15 @@ module.exports = function validateOpts (opts, cbPassedIn) {
}

if (typeof opts.duration === 'string') {
if (/[a-zA-Z]/.exec(opts.duration)) opts.duration = timestring(opts.duration)
else opts.duration = Number(opts.duration.trim())
if (/[a-zA-Z]/.exec(opts.duration)) {
try {
opts.duration = timestring(opts.duration)
} catch (error) {
return error
}
Comment on lines +86 to +91
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If timestring was invalid then the error was being swallowed. It is now returned as per other errors.

} else {
opts.duration = Number(opts.duration.trim())
}
}

if (typeof opts.duration !== 'number') {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"sinon": "^10.0.1",
"split2": "^3.2.2",
"standard": "^16.0.3",
"tap": "^14.10.8"
"tap": "^15.0.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

considering that 90% of the changes are tap-related, you could have considered keeping the tap upgrade out of this change and do it standalone instead

},
"dependencies": {
"chalk": "^4.1.0",
Expand Down
2 changes: 1 addition & 1 deletion test/basic-auth.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const child = childProcess.spawn(process.execPath, [path.join(__dirname, '..'),
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down
2 changes: 1 addition & 1 deletion test/cli-ipc.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ if (!win) {
}

t.autoend(false)
t.tearDown(function () {
t.teardown(function () {
child.kill()
})

Expand Down
12 changes: 6 additions & 6 deletions test/cli.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ test('should run benchmark against server', (t) => {
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down Expand Up @@ -97,7 +97,7 @@ test('should parse HAR file and run requests', (t) => {
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand All @@ -121,7 +121,7 @@ test('should throw on unknown HAR file', (t) => {
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down Expand Up @@ -150,7 +150,7 @@ test('should throw on invalid HAR file', (t) => {
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down Expand Up @@ -182,7 +182,7 @@ test('should write warning about unused HAR requests', (t) => {
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down Expand Up @@ -234,7 +234,7 @@ test('run with workers', { skip: !hasWorkerSupport }, (t) => {
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down
2 changes: 1 addition & 1 deletion test/envPort.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const child = childProcess.spawn(process.execPath, [path.join(__dirname, '..'),
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down
38 changes: 19 additions & 19 deletions test/httpClient.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ test('client calls a tls server without SNI servername twice', (t) => {

client.on('headers', (response) => {
t.equal(response.statusCode, 200, 'status code matches')
t.deepEqual(response.headers, ['X-servername', '', 'Content-Length', '0'])
t.same(response.headers, ['X-servername', '', 'Content-Length', '0'])
if (count++ > 0) {
client.destroy()
}
Expand All @@ -116,7 +116,7 @@ test('client calls a tls server with SNI servername twice', (t) => {

client.on('headers', (response) => {
t.equal(response.statusCode, 200, 'status code matches')
t.deepEqual(response.headers, ['X-servername', opts.servername, 'Content-Length', '0'])
t.same(response.headers, ['X-servername', opts.servername, 'Content-Length', '0'])
if (count++ > 0) {
client.destroy()
}
Expand All @@ -134,7 +134,7 @@ test('client uses SNI servername from URL hostname by default', (t) => {

client.on('headers', (response) => {
t.equal(response.statusCode, 200, 'status code matches')
t.deepEqual(response.headers, ['X-servername', opts.hostname, 'Content-Length', '0'])
t.same(response.headers, ['X-servername', opts.hostname, 'Content-Length', '0'])
if (count++ > 0) {
client.destroy()
}
Expand All @@ -153,7 +153,7 @@ test('client prefers SNI servername from opts over URL hostname', (t) => {

client.on('headers', (response) => {
t.equal(response.statusCode, 200, 'status code matches')
t.deepEqual(response.headers, ['X-servername', opts.servername, 'Content-Length', '0'])
t.same(response.headers, ['X-servername', opts.servername, 'Content-Length', '0'])
if (count++ > 0) {
client.destroy()
}
Expand All @@ -171,7 +171,7 @@ test('client ignores IP address in hostname-derived SNI servername', (t) => {

client.on('headers', (response) => {
t.equal(response.statusCode, 200, 'status code matches')
t.deepEqual(response.headers, ['X-servername', '', 'Content-Length', '0'])
t.same(response.headers, ['X-servername', '', 'Content-Length', '0'])
if (count++ > 0) {
client.destroy()
}
Expand All @@ -189,7 +189,7 @@ test('client ignores falsy SNI servername', (t) => {

client.on('headers', (response) => {
t.equal(response.statusCode, 200, 'status code matches')
t.deepEqual(response.headers, ['X-servername', '', 'Content-Length', '0'])
t.same(response.headers, ['X-servername', '', 'Content-Length', '0'])
if (count++ > 0) {
client.destroy()
}
Expand All @@ -211,7 +211,7 @@ test('client passes through tlsOptions to connect', (t) => {

client.on('headers', (response) => {
t.equal(response.statusCode, 200, 'status code matches')
t.deepEqual(response.headers, ['X-servername', '', 'X-email', 'tes@test.com', 'Content-Length', '0'])
t.same(response.headers, ['X-servername', '', 'X-email', 'tes@test.com', 'Content-Length', '0'])
if (count++ > 0) {
client.destroy()
}
Expand Down Expand Up @@ -362,7 +362,7 @@ test('client supports sending a body', (t) => {
server.once('request', (req, res) => {
req.pipe(bl((err, body) => {
t.error(err)
t.deepEqual(body.toString(), opts.body.toString(), 'body matches')
t.same(body.toString(), opts.body.toString(), 'body matches')
}))
})

Expand All @@ -385,7 +385,7 @@ test('client supports sending a body which is a string', (t) => {
server.once('request', (req, res) => {
req.pipe(bl((err, body) => {
t.error(err)
t.deepEqual(body.toString(), opts.body, 'body matches')
t.same(body.toString(), opts.body, 'body matches')
}))
})

Expand Down Expand Up @@ -718,21 +718,21 @@ test('client exposes response bodies and statuses', (t) => {
switch (number) {
case 1:
t.same(client.getRequestBuffer().toString(), makeResponseFromBody({ server, ...opts.requests[0] }), 'first request')
t.deepEqual(responses, [{
t.same(responses, [{
status: 200,
body: 'hello!'
}])
break
case 2:
t.same(client.getRequestBuffer().toString(), makeResponseFromBody({ server, ...opts.requests[1] }), 'second request')
t.deepEqual(responses, [{
t.same(responses, [{
status: 200,
body: 'hello!'
}])
break
case 3:
t.same(client.getRequestBuffer().toString(), makeResponseFromBody({ server, ...opts.requests[2] }), 'third request')
t.deepEqual(responses, [{
t.same(responses, [{
status: 200,
body: 'hello!'
}, {
Expand All @@ -742,7 +742,7 @@ test('client exposes response bodies and statuses', (t) => {
break
case 4:
t.same(client.getRequestBuffer().toString(), makeResponseFromBody({ server, ...opts.requests[0] }), 'first request')
t.deepEqual(responses, [{
t.same(responses, [{
status: 200,
body: 'hello!'
}, {
Expand Down Expand Up @@ -771,7 +771,7 @@ test('client keeps context and reset it when looping on requests', (t) => {
body: 'hello world again',
onResponse: (status, body, context) => {
if (number < 3) {
t.deepEqual(context, {}, 'context was supposed to be null')
t.same(context, {}, 'context was supposed to be null')
context.previousRes = body
}
}
Expand All @@ -780,7 +780,7 @@ test('client keeps context and reset it when looping on requests', (t) => {
method: 'PUT',
setupRequest: (req, context) => {
if (number < 3) {
t.deepEqual(context, { previousRes: expectedResponse }, 'context was supposed to contain previous response')
t.same(context, { previousRes: expectedResponse }, 'context was supposed to contain previous response')
}
return Object.assign({}, req, { body: context.previousRes })
}
Expand Down Expand Up @@ -913,22 +913,22 @@ test('client invokes appropriate onResponse when using pipelining', (t) => {
case 1:
// 1st & 2nd were sent, receiving 1st
t.same(client.getRequestBuffer().toString(), makeResponseFromBody({ server, method: 'GET' }), 'current should be second request')
t.deepEqual(responses, ['POST'])
t.same(responses, ['POST'])
break
case 2:
// 3rd was sent as 1st is finished, receiving 2nd
t.same(client.getRequestBuffer().toString(), makeResponseFromBody({ server, method: 'PUT' }), 'current should be third request')
t.deepEqual(responses, ['POST', 'GET'])
t.same(responses, ['POST', 'GET'])
break
case 3:
// 1st was resent, receiving 3rd
t.same(client.getRequestBuffer().toString(), makeResponseFromBody({ server, method: 'POST' }), 'current should be first request')
t.deepEqual(responses, ['POST', 'GET', 'PUT'])
t.same(responses, ['POST', 'GET', 'PUT'])
break
case 4:
// 2nd was resent, receiving 1st
t.same(client.getRequestBuffer().toString(), makeResponseFromBody({ server, method: 'GET' }), 'current should be second request')
t.deepEqual(responses, ['POST', 'GET', 'PUT', 'POST'])
t.same(responses, ['POST', 'GET', 'PUT', 'POST'])
client.destroy()
t.end()
break
Expand Down
2 changes: 1 addition & 1 deletion test/httpRequestBuilder.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,6 @@ test('should throw error if body is not a string or a buffer', (t) => {
try {
build({ body: [] })
} catch (error) {
t.is(error.message, 'body must be either a string or a buffer')
t.equal(error.message, 'body must be either a string or a buffer')
}
})
2 changes: 1 addition & 1 deletion test/onPort.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ test('--on-port flag', { skip: !hasAsyncHooks() }, (t) => {
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down
4 changes: 2 additions & 2 deletions test/pipelinedRequestsQueue.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ test('PipelinedRequestsQueue methods set values to the request in first-in-last-
const req1 = queue.terminateRequest()
t.equal(req1.req, 1)
t.equal(req1.body, '1')
t.deepEqual(req1.headers, { val: '1' })
t.same(req1.headers, { val: '1' })

queue.addBody('2')
queue.addByteCount(2)
Expand All @@ -77,5 +77,5 @@ test('PipelinedRequestsQueue methods set values to the request in first-in-last-
const req2 = queue.terminateRequest()
t.equal(req2.req, 2)
t.equal(req2.body, '2')
t.deepEqual(req2.headers, { val: '2' })
t.same(req2.headers, { val: '2' })
})
2 changes: 1 addition & 1 deletion test/printResult-renderStatusCodes.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ test('should stdout (print) the result', (t) => {
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down
2 changes: 1 addition & 1 deletion test/printResult.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ test('should stdout (print) the result', (t) => {
detached: false
})

t.tearDown(() => {
t.teardown(() => {
child.kill()
})

Expand Down
2 changes: 1 addition & 1 deletion test/progressTracker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ test(`should not emit warnings when using >= ${defaultMaxListeners} workers`, {

setTimeout(() => {
instance.stop()
t.false(emitWarningSpy.called)
t.notOk(emitWarningSpy.called)
emitWarningSpy.restore()
t.end()
}, 2000)
Expand Down
Loading