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

Update some more tests relying on timeouts #341

Merged
merged 4 commits into from
Mar 2, 2021
Merged

Update some more tests relying on timeouts #341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0405f14
Remove unnecessary timeout check in httpClient delay
nherment Mar 1, 2021
0bab9ed
Update a couple of tests that don't do well in low CPU env.
nherment Mar 2, 2021
7fdffa9
add empty line for readability in a test
nherment Mar 2, 2021
782c9a7
Allow sync call of httpClient.destroy() in a timeout event
nherment Mar 2, 2021
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
8 changes: 4 additions & 4 deletions lib/httpClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,15 @@ function Client (opts) {
this.opts.setupClient(this)

const handleTimeout = () => {
// all pipelined requests have timed out here
this.resData.forEach(() => this.emit('timeout'))
this.cer = 0
this._destroyConnection()

// timeout has already occured, need to set a new timeoutTicker
this.timeoutTicker = retimer(handleTimeout, this.timeout)
this.timeoutTicker.reschedule(this.timeout)

this._connect()

// all pipelined requests have timed out here
this.resData.forEach(() => this.emit('timeout'))
}

if (this.rate) {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"pretty-bytes": "^5.4.1",
"progress": "^2.0.3",
"reinterval": "^1.1.0",
"retimer": "^2.0.0",
"retimer": "^3.0.0",
"semver": "^7.3.2",
"timestring": "^6.0.0"
}
Expand Down
15 changes: 8 additions & 7 deletions test/httpClient.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,16 @@ test('client calls a https server twice', (t) => {
test('client calculates correct duration when using pipelining', (t) => {
t.plan(4)

const lazyServer = helper.startServer({ delayResponse: 500 })
const delayResponse = 500
const lazyServer = helper.startServer({ delayResponse })
const opts = lazyServer.address()
opts.pipelining = 2
const client = new Client(opts)
let count = 0

client.on('response', (statusCode, length, duration) => {
t.equal(statusCode, 200, 'status code matches')
t.ok(duration > 500 && duration < 800)
t.ok(duration > delayResponse, `Expected response delay > ${delayResponse}ms but got ${duration}ms`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI the original reason for checking if duration < 800 was this.

our calculations were giving ~half results, so not checking if it takes less than 600ms might not catch if the calculations in future break and start giving 2x time. We can increase it to 700 if builds start to fail.

I know it wasn't the right fix and there should be a more predictable way to assert this but removing the upper limit check brings the risk I explained in the comment.

Copy link
Owner

Choose a reason for hiding this comment

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

@nherment could you handle this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I'll add a unit test for these calculations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@salmanm could you please point me to the 'calculations' you are referring to?

The code around the delayed response is pretty straightforward and I don't see anything that I would classify as calculations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Calcs were wrong for avg latency when using pipeline (See this). Which was fixed by correctly calculating current expected response (cer) here.


if (++count === 2) {
client.destroy()
Expand Down Expand Up @@ -580,9 +581,8 @@ test('client should emit a timeout when no response is received', (t) => {

client.on('timeout', () => {
t.ok(1, 'timeout should have happened')
client.destroy()
})

setTimeout(() => client.destroy(), 1800)
})

test('client should emit 2 timeouts when no responses are received', (t) => {
Expand All @@ -591,12 +591,13 @@ test('client should emit 2 timeouts when no responses are received', (t) => {
const opts = timeoutServer.address()
opts.timeout = 1
const client = new Client(opts)

let count = 0
client.on('timeout', () => {
t.ok(1, 'timeout should have happened')
if (count++ > 0) {
client.destroy()
}
})

setTimeout(() => client.destroy(), 2800)
})

test('client should have 2 different requests it iterates over', (t) => {
Expand Down