fix(test): make upgrade test pass

[DonutEspresso]

Pre-Submission Checklist

• Opened an issue discussing these changes before opening the PR
• Ran the linter and tests via make prepush
• Included comprehensive and convincing tests for changes

Issues

Two issues.

1. Upgrade tests are failing locally (and likely on CI), but we didn't know about it. Travis isn't running all of the nodeunit tests for some reason (??). We need to investigate.

2. Something has changed in recent Node 10 release, perhaps a lifecycle ordering change or something else like we saw previously. The upgrade test is currently set up in a way that requires proper response lifecycle cleanup to exit properly. This doesn't seem to be happening, as calling res.resume() on an upgraded request isn't triggering the correct behavior to allow the server to "close/resume" the response from restify, which leaves the client hanging.

Changes

I updated the test to exit the test earlier and not depend on proper response cleanup, as I suspect in the field we wouldn't really want to be encouraging the kind of scenarios exhibited by this test anyway. This kind of usage should be fatal.

That said, it's probably worth digging further into what's causing this breakage between Node8 => Node10. There may be other issues we uncover along the way.

[DonutEspresso]

@hekike 😂

[DonutEspresso]

This is why Travis was skipping the rest of the tests. Not sure why this wouldn't cause an error code though.

[hekike]

ooups, VW

[DonutEspresso]

Looks like this commit in 10.10.0 is likely related:
https://github.com/nodejs/node/blob/v10.10.0/lib/_stream_readable.js#L864

It seems calling resume won't do anything unless I have a data listener attached to the stream. I haven't dug too deeply yet into why this change was made. I'll make the requisite changes in this repo here to get us back into a good state.

[DonutEspresso]

This was removed because the claimUpgrade() call is what throws, so this line is never executed.

[DonutEspresso]

1. Upgrade tests are failing locally (and likely on CI), but we didn't know about it. Travis isn't running all of the nodeunit tests for some reason (??). We need to investigate.

This is because of a typo console.warning in a nodeunit test that caused the test runner to bail with an exit code of 0. As nodeunit is deprecated, I'm not sure if it's worth the time to investigate this more deeply. We should prioritize migrating those tests onto a new test runner.

2. Something has changed in recent Node 10 release, perhaps a lifecycle ordering change or something else like we saw previously. The upgrade test is currently set up in a way that requires proper response lifecycle cleanup to exit properly. This doesn't seem to be happening, as calling res.resume() on an upgraded request isn't triggering the correct behavior to allow the server to "close/resume" the response from restify, which leaves the client hanging.

The test did not have a listener for the data event. Without it, it appears that calling resume() was not doing anything. Worth digging more to see if it's a problem with the way the test is setup, vs an unintentional change in a non breaking release of Node (10.10.0).