Crypto tests overhaul #1049

jbergstroem · 2015-03-04T01:19:46Z

So, based on the situation we introduced in #1035 (someone actually running tests with ./configure --without-ssl), I rewrote the way we look for crypto/tls/https support.

A few things:

Introduce a helper in common.js that checks for process.versions.openssl. This could potentially be changed to try importing crypto instead.
Review all tests that fail (and the one's that imported tls, https or crypto) to use the new check
Unify tests that had their own way of checking for various types of crypto support
Have a few tests do partial runs (one or two) -- for instance tests that checks both against http and https servers. These should probably be moved out to their own tests
Fix fallout found in for instance testing process.versions (my bad)
Avoid style/lint changes as much as possible, but update two tests (2a4a51d) that broke because they imported https while not using it.

Since this affects a lot of files I'd appreciate a quicker feedback round; can see myself rebasing this a fair few times otherwise.

rvagg · 2015-03-04T04:15:57Z

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/240/

since this applies to tls and https (among other things), it'll be used for those tests as well. if we decouple the build system to somehow support crypto but not tls, we could refine this.

the previous version checked if io.js was compiled with openssl support which isn't really relevant since we're starting a http server. we on the other hand need an openssl-cli which may or may not exist.

jbergstroem · 2015-03-04T06:32:03Z

(changed some commit message texts)

jbergstroem · 2015-03-04T07:51:28Z

/cc @brendanashworth @shigeki

bnoordhuis · 2015-03-04T12:41:36Z

Left some comments. Perhaps it'd be nice to make the skip message TAP-compatible, i.e. console.log('# SKIP skipping, no crypto')

jbergstroem · 2015-03-04T12:46:49Z

@bnoordhuis Thanks for the feedback. I'll revise shortly. While going through a bunch of tests, I came to realise there's a fair few things to clean up and improve.

With regards to your syntax suggestion, how about doing a helper for skip (and possibly exit) which is configurable so we more easily can control output? I guess its out of scope for this PR, but it could come shortly thereafter.

bnoordhuis · 2015-03-04T19:34:42Z

With regards to your syntax suggestion, how about doing a helper for skip (and possibly exit) which is configurable so we more easily can control output?

Virtual shrug. Is there a need for it?

jbergstroem · 2015-03-04T22:42:24Z

@bnoordhuis just updated, fixing your remarks. I also split the https part of test-http-host-header into its own test. I left buffer alone for now.

As for the helper: we could control skip output type so different test runner understands them. No strong argument though, tap is readable enough.

jbergstroem · 2015-03-04T22:58:42Z

Just added tap output (output suggested here).

shigeki · 2015-03-05T00:45:27Z

test/parallel/test-buffer.js

-  crypto.createHash('sha1').update(b1).digest('hex'),
-  crypto.createHash('sha1').update(b2).digest('hex')
-);
+if(common.hasCrypto) {


styles: Missing space before "(". There are the same styles in this diff.
Tests files are usually not checked by gjshint so we might need not to care about styles so much.

Darn it, this case slipped through.

shigeki · 2015-03-05T00:49:34Z

I run tests without ssl and release, message parallel and sequential are fine. There still remains tests using openssl in pummel but I think very few people use tests without ssl. Overall LGTM. Good Jobs.

jbergstroem · 2015-03-05T00:51:46Z

Good point regarding pummel. I'll have a look at that as well.

we had a few ways versions of looking for support before executing a test. this commit unifies them as well as add the check for all tests that previously lacked them. found by running `./configure --without-ssl && make test`. also, produce tap skip output if the test is skipped.

this makes the separation between http and https testing cleaner

jbergstroem · 2015-03-05T01:16:47Z

@shigeki fixed the styling nit as well as broken tests in pummel and internet. Also removed some unused stuff as a result of splitting up a test.

PR-URL: #1049 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

since this applies to tls and https (among other things), it'll be used for those tests as well. if we decouple the build system to somehow support crypto but not tls, we could refine this. PR-URL: #1049 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

PR-URL: #1049 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

the previous version checked if io.js was compiled with openssl support which isn't really relevant since we're starting a http server. we on the other hand need an openssl-cli which may or may not exist. PR-URL: #1049 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

we had a few ways versions of looking for support before executing a test. this commit unifies them as well as add the check for all tests that previously lacked them. found by running `./configure --without-ssl && make test`. also, produce tap skip output if the test is skipped. PR-URL: #1049 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

this makes the separation between http and https testing cleaner PR-URL: #1049 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

shigeki · 2015-03-05T01:37:23Z

Thanks, I checked all tests including pummel and internet are fine. Landed in 563771d , 671fbd5 , c7ad320 , 71776f9 , 3d5726c and d0e7c35 .
@bnoordhuis I add your name in the reviewer. If it is not good, please tell me it.

bnoordhuis · 2015-03-05T12:55:29Z

@shigeki That's fine. My only nit is about the commit logs: the first line should be <= 50 chars, the body <= 72 chars and properly capitalized and punctuated.

test: don't assume process.versions.openssl always is available

62300e6

jbergstroem added 3 commits March 4, 2015 17:30

test: introduce a helper that checks if crypto is available

15a6001

since this applies to tls and https (among other things), it'll be used for those tests as well. if we decouple the build system to somehow support crypto but not tls, we could refine this.

test: remove unused https imports

c2f0a62

test: check for openssl cli and provide path if it exists

6425238

the previous version checked if io.js was compiled with openssl support which isn't really relevant since we're starting a http server. we on the other hand need an openssl-cli which may or may not exist.

jbergstroem force-pushed the crypto_tests_overhaul branch from bf9bf8e to b9785b7 Compare March 4, 2015 06:31

jbergstroem force-pushed the crypto_tests_overhaul branch from b9785b7 to 5c764ff Compare March 4, 2015 22:40

jbergstroem force-pushed the crypto_tests_overhaul branch from 5c764ff to ee99c67 Compare March 4, 2015 22:57

shigeki reviewed Mar 5, 2015
View reviewed changes

jbergstroem added 2 commits March 5, 2015 12:14

test: split parts out of host-headers test into its own test

d28898e

this makes the separation between http and https testing cleaner

jbergstroem force-pushed the crypto_tests_overhaul branch from ee99c67 to d28898e Compare March 5, 2015 01:16

shigeki pushed a commit that referenced this pull request Mar 5, 2015

test: don't assume process.versions.openssl always is available

d0e7c35

PR-URL: #1049 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

shigeki pushed a commit that referenced this pull request Mar 5, 2015

test: remove unused https imports

71776f9

PR-URL: #1049 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

shigeki closed this Mar 5, 2015

rvagg mentioned this pull request Mar 5, 2015

Release proposal: v1.5.0 #1060

Closed

This was referenced Sep 15, 2020

http2: The http2 server rejects large headers and the cutoff does not appear to be configurable #35218

Closed

http2: Node crashes with an assertion error if an http2 server is closed after receiving and rejecting very large headers #35233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crypto tests overhaul #1049

Crypto tests overhaul #1049

jbergstroem commented Mar 4, 2015

rvagg commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

bnoordhuis commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

bnoordhuis commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

shigeki Mar 5, 2015

jbergstroem Mar 5, 2015

shigeki commented Mar 5, 2015

jbergstroem commented Mar 5, 2015

jbergstroem commented Mar 5, 2015

shigeki commented Mar 5, 2015

bnoordhuis commented Mar 5, 2015

Crypto tests overhaul #1049

Crypto tests overhaul #1049

Conversation

jbergstroem commented Mar 4, 2015

rvagg commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

bnoordhuis commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

bnoordhuis commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

jbergstroem commented Mar 4, 2015

shigeki Mar 5, 2015

Choose a reason for hiding this comment

jbergstroem Mar 5, 2015

Choose a reason for hiding this comment

shigeki commented Mar 5, 2015

jbergstroem commented Mar 5, 2015

jbergstroem commented Mar 5, 2015

shigeki commented Mar 5, 2015

bnoordhuis commented Mar 5, 2015