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

Update deps to new hapi scope #14

Merged
merged 3 commits into from
Nov 25, 2019
Merged

Update deps to new hapi scope #14

merged 3 commits into from
Nov 25, 2019

Conversation

zemccartney
Copy link
Member

@zemccartney zemccartney commented May 22, 2019

Addresses #13

@devinivy I added the odd new lines in the testing file to deal with that test failing on node 10 (v.10.15.3) and 12 (v.12.2.0).

The test would complete, all assertions would pass, but would then error out in the onCleanup callback with an ECONNRESET error. As far as I can tell, this happens because the http2 client's response stream hasn't closed, so calling client.destroy() force-ends that stream, triggering an ECONNRESET. Note that calling client.close(), the "graceful/clean" version of that call, closes the stream without error (though takes some extra time to complete).

The current approach ensures the response closes by consuming it via the data handler, which seems to enable the stream to end (without the data handler, the Toys.event(request, 'end') never fulfills, causing the test to timeout. I think? God, I dunno, very quickly got out of my depth.

My bad if I just made a mess of things here :trollface:

test/index.js Outdated Show resolved Hide resolved
@devinivy
Copy link
Member

Sounds reasonable to me! Thanks.

@coveralls
Copy link

coveralls commented May 22, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling da21aa6 on zemccartney:scoping-update into 9afce79 on hapipal:master.

@devinivy devinivy self-assigned this Nov 25, 2019
@devinivy devinivy added this to the 3.0.1 milestone Nov 25, 2019
@devinivy devinivy merged commit 0e71ca0 into hapipal:master Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants