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.

Sign up for GitHub

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

Jump to bottom

test: allow disabling crypto tests #31268

Closed
wants to merge 1 commit into from
Closed

test: allow disabling crypto tests #31268

wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jan 9, 2020 •
edited
Loading

This PR adds a new environment variable to the test suit: NODE_SKIP_CRYPTO. Right now whether or not crypto tests can be run is solely determined by whether or not openssl is defined on the process object, which is true for Electron.js since many consumers use it to determine the presence of crypto even though we use BoringSSL and thus experience incompatibilities.

This new option would allow us to more effectively run our own smoke tests of Node.js' test suite without needing to manually disable every crypto test individually.

cc @tniessen

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

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jan 9, 2020
@codebytere codebytere requested a review from tniessen January 9, 2020 04:58
@Trott
Copy link
Member

Trott commented Jan 9, 2020

With this change, using --no-crypto doesn't cause any tests to be skipped as far as I can tell. Am I doing something wrong? I'm trying things like tools/test.py --no-crypto -p tap async-hooks and seeing if the crypto tests in the async-hooks directory are skipped.

@codebytere codebytere force-pushed the allow-disabling-crypto-tests branch from 325cfa8 to 8835e5f Compare January 9, 2020 16:28
@codebytere
Copy link
Member Author

@Trott It turns out that individual tests are validated in js with the same mechanism, so i've moved this logic into common and added documentation for a new simple env var, NODE_SKIP_CRYPTO. I'm happy to name it something else if you all would prefer :)

cc @addaleax since i've moved this mechanism, this probably needs a re-review

@codebytere codebytere force-pushed the allow-disabling-crypto-tests branch from 8835e5f to ee3d29b Compare January 9, 2020 16:30
@codebytere codebytere removed the tools Issues and PRs related to the tools directory. label Jan 9, 2020
@codebytere codebytere force-pushed the allow-disabling-crypto-tests branch from ee3d29b to c8cf0fd Compare January 9, 2020 17:45
addaleax
addaleax approved these changes Jan 9, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM :)

It’s a bit curious that the former approach didn’t work… Was that Python variable ignored altogether? Should we remove it?

@codebytere codebytere changed the title tools: allow disabling crypto tests test: allow disabling crypto tests Jan 9, 2020
@codebytere codebytere force-pushed the allow-disabling-crypto-tests branch from c8cf0fd to 55d796d Compare January 9, 2020 18:52
@BridgeAR
Copy link
Member

BridgeAR commented Jan 9, 2020

Marking as author ready without CI, since it should not have any effect.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 9, 2020
@codebytere
Copy link
Member Author

codebytere commented Jan 9, 2020 •
edited
Loading

@addaleax it turned out that it was used but that it's purpose wasn't clear at first glance; it's used here.

@richardlau
Copy link
Member

Marking as author ready without CI, since it should not have any effect.

I'd still suggest running CI to make sure, for example, ubuntu1804_sharedlibs_withoutssl_x64 still works.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 10, 2020 •
edited by Trott
Loading

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

mhdawson
mhdawson approved these changes Jan 10, 2020
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

Trott pushed a commit to Trott/io.js that referenced this pull request Jan 11, 2020
@codebytere @Trott
test: allow disabling crypto tests
be055d1 
PR-URL: nodejs#31268
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott
Copy link
Member

Trott commented Jan 11, 2020

Landed in be055d1

@Trott Trott closed this Jan 11, 2020
@codebytere codebytere deleted the allow-disabling-crypto-tests branch January 11, 2020 06:20
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
@codebytere @MylesBorins
test: allow disabling crypto tests
1df7961 
PR-URL: #31268
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@codebytere codebytere mentioned this pull request Jan 16, 2020
Merged
codebytere added a commit that referenced this pull request Mar 14, 2020
@codebytere
test: allow disabling crypto tests
40b5e0e 
PR-URL: #31268
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
codebytere added a commit that referenced this pull request Mar 17, 2020
@codebytere
test: allow disabling crypto tests
c719f7a 
PR-URL: #31268
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
@codebytere codebytere mentioned this pull request Apr 14, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@richardlau richardlau richardlau approved these changes

@devnexen devnexen devnexen approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.