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: fix test-tls-securepair-client #25222

Closed
wants to merge 2 commits into from

Conversation

@Trott
Copy link
Member

commented Dec 26, 2018

test-tls-securepair-client has been failing for over a year but no one
noticed because pummel tests are almost never run.

In preparation for running pummel tests once a day in CI, fix
test-tls-securepair-client.

test-tls-securepair-client does not seem to need to be in the pummel
directory. Move it to sequential. (It can't go into parallel because it
uses common.PORT and therefore might conflict with another test that is
using system-assigned available ports.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Trott referenced this pull request Dec 26, 2018
tls: make deprecated tls.createSecurePair() use public API
Make the deprecated `tls.createSecurePair()` method use other public
APIs only (`TLSSocket` in particular).

Since `tls.createSecurePair()` has been runtime-deprecated only
since Node 8, it probably isn’t quite time to remove it yet,
but this patch removes almost all of the code complexity that
is retained by it.

The API, as it is documented, is retained. However, it is very likely
that some users have come to rely on parts of undocumented API
of the `SecurePair` class, especially since some of the existing
tests checked for those. Therefore, this should definitely be
considered a breaking change.

PR-URL: #17882
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

@Trott Trott force-pushed the Trott:fix-securepair branch from 9b4a93e to 407521d Dec 26, 2018

@Trott

This comment has been minimized.

Copy link
Member Author

commented Dec 26, 2018

Works everywhere now but Windows...

test/sequential/test-tls-securepair-client.js Outdated Show resolved Hide resolved
@lundibundi lundibundi referenced this pull request Dec 27, 2018
4 of 4 tasks complete

@Trott Trott force-pushed the Trott:fix-securepair branch from 407521d to a994d5d Jan 4, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

Here's what the Windows failure looks like:

16:02:14 not ok 619 sequential/test-tls-securepair-client
16:02:14   ---
16:02:14   duration_ms: 5.680
16:02:14   severity: fail
16:02:14   exitcode: 1
16:02:14   stack: |-
16:02:14     Using default temp DH parameters
16:02:14     WAIT-ACCEPT
16:02:14     ACCEPT
16:02:14     WAIT-ACCEPT
16:02:14     client connected
16:02:14     (node:5684) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.TLSSocket instead.
16:02:14     assert.js:86
16:02:14       throw new AssertionError(obj);
16:02:14       ^
16:02:14     
16:02:14     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
16:02:14     
16:02:14     -1 !== 0
16:02:14     
16:02:14         at process.<anonymous> (c:\workspace\node-test-binary-windows\test\sequential\test-tls-securepair-client.js:174:12)
16:02:14         at process.emit (events.js:193:15)
16:02:14         at process.exit (internal/process/per_thread.js:150:15)
16:02:14         at Timeout._onTimeout (c:\workspace\node-test-binary-windows\test\sequential\test-tls-securepair-client.js:104:13)
16:02:14         at listOnTimeout (timers.js:324:15)
16:02:14         at processTimers (timers.js:268:5)
16:02:14   ...

@Trott Trott force-pushed the Trott:fix-securepair branch from a994d5d to a193928 Jan 7, 2019

Trott added 2 commits Dec 26, 2018
test: fix test-tls-securepair-client
test-tls-securepair-client has been failing for over a year but no one
noticed because pummel tests are almost never run.

In preparation for running pummel tests once a day in CI, fix
test-tls-securepair-client.
test: move test-tls-securepair-client out of pummel
test-tls-securepair-client does not seem to need to be in the pummel
directory. Move it to sequential. (It can't go into parallel because it
uses common.PORT and therefore might conflict with another test that is
using system-assigned available ports.)

@Trott Trott force-pushed the Trott:fix-securepair branch from a193928 to 2a879a3 Jan 7, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

Added a common.skip() for Windows. My reasoning is that this test is currently broken and not run in CI at all. This PR fixes it everywhere but Windows and moves it somewhere that it will run in CI. So even if it is skipped on Windows, that's still a significant improvement over the current state. When this lands, I'll open an issue for the test to be fixed on Windows and add a help wanted label to it.

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

@Trott

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

Landed in ec5884a...0066fdb

@Trott Trott closed this Jan 7, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jan 7, 2019
test: fix test-tls-securepair-client
test-tls-securepair-client has been failing for over a year but no one
noticed because pummel tests are almost never run.

In preparation for running pummel tests once a day in CI, fix
test-tls-securepair-client.

PR-URL: nodejs#25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Jan 7, 2019
test: move test-tls-securepair-client out of pummel
test-tls-securepair-client does not seem to need to be in the pummel
directory. Move it to sequential. (It can't go into parallel because it
uses common.PORT and therefore might conflict with another test that is
using system-assigned available ports.)

PR-URL: nodejs#25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Jan 9, 2019
test: fix test-tls-securepair-client
test-tls-securepair-client has been failing for over a year but no one
noticed because pummel tests are almost never run.

In preparation for running pummel tests once a day in CI, fix
test-tls-securepair-client.

PR-URL: #25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Jan 9, 2019
test: move test-tls-securepair-client out of pummel
test-tls-securepair-client does not seem to need to be in the pummel
directory. Move it to sequential. (It can't go into parallel because it
uses common.PORT and therefore might conflict with another test that is
using system-assigned available ports.)

PR-URL: #25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
test: fix test-tls-securepair-client
test-tls-securepair-client has been failing for over a year but no one
noticed because pummel tests are almost never run.

In preparation for running pummel tests once a day in CI, fix
test-tls-securepair-client.

PR-URL: nodejs#25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
test: move test-tls-securepair-client out of pummel
test-tls-securepair-client does not seem to need to be in the pummel
directory. Move it to sequential. (It can't go into parallel because it
uses common.PORT and therefore might conflict with another test that is
using system-assigned available ports.)

PR-URL: nodejs#25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
test: fix test-tls-securepair-client
test-tls-securepair-client has been failing for over a year but no one
noticed because pummel tests are almost never run.

In preparation for running pummel tests once a day in CI, fix
test-tls-securepair-client.

PR-URL: nodejs#25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
test: move test-tls-securepair-client out of pummel
test-tls-securepair-client does not seem to need to be in the pummel
directory. Move it to sequential. (It can't go into parallel because it
uses common.PORT and therefore might conflict with another test that is
using system-assigned available ports.)

PR-URL: nodejs#25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
BethGriggs added a commit that referenced this pull request Apr 28, 2019
test: fix test-tls-securepair-client
test-tls-securepair-client has been failing for over a year but no one
noticed because pummel tests are almost never run.

In preparation for running pummel tests once a day in CI, fix
test-tls-securepair-client.

PR-URL: #25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs added a commit that referenced this pull request Apr 28, 2019
test: move test-tls-securepair-client out of pummel
test-tls-securepair-client does not seem to need to be in the pummel
directory. Move it to sequential. (It can't go into parallel because it
uses common.PORT and therefore might conflict with another test that is
using system-assigned available ports.)

PR-URL: #25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs referenced this pull request May 1, 2019
BethGriggs added a commit that referenced this pull request May 10, 2019
test: fix test-tls-securepair-client
test-tls-securepair-client has been failing for over a year but no one
noticed because pummel tests are almost never run.

In preparation for running pummel tests once a day in CI, fix
test-tls-securepair-client.

PR-URL: #25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs added a commit that referenced this pull request May 10, 2019
test: move test-tls-securepair-client out of pummel
test-tls-securepair-client does not seem to need to be in the pummel
directory. Move it to sequential. (It can't go into parallel because it
uses common.PORT and therefore might conflict with another test that is
using system-assigned available ports.)

PR-URL: #25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request May 16, 2019
test: fix test-tls-securepair-client
test-tls-securepair-client has been failing for over a year but no one
noticed because pummel tests are almost never run.

In preparation for running pummel tests once a day in CI, fix
test-tls-securepair-client.

PR-URL: #25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request May 16, 2019
test: move test-tls-securepair-client out of pummel
test-tls-securepair-client does not seem to need to be in the pummel
directory. Move it to sequential. (It can't go into parallel because it
uses common.PORT and therefore might conflict with another test that is
using system-assigned available ports.)

PR-URL: #25222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.