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: make test-tls-alert-handling more strict #14650

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 6, 2017

Use common.mustCall() and common.mustNotCall() to more rigorously
check that functions (especially no-op error handlers) are called the
expected number of times in test-tls-alert-handling.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test tls

@Trott Trott added test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. labels Aug 6, 2017
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 6, 2017
@Trott
Copy link
Member Author

Trott commented Aug 10, 2017

@Trott
Copy link
Member Author

Trott commented Aug 10, 2017

Boo! Failed on AIX, CentOS 5, and some (but not all) Windows variants. I think I can fix it, though...

@Trott
Copy link
Member Author

Trott commented Aug 10, 2017

Hrmmm....actually...is this a possible bug in our tls module or is it expected that we might get something like Error: 140736617649088:error:1408F10B:SSL routines:SSL3_GET_RECORD:wrong version number:../deps/openssl/openssl/ssl/s3_pkt.c:365: on one operating system while another operating system does not emit an error event at all?

📟 @nodejs/crypto

Uh...never mind!

@Trott
Copy link
Member Author

Trott commented Aug 10, 2017

Argh! Never mind! Test is flaky under load but only because we're now checking that error events are being emitted etc. I think I just need to keep the event loop open until everything is emitted.

Use `common.mustCall()` and `common.mustNotCall()` to more rigorously
check that functions (especially no-op error handlers) are called the
expected number of times in test-tls-alert-handling.
@Trott
Copy link
Member Author

Trott commented Aug 10, 2017

OK, the fix is in! Please Take Another Look!

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

@Trott
Copy link
Member Author

Trott commented Aug 11, 2017

Landed in ab2b331

@Trott Trott closed this Aug 11, 2017
Trott added a commit to Trott/io.js that referenced this pull request Aug 11, 2017
Use `common.mustCall()` and `common.mustNotCall()` to more rigorously
check that functions (especially no-op error handlers) are called the
expected number of times in test-tls-alert-handling.

PR-URL: nodejs#14650
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 9, 2017
Use `common.mustCall()` and `common.mustNotCall()` to more rigorously
check that functions (especially no-op error handlers) are called the
expected number of times in test-tls-alert-handling.

PR-URL: #14650
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

@Trott Trott deleted the tls-refactor branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants