Skip to content

bump minimum shot dependency version#4150

Closed
cjihrig wants to merge 1 commit intomasterfrom
cjihrig-patch-1
Closed

bump minimum shot dependency version#4150
cjihrig wants to merge 1 commit intomasterfrom
cjihrig-patch-1

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Aug 25, 2020

No description provided.

@devinivy
Copy link
Member

I see there's a failure. Darn— on hapijs/shot#129 I almost specifically requested that someone try linking it to hapi and giving it a test run. It might be the case that it will work once #4095 lands.

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 25, 2020

So it looks like this fixes the test:

diff --git a/test/payload.js b/test/payload.js
index b776a44f..0379bdfa 100755
--- a/test/payload.js
+++ b/test/payload.js
@@ -71,10 +71,10 @@ describe('Payload', () => {
 
         const responded = server.ext('onPostResponse');
 
-        server.inject({ method: 'POST', url: '/', payload: 'test', simulate: { close: true, end: false } });
+        server.inject({ method: 'POST', url: '/', payload: 'test', simulate: { close: true } });
         const request = await responded;
         expect(request._isReplied).to.equal(true);
-        expect(request.response.output.statusCode).to.equal(500);
+        expect(request.response.output.statusCode).to.equal(400);
     });
 
     it('handles aborted request', { retry: true }, async () => {

The issue is that after hapijs/shot#129, we no longer force a 'close' event to be emitted, and instead rely on Node to do it for us (which is why we need 'end' to be emitted).

The question is whether we want to update the test and move on with life, or fix/revert the behavior change in shot. The only issue with just updating the test here is that the shot release was a patch release and this is a subtle behavioral change.

cc: @kanongil

@cjihrig cjihrig closed this Aug 25, 2020
@cjihrig cjihrig deleted the cjihrig-patch-1 branch August 25, 2020 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants