test: make tests pass when configured --without-ssl by danbev · Pull Request #11631 · nodejs/node

danbev · 2017-03-01T13:49:58Z

This pull request contains a number of commits to enable the tests to pass when having configured node using --without-ssl. For example:

$ ./configure --without-ssl && make -j8 test

Checklist

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

Affected core subsystem(s)

test

danbev · 2017-03-01T13:50:52Z

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

danbev · 2017-03-02T06:52:00Z

Rebased and new CI: https://ci.nodejs.org/job/node-test-pull-request/6651/

bnoordhuis · 2017-03-02T10:01:52Z

parallel/test-https-agent-create-connection is failing on the centos5-32 and win2008r2 buildbots with this PR but not others.

EDIT: Although, see #11644 - almost too much of a coincidence.

bnoordhuis · 2017-03-02T10:02:36Z

test/fixtures/tls-connect.js

Whitespace change.

bnoordhuis · 2017-03-02T10:07:02Z

test/testpy/__init__.py

"it will that will"

bnoordhuis · 2017-03-02T10:15:56Z

test/addons/openssl-binding/binding.gyp

I'm kind of surprised this works. gyp generally doesn't like it when there are no source files to compile.

bnoordhuis · 2017-03-02T10:24:28Z

test/fixtures/tls-connect.js

Can you fix up the tests that include this file? They call this function too but that's unnecessary with this change. You can probably stop exporting checkCrypto() altogether and just inline it here.

Ah right, missed that. I'll fix that.

bnoordhuis · 2017-03-02T10:25:54Z

test/testpy/__init__.py

Does this run once per run or once per test? The latter would be a little wasteful.

It is actually once per test which is wasteful as you say. I'll take a look at how this can be run only once.

richardlau · 2017-03-02T13:52:53Z

test/addons/openssl-binding/test.js

Existing uses of common.skip() in other tests return afterwards rather than calll process.exit().

I'll take another look at this. Just returning does not work in this case but I'll try and dig a little further.

In this case I don't think we can just return as that would cause the tests that require tls-connect should not be able to proceed if there is no crypto. There might be other/better ways to deal with this but perhaps that should be done as a separate PR in that case.

danbev · 2017-03-02T14:50:13Z

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

danbev · 2017-03-02T18:46:14Z

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

Currently when node is build --without-ssl and the test are run, the test that use the tls-connect fixture will throw an Error saying that Node was not built with ssl support. The error is thrown when the tls module is required causing the test that use it to fail unstead of being skipped when there is no ssl support to test. This commit calls checkCrypto before requiring the tls module so that an error is not thrown and the test skipped instead.

This commit enables the test-parallel target to pass when the build is configured --without-ssl To configure and run: $ ./configure --without-ssl --debug && make -j8 test-parallel The update to test/testspy/__init__.py is for test-cluster-inspector-debug-port.js which passes '--inspect={PORT} flag to the node process which causes the process to exit as the inspector is not enabled when ssl is disabled. The suggested solution here is to simply remove the flag when v8_enable_inspector is 0 and then have a check in the test like the others test in this commit.

Add checks to the compilation of this addon and also a check when the test is run to see if ssl is enabled or not. If it is not enabled then skip this test.

danbev · 2017-03-03T06:42:25Z

Rebased and triggered another CI: https://ci.nodejs.org/job/node-test-pull-request/6675/
Trying to figure out the reason for the failures.

danbev · 2017-03-03T06:44:24Z

@bnoordhuis Would you mind taking a look at the lastest commits which attempt to address your review?

bnoordhuis

LGTM if you update the remaining test files. Nice work.

bnoordhuis · 2017-03-03T07:18:58Z

test/fixtures/tls-connect.js

+module.exports = exports = function() {
  return exports;
-}
+};


You can remove this if you update the files that include this file:

$ git grep -n tls-connect test/ test/parallel/test-tls-addca.js:11:} = require(join(common.fixturesDir, 'tls-connect'))(); test/parallel/test-tls-ca-concat.js:10:} = require(join(common.fixturesDir, 'tls-connect'))(); test/parallel/test-tls-cert-chains-concat.js:10:} = require(join(common.fixturesDir, 'tls-connect'))(); test/parallel/test-tls-cert-chains-in-ca.js:10:} = require(join(common.fixturesDir, 'tls-connect'))(); test/parallel/test-tls-connect-secure-context.js:9:} = require(join(common.fixturesDir, 'tls-connect'))(); test/parallel/test-tls-peer-certificate.js:9:} = require(join(common.fixturesDir, 'tls-connect'))(); test/parallel/test-tls-socket-default-options.js:10:} = require(join(common.fixturesDir, 'tls-connect'));

Simply remove the () at the end.

Ah great, was not sure I was "allowed" to do that :) Thanks for the review!

danbev · 2017-03-03T08:46:34Z

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

Currently when node is build --without-ssl and the test are run, there are a number of failing test due to tests expecting crypto support to be available. This commit fixes fixes the failure and instead skips the tests that expect crypto to be available. PR-URL: nodejs#11631 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

danbev · 2017-03-04T19:21:54Z

Landed in 1402fef

evanlucas · 2017-03-07T11:45:44Z

This doesn't appear to work properly when cherry-picked to v7.x-staging. This is the error I am getting when trying to run make test

Traceback (most recent call last):
  File "tools/test.py", line 1727, in <module>
    sys.exit(Main())
  File "tools/test.py", line 1607, in Main
    GetV8InspectorEnabledFlag())
  File "tools/test.py", line 929, in GetV8InspectorEnabledFlag
    return config_gypi['variables']['v8_enable_inspector']
KeyError: 'v8_enable_inspector'
make: *** [test] Error 1

Mind opening a backport PR?

Currently when node is build --without-ssl and the test are run, there are a number of failing test due to tests expecting crypto support to be available. This commit fixes fixes the failure and instead skips the tests that expect crypto to be available. PR-URL: nodejs#11631 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

gibfahn · 2017-06-17T20:27:42Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

gibfahn · 2017-06-18T16:45:05Z

Probably makes sense to backport #12485 in the same PR.

gibfahn · 2017-06-20T22:53:20Z

Also #12882 (and any other later ones that use skipIfInspectorDisabled).

danbev · 2017-06-21T05:17:46Z

@gibfahn Sorry about not replying to the backporting issues/requests. I'm not ignoring them, I'm just a little swamped this week and hope to take a closer look at them next week.

gibfahn · 2017-06-21T07:34:46Z

@danbev there's no rush! If it misses the windows for this release you might have to rebase before the next one, but otherwise it doesn't make a big difference. If something really needs to be backported I'll let you know.

MylesBorins · 2017-10-16T21:44:36Z

I've backported common.skipIfInspectorDisabled with #12757 so the test will pass. we may want to consider backporting this PR wholesale though

gibfahn · 2017-11-13T23:28:54Z

If we backport the rest of this to v6.x we should include #16621

danbev added the wip Issues and PRs that are still a work in progress. label Mar 1, 2017

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 1, 2017

jasnell approved these changes Mar 1, 2017

View reviewed changes

mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Mar 1, 2017

danbev force-pushed the without-ssl-crypto-test-skip branch from abdc101 to b81bdb2 Compare March 2, 2017 06:48

danbev removed the wip Issues and PRs that are still a work in progress. label Mar 2, 2017

bnoordhuis requested changes Mar 2, 2017

View reviewed changes

richardlau reviewed Mar 2, 2017

View reviewed changes

danbev closed this Mar 2, 2017

danbev reopened this Mar 2, 2017

danbev force-pushed the without-ssl-crypto-test-skip branch from c47e103 to e648edb Compare March 2, 2017 18:45

danbev added 7 commits March 3, 2017 07:33

test: make addons/openssl-binding pass without-ssl

c9c6c2b

Add checks to the compilation of this addon and also a check when the test is run to see if ssl is enabled or not. If it is not enabled then skip this test.

remove empty line added by mistake

a7b0c2e

remove checkCrypto function

4084f4b

correct comment that does not make sense

f6d7cb0

move reading of v8_enable_inspector to tools/test.py

b440006

danbev force-pushed the without-ssl-crypto-test-skip branch from e648edb to b440006 Compare March 3, 2017 06:41

bnoordhuis approved these changes Mar 3, 2017

View reviewed changes

remove function call when requiring tls-connect

3addfd0

danbev closed this Mar 4, 2017

evanlucas added the dont-land-on-v7.x label Mar 7, 2017

italoacasas mentioned this pull request Mar 14, 2017

inspector: proper WS URLs when bound to 0.0.0.0 #11755

Merged

2 tasks

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

danbev deleted the without-ssl-crypto-test-skip branch April 20, 2017 07:56

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the backport-requested-v6.x label Jun 17, 2017

gibfahn mentioned this pull request Jun 18, 2017

test: skip tests using ca flags #12485

Closed

2 tasks

gibfahn mentioned this pull request Jun 20, 2017

test: add skipIfInspectorDisabled to debugger-pid #12882

Closed

2 tasks

gibfahn mentioned this pull request Jan 16, 2018

tools: allow running test.py without configuring #16621

Merged

2 tasks

Uh oh!

Conversation

danbev commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented Mar 1, 2017

Uh oh!

danbev commented Mar 2, 2017

Uh oh!

bnoordhuis commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Mar 2, 2017

Uh oh!

danbev commented Mar 2, 2017

Uh oh!

danbev commented Mar 3, 2017

Uh oh!

danbev commented Mar 3, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Mar 3, 2017

Uh oh!

danbev commented Mar 4, 2017

Uh oh!

evanlucas commented Mar 7, 2017

Uh oh!

gibfahn commented Jun 17, 2017

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

gibfahn commented Jun 20, 2017

Uh oh!

danbev commented Jun 21, 2017

Uh oh!

gibfahn commented Jun 21, 2017

Uh oh!

MylesBorins commented Oct 16, 2017

Uh oh!

gibfahn commented Nov 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

danbev commented Mar 1, 2017 •

edited

Loading

bnoordhuis commented Mar 2, 2017 •

edited

Loading