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

test: https server close event #5106

Closed
wants to merge 1 commit into from
Closed

Conversation

braydonf
Copy link

@braydonf braydonf commented Feb 5, 2016

Closes: #5083

@mscdex mscdex added https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests. labels Feb 5, 2016
@mscdex
Copy link
Contributor

mscdex commented Feb 5, 2016

The commit should target the test: subsystem rather than net:.

@braydonf braydonf changed the title net: add test for https server close event test: add test for https server close event Feb 5, 2016

function shutdown() {
server.close(function() {
serverCloseEventCalled = true;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Instead of using the serverCloseEventCalled boolean, use common.mustCall() on the close callback.

console.log('1..0 # Skipped: missing crypto');
return;
}
var https = require('https');
Copy link
Member

Choose a reason for hiding this comment

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

const here please

@jasnell
Copy link
Member

jasnell commented Feb 7, 2016

LGTM if CI is green. Few nits tho.

@jasnell
Copy link
Member

jasnell commented Feb 7, 2016

@Trott
Copy link
Member

Trott commented Feb 7, 2016

@jasnell This is a test that needs a bugfix. It is intended to fail. It should not land until there is a bug fix (assuming this is a bug and not expected behavior being misconstrued as a bug, but it looks like a genuine bug to me).

Technically, I guess this shouldn't be a pull request and should instead be put in #5083. But the folks reporting this have done such a great job tracking down everything relevant to their bug report that I'm not inclined to complain.

Anyway, with that context, I guess we can look at the all-red CI as a win. (Well, except the lint warning which says that assert is defined but not used.)

@braydonf
Copy link
Author

braydonf commented Feb 8, 2016

For a bug fix, restoring the server property on socket fixes this particular test. This can be done by removing lines (381 and 382) in TLSSocket.prototype._init of _tls_wrap.js:

 if (socket && socket.server === this.server)
    socket.server = null;

However these two tests will not pass:

  • test/parallel/test-tls-dhe.js
  • test/parallel/test-tls-ticket-cluster.js

@braydonf
Copy link
Author

braydonf commented Feb 8, 2016

Also, yep the goal with opening as a PR is to have CI run and having the failing tests (so we can fix). We can put a WIP on this if that's necessary.

@braydonf braydonf changed the title test: add test for https server close event https: fix https server close event Feb 9, 2016
@braydonf
Copy link
Author

braydonf commented Feb 9, 2016

Bug fix found, all tests passing locally.

@Trott
Copy link
Member

Trott commented Feb 9, 2016

@Trott
Copy link
Member

Trott commented Feb 9, 2016

CI looks good. (Just one known flaky test that should be fixed when #5154 lands.)

LGTM but sign-off from someone who has spent some quality time with _tls_wrap.js would probably be good. Maybe one or more of @bnoordhuis @indutny @shigeki @mscdex ?

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. net Issues and PRs related to the net subsystem. and removed https Issues or PRs related to the https subsystem. labels Feb 9, 2016
@indutny
Copy link
Member

indutny commented Feb 16, 2016

Sorry, just found this after proposing #5262 . Please let me know if you still want to pursue this PR's approach.

@indutny
Copy link
Member

indutny commented Feb 17, 2016

@braydonf 7885b1d has been landed. Is this PR still relevant?

@braydonf
Copy link
Author

I ran the test included in this PR with the fix in #5262 and everything passed, perhaps we should include this test also.

@indutny
Copy link
Member

indutny commented Feb 17, 2016

@braydonf sounds like a good idea, may I ask you to strip all other changes from this PR then?

@braydonf
Copy link
Author

Okay rebased with only the test included.

Made a backup of the branch with other changes, for reference.

@braydonf braydonf changed the title https: fix https server close event test: https server close event Feb 17, 2016
@indutny
Copy link
Member

indutny commented Feb 17, 2016

@indutny
Copy link
Member

indutny commented Feb 17, 2016

CI is green except one unrelated failure.

@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

LGTM

@indutny
Copy link
Member

indutny commented Feb 19, 2016

Landing.

@indutny
Copy link
Member

indutny commented Feb 19, 2016

LGTM

@indutny
Copy link
Member

indutny commented Feb 19, 2016

Landed in 210e65a, thank you!

@indutny indutny closed this Feb 19, 2016
indutny pushed a commit that referenced this pull request Feb 19, 2016
PR-URL: #5106
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 21, 2016
PR-URL: #5106
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@MylesBorins
Copy link
Contributor

This test is failing in v4.x-staging 😭
@nodejs/testing thoughts?

@Trott
Copy link
Member

Trott commented Mar 1, 2016

@thealphanerd I think this test depends on 7885b1d so if that didn't land on v4.x-staging, that might explain it.

@MylesBorins
Copy link
Contributor

7885b1d just landed and this is now landing cleanly. Thank you as always for your swift and accurate response

MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
PR-URL: #5106
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
PR-URL: #5106
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
PR-URL: #5106
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants