test: http2 connectionListener reject client #16080

trivikr · 2017-10-08T02:10:41Z

This code change modifies connectionListener tests to cover a test case where this.emit('unknownProtocol', socket) returns false

node/lib/internal/http2/core.js

Lines 2284 to 2287 in e012229

    
           } else { 
        
             // Reject non-HTTP/2 client 
        
             return socket.destroy(); 
        
           }

Refs: #14985

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test, http2

apapirovski · 2017-10-09T20:40:56Z

test/parallel/test-http2-https-fallback.js


  server.listen(0);

+  // Test for checking undefined timeout (code: https://git.io/vdzE9)
+  server.setTimeout(undefined);


This feels out of place in this test. I would suggest removing it. If it needs to be tested, there must be a more appropriate existing test to insert it in (one that already deals with timeouts). (The link to git.io is also unnecessary.)

Removing the test or removing the check for this.timeout in connectionListener?
I think latter should be done as timeout is always expected to be a number (either default one or the one explicitly set).

Removing the test for setTimeout(undefined). It doesn't relate to http2 https fallback and is likely to just confuse people in the future.

Removed the test for setTimeout(undefined) and updated the commit

This code change modifies connectionListener tests to cover test case where this.emit('unknownProtocol', socket) returns false Refs: nodejs#14985

apapirovski

LGTM 👍 Thank you!

trivikr · 2017-10-13T17:34:04Z

Can someone create CI link for this request?

joyeecheung · 2017-10-14T14:00:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10715/

This code change modifies connectionListener tests to cover test case where this.emit('unknownProtocol', socket) returns false PR-URL: #16080 Ref: #14985 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-10-15T22:04:42Z

Landed in 3cc725d

This code change modifies connectionListener tests to cover test case where this.emit('unknownProtocol', socket) returns false PR-URL: #16080 Ref: #14985 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This code change modifies connectionListener tests to cover test case where this.emit('unknownProtocol', socket) returns false PR-URL: nodejs/node#16080 Ref: nodejs/node#14985 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 8, 2017

trivikr force-pushed the test-http2-https-fallback branch from 6e56361 to 54b77aa Compare October 8, 2017 03:09

mscdex added the http2 Issues or PRs related to the http2 subsystem. label Oct 8, 2017

apapirovski reviewed Oct 9, 2017

View reviewed changes

test: http2 connectionListener reject client

b56b64a

This code change modifies connectionListener tests to cover test case where this.emit('unknownProtocol', socket) returns false Refs: nodejs#14985

trivikr force-pushed the test-http2-https-fallback branch from 54b77aa to b56b64a Compare October 10, 2017 00:06

apapirovski approved these changes Oct 10, 2017

View reviewed changes

lpinca approved these changes Oct 10, 2017

View reviewed changes

jasnell approved these changes Oct 12, 2017

View reviewed changes

jasnell closed this Oct 15, 2017

trivikr deleted the test-http2-https-fallback branch October 15, 2017 22:34

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: http2 connectionListener reject client #16080

test: http2 connectionListener reject client #16080

trivikr commented Oct 8, 2017 •

edited by lpinca

Loading

apapirovski Oct 9, 2017

trivikr Oct 9, 2017

apapirovski Oct 9, 2017

trivikr Oct 10, 2017

apapirovski left a comment •

edited

Loading

trivikr commented Oct 13, 2017

joyeecheung commented Oct 14, 2017

jasnell commented Oct 15, 2017

	} else {
	// Reject non-HTTP/2 client
	return socket.destroy();
	}

test: http2 connectionListener reject client #16080

test: http2 connectionListener reject client #16080

Conversation

trivikr commented Oct 8, 2017 • edited by lpinca Loading

Checklist

Affected core subsystem(s)

apapirovski Oct 9, 2017

Choose a reason for hiding this comment

trivikr Oct 9, 2017

Choose a reason for hiding this comment

apapirovski Oct 9, 2017

Choose a reason for hiding this comment

trivikr Oct 10, 2017

Choose a reason for hiding this comment

apapirovski left a comment • edited Loading

Choose a reason for hiding this comment

trivikr commented Oct 13, 2017

joyeecheung commented Oct 14, 2017

jasnell commented Oct 15, 2017

trivikr commented Oct 8, 2017 •

edited by lpinca

Loading

apapirovski left a comment •

edited

Loading