Crypto tests overhaul #1049
Crypto tests overhaul #1049
Conversation
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
force-pushed
the
crypto_tests_overhaul
branch
from
bf9bf8e
to
b9785b7
Compare
|
(changed some commit message texts) |
|
Left some comments. Perhaps it'd be nice to make the skip message TAP-compatible, i.e. |
|
@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. |
Virtual shrug. Is there a need for it? |
jbergstroem
force-pushed
the
crypto_tests_overhaul
branch
from
b9785b7
to
5c764ff
Compare
|
@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
force-pushed
the
crypto_tests_overhaul
branch
from
5c764ff
to
ee99c67
Compare
|
Just added tap output (output suggested here). |
| crypto.createHash('sha1').update(b1).digest('hex'), | ||
| crypto.createHash('sha1').update(b2).digest('hex') | ||
| ); | ||
| if(common.hasCrypto) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Darn it, this case slipped through.
|
I run tests without ssl and |
|
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
force-pushed
the
crypto_tests_overhaul
branch
from
ee99c67
to
d28898e
Compare
|
@shigeki fixed the styling nit as well as broken tests in |
PR-URL: #1049 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]>
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 <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]>
PR-URL: #1049 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]>
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 <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]>
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 <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]>
this makes the separation between http and https testing cleaner PR-URL: #1049 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]>
|
@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. |
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:
process.versions.openssl. This could potentially be changed to try importing crypto instead.process.versions(my bad)Since this affects a lot of files I'd appreciate a quicker feedback round; can see myself rebasing this a fair few times otherwise.