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: skip test that use --tls-v1.x flags #24376

Closed
wants to merge 7 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 15, 2018

Currently, configuring --without-ssl will cause the following test to
fail:

=== release test-https-agent-additional-options ===
Path: parallel/test-https-agent-additional-options
out/Release/node: bad option: --tls-v1.1
Command: out/Release/node --tls-v1.1
  /node/test/parallel/test-https-agent-additional-options.js
=== release test-https-agent-session-eviction ===
Path: parallel/test-https-agent-session-eviction
out/Release/node: bad option: --tls-v1.0

Command: out/Release/node --tls-v1.0
  /node/test/parallel/test-https-agent-session-eviction.js

This commit adds a check for the --tls-v.x flags and skips them if node
was built without crypto support.

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

Currently, configuring --without-ssl will cause the following test to
fail:

=== release test-https-agent-additional-options ===
Path: parallel/test-https-agent-additional-options
out/Release/node: bad option: --tls-v1.1
Command: out/Release/node --tls-v1.1
  /node/test/parallel/test-https-agent-additional-options.js

=== release test-https-agent-session-eviction ===
Path: parallel/test-https-agent-session-eviction
out/Release/node: bad option: --tls-v1.0

Command: out/Release/node --tls-v1.0
  /node/test/parallel/test-https-agent-session-eviction.js

This commit adds a check for the --tls-v.x flags and skips them if node
was built without crypto support.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 15, 2018
@danbev
Copy link
Contributor Author

danbev commented Nov 15, 2018

@refack
Copy link
Contributor

refack commented Nov 15, 2018

@danbev could you open an issues is nodejs/build for us to add a jub that builds --withou-ssl, please?

@danbev
Copy link
Contributor Author

danbev commented Nov 15, 2018

could you open an issues is nodejs/build for us to add a jub that builds --withou-ssl, please?

Absolutely, that would be great to have. I've created nodejs/build#1574 for this. Thanks

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Change itself LGTM but see comment.

test/testpy/__init__.py Outdated Show resolved Hide resolved
Currently, there is a check for crypto related flags used in tests,
for example // Flags: --use-bundled-ca at the top of a test.
The current implementation only checks if if the inspector is enabled or
not and if not skips tests that use certain crypto flags. The has worked
because when disabling the inspector also disables crypto (like
configuring --without-ssl). But when configured --without-intl the
inspector is disabled but not crypto causing failures. From
configure.py:
"Disable Intl, same as --with-intl=none (disables inspector)"

The commit adds a node_has_crypto property to the Context class which is
checked for crypto specific flags.
@danbev
Copy link
Contributor Author

danbev commented Nov 15, 2018

test/testpy/__init__.py Outdated Show resolved Hide resolved
@refack refack added crypto Issues and PRs related to the crypto subsystem. tools Issues and PRs related to the tools directory. inspector Issues and PRs related to the V8 inspector protocol labels Nov 15, 2018
@refack
Copy link
Contributor

refack commented Nov 15, 2018

/CC @nodejs/python

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯

@refack
Copy link
Contributor

refack commented Nov 15, 2018

CI is ready:

  • master ❌ (2 test that fail are because of --tls-v1.x)
  • this PR

test/testpy/__init__.py Show resolved Hide resolved
test/testpy/__init__.py Show resolved Hide resolved
tools/test.py Outdated Show resolved Hide resolved
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

approved pending Ben's style nit

@danbev
Copy link
Contributor Author

danbev commented Nov 18, 2018

@danbev
Copy link
Contributor Author

danbev commented Nov 18, 2018

Landed in 23e6928.

@danbev danbev closed this Nov 18, 2018
@danbev danbev deleted the test_skip_tls_flags_withoutssl branch November 18, 2018 13:42
danbev added a commit that referenced this pull request Nov 18, 2018
Currently, configuring --without-ssl will cause the following test to
fail:

=== release test-https-agent-additional-options ===
Path: parallel/test-https-agent-additional-options
out/Release/node: bad option: --tls-v1.1
Command: out/Release/node --tls-v1.1
  /node/test/parallel/test-https-agent-additional-options.js

=== release test-https-agent-session-eviction ===
Path: parallel/test-https-agent-session-eviction
out/Release/node: bad option: --tls-v1.0

Command: out/Release/node --tls-v1.0
  /node/test/parallel/test-https-agent-session-eviction.js

This commit adds a check for the --tls-v.x flags and skips them if node
was built without crypto support.

PR-URL: #24376
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
targos pushed a commit that referenced this pull request Nov 19, 2018
Currently, configuring --without-ssl will cause the following test to
fail:

=== release test-https-agent-additional-options ===
Path: parallel/test-https-agent-additional-options
out/Release/node: bad option: --tls-v1.1
Command: out/Release/node --tls-v1.1
  /node/test/parallel/test-https-agent-additional-options.js

=== release test-https-agent-session-eviction ===
Path: parallel/test-https-agent-session-eviction
out/Release/node: bad option: --tls-v1.0

Command: out/Release/node --tls-v1.0
  /node/test/parallel/test-https-agent-session-eviction.js

This commit adds a check for the --tls-v.x flags and skips them if node
was built without crypto support.

PR-URL: #24376
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Currently, configuring --without-ssl will cause the following test to
fail:

=== release test-https-agent-additional-options ===
Path: parallel/test-https-agent-additional-options
out/Release/node: bad option: --tls-v1.1
Command: out/Release/node --tls-v1.1
  /node/test/parallel/test-https-agent-additional-options.js

=== release test-https-agent-session-eviction ===
Path: parallel/test-https-agent-session-eviction
out/Release/node: bad option: --tls-v1.0

Command: out/Release/node --tls-v1.0
  /node/test/parallel/test-https-agent-session-eviction.js

This commit adds a check for the --tls-v.x flags and skips them if node
was built without crypto support.

PR-URL: #24376
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
codebytere pushed a commit that referenced this pull request Jan 13, 2019
Currently, configuring --without-ssl will cause the following test to
fail:

=== release test-https-agent-additional-options ===
Path: parallel/test-https-agent-additional-options
out/Release/node: bad option: --tls-v1.1
Command: out/Release/node --tls-v1.1
  /node/test/parallel/test-https-agent-additional-options.js

=== release test-https-agent-session-eviction ===
Path: parallel/test-https-agent-session-eviction
out/Release/node: bad option: --tls-v1.0

Command: out/Release/node --tls-v1.0
  /node/test/parallel/test-https-agent-session-eviction.js

This commit adds a check for the --tls-v.x flags and skips them if node
was built without crypto support.

PR-URL: #24376
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Currently, configuring --without-ssl will cause the following test to
fail:

=== release test-https-agent-additional-options ===
Path: parallel/test-https-agent-additional-options
out/Release/node: bad option: --tls-v1.1
Command: out/Release/node --tls-v1.1
  /node/test/parallel/test-https-agent-additional-options.js

=== release test-https-agent-session-eviction ===
Path: parallel/test-https-agent-session-eviction
out/Release/node: bad option: --tls-v1.0

Command: out/Release/node --tls-v1.0
  /node/test/parallel/test-https-agent-session-eviction.js

This commit adds a check for the --tls-v.x flags and skips them if node
was built without crypto support.

PR-URL: nodejs#24376
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@codebytere codebytere mentioned this pull request Jan 15, 2019
codebytere pushed a commit that referenced this pull request Jan 29, 2019
Currently, configuring --without-ssl will cause the following test to
fail:

=== release test-https-agent-additional-options ===
Path: parallel/test-https-agent-additional-options
out/Release/node: bad option: --tls-v1.1
Command: out/Release/node --tls-v1.1
  /node/test/parallel/test-https-agent-additional-options.js

=== release test-https-agent-session-eviction ===
Path: parallel/test-https-agent-session-eviction
out/Release/node: bad option: --tls-v1.0

Command: out/Release/node --tls-v1.0
  /node/test/parallel/test-https-agent-session-eviction.js

This commit adds a check for the --tls-v.x flags and skips them if node
was built without crypto support.

PR-URL: #24376
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants