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: make tests pass when configured --without-ssl #11631

Closed
wants to merge 8 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Mar 1, 2017

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 danbev added the wip Issues and PRs that are still a work in progress. label Mar 1, 2017
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 1, 2017
@danbev
Copy link
Contributor Author

danbev commented Mar 1, 2017

@mscdex mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Mar 1, 2017
@danbev danbev force-pushed the without-ssl-crypto-test-skip branch from abdc101 to b81bdb2 Compare March 2, 2017 06:48
@danbev danbev removed the wip Issues and PRs that are still a work in progress. label Mar 2, 2017
@danbev
Copy link
Contributor Author

danbev commented Mar 2, 2017

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

@bnoordhuis
Copy link
Member

bnoordhuis commented Mar 2, 2017

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.

@@ -19,6 +22,7 @@ function checkCrypto() {
return exports;
}


Copy link
Member

Choose a reason for hiding this comment

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

Whitespace change.

# The following block reads config.gypi to extract the v8_enable_inspector
# value. This is done to check if the inspector is disabled in which case
# the '--inspect' flag cannot be passed to the node process as it will
# that will cause node to exit and report the test as failed. The use case
Copy link
Member

Choose a reason for hiding this comment

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

"it will that will"

'sources': ['binding.cc'],
'include_dirs': ['../../../deps/openssl/openssl/include'],
}]
]
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -6,6 +6,9 @@
const common = require('../common');
const fs = require('fs');
const join = require('path').join;
// Check if Node was compiled --without-ssl and if so exit early
// as the require of tls will otherwise throw an Error.
checkCrypto();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if flag[0].startswith('--inspect') and v8_disable_inspector:
print('Skipping as inspector is disabled')
else:
result += flag
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -1,6 +1,10 @@
'use strict';

const common = require('../../common');
if (!common.hasCrypto) {
common.skip('missing crypto');
process.exit(0);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

danbev commented Mar 2, 2017

@danbev danbev closed this Mar 2, 2017
@danbev danbev reopened this Mar 2, 2017
@danbev danbev force-pushed the without-ssl-crypto-test-skip branch from c47e103 to e648edb Compare March 2, 2017 18:45
@danbev
Copy link
Contributor Author

danbev commented Mar 2, 2017

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 danbev force-pushed the without-ssl-crypto-test-skip branch from e648edb to b440006 Compare March 3, 2017 06:41
@danbev
Copy link
Contributor Author

danbev commented Mar 3, 2017

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
Copy link
Contributor Author

danbev commented Mar 3, 2017

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

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.

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

return exports;
}
};
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@danbev
Copy link
Contributor Author

danbev commented Mar 3, 2017

danbev added a commit to danbev/node that referenced this pull request Mar 4, 2017
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Mar 4, 2017

Landed in 1402fef

@danbev danbev closed this Mar 4, 2017
@evanlucas
Copy link
Contributor

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?

@jasnell jasnell mentioned this pull request Apr 4, 2017
danbev added a commit to danbev/node that referenced this pull request Apr 11, 2017
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@danbev danbev deleted the without-ssl-crypto-test-skip branch April 20, 2017 07:56
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

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 gibfahn mentioned this pull request Jun 18, 2017
2 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

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

@gibfahn
Copy link
Member

gibfahn commented Jun 20, 2017

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

@danbev
Copy link
Contributor Author

danbev commented Jun 21, 2017

@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
Copy link
Member

gibfahn commented Jun 21, 2017

@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
Copy link
Contributor

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

@gibfahn
Copy link
Member

gibfahn commented Nov 13, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants