GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: skip test that use --tls-v1.x flags by danbev · Pull Request #24376 · nodejs/node

Conversation

@danbev

@danbev danbev commented Nov 15, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@danbev
test: skip test that use --tls-v1.x flags
4b34878 
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 commented Nov 15, 2018

Copy link
Copy Markdown
Collaborator

@danbev build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1620/pipeline
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Nov 15, 2018
@danbev

danbev commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/18639/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack
refack approved these changes Nov 15, 2018
View reviewed changes
@refack

refack commented Nov 15, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@danbev

danbev commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

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
refack reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis approved these changes Nov 15, 2018
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/testpy/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
danbev added 2 commits November 15, 2018 12:05
@danbev
squash: check more than the first flag option
7998059
@danbev
tools: add node_has_crypto Context class
ed7c520 
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

danbev commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

Updated CI: https://ci.nodejs.org/job/node-test-pull-request/18643/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack
refack reviewed Nov 15, 2018
View reviewed changes
Comment thread test/testpy/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
refack
refack approved these changes Nov 15, 2018
View reviewed changes
@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

refack commented Nov 15, 2018

Copy link
Copy Markdown
Contributor

/CC @nodejs/python
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
danbev added 3 commits November 15, 2018 15:07
@danbev
squash: update message when no inspector support
5775587
@danbev
squash: update skip messages
971189c
@danbev
squash: fix typo in inspector message
1cf1462
refack
refack approved these changes Nov 15, 2018
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

💯

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@refack

refack commented Nov 15, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

CI is ready:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
thefourtheye
thefourtheye approved these changes Nov 15, 2018
View reviewed changes
Comment thread test/testpy/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@refack refack mentioned this pull request Nov 15, 2018
Create a job for building --without-ssl nodejs/build#1574
Closed
bnoordhuis
bnoordhuis approved these changes Nov 15, 2018
View reviewed changes
Comment thread test/testpy/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread tools/test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
fhinkel
fhinkel approved these changes Nov 16, 2018
View reviewed changes
rvagg
rvagg approved these changes Nov 18, 2018
View reviewed changes

@rvagg rvagg left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@danbev
squash: fix indentation and use single quotes
57d17c3
@danbev

danbev commented Nov 18, 2018

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/18722/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@danbev

danbev commented Nov 18, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 23e6928.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@danbev
test: skip test that use --tls-v1.x flags
23e6928 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
targos pushed a commit that referenced this pull request Nov 19, 2018
@danbev @targos
test: skip test that use --tls-v1.x flags
8ecba56 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@danbev @rvagg
test: skip test that use --tls-v1.x flags
de378c0 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
v11.4.0 proposal #24854
Merged
4 tasks done
This was referenced Dec 7, 2018
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#261
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#262
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#263
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#264
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#265
Closed
codebytere pushed a commit that referenced this pull request Jan 13, 2019
@danbev @codebytere
test: skip test that use --tls-v1.x flags
5b45642 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@danbev @refack
test: skip test that use --tls-v1.x flags
aa2c63e 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@codebytere codebytere mentioned this pull request Jan 15, 2019
v10.15.1 proposal #25346
Merged
codebytere pushed a commit that referenced this pull request Jan 29, 2019
@danbev @codebytere
test: skip test that use --tls-v1.x flags
d2d6287 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
This was referenced Jan 29, 2019
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#280
Closed
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#258
Closed
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#281
Closed
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#259
Closed
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#282
Closed
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#260
Closed
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#283
Closed
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#261
Closed
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#284
Closed
2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#262
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes
+4 more reviewers
@thefourtheye thefourtheye thefourtheye approved these changes
@refack refack refack approved these changes
@fhinkel fhinkel fhinkel approved these changes
@rvagg rvagg rvagg approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

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 Node.js core tests and test infrastructure. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@danbev @nodejs-github-bot @refack @fhinkel @bnoordhuis @rvagg @thefourtheye
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.