test: fix test failure with shared openssl #762
Conversation
| // use external command | ||
| exports.opensslCli = 'openssl'; | ||
| } else { | ||
| // use build-in command |
There was a problem hiding this comment.
kind of very irrelevant, but built-in
There was a problem hiding this comment.
@brendanashworth Do you have a proper word? I think of others such as bundled or included command in iojs or to delete this comment.
|
The openssl binary is now properly called. Minor language nits, but lgtm. |
| try { | ||
| fs.accessSync(exports.opensslCli); | ||
| } catch (err) { | ||
| var openssl_cmd = child_process.spawnSync(exports.opensslCli, ['version']); |
There was a problem hiding this comment.
Ideally, this only runs when the openssl binary is actually needed. Spawning a child process adds a few milliseconds to each test; multiplied by ~600 tests, that's a couple of extra seconds. Maybe you can turn the .opensslCli property into a lazy getter?
|
@bnoordhuis Thanks for your review. Indeed, |
| var opensslCli = null; | ||
|
|
||
|
|
||
| function opensslCliGetter() { |
There was a problem hiding this comment.
Maybe add a if (opensslCli !== null) return opensslCli; at the top? Avoids multiple invocations of the openssl binary.
EDIT: Never mind, I see you're doing that below. Just curious: is there a reason to split the logic into two functions?
There was a problem hiding this comment.
@bnoordhuis Splitting is just for easy to read and notify this is a getter function. I don't mind to change it into inline.
There was a problem hiding this comment.
Inline is a little easier to comprehend when you read the code top-down.
|
LGTM with a question. |
When configured with share openssl, use external openssl command and check if it can be executed. Fixes: nodejs#618
|
@bnoordhuis Changed this into inline and landed c86e383 . Thanks |
|
@brendanashworth The comment was revised more clearly as c86e383#diff-8736c5cbff21e1dee18b0c86d3d2689dR32 according your review. Thanks. |
When configured with share openssl, use external openssl command and check if it can be executed.
Fixes: #618
Cc: @jbergstroem
Please someone in @iojs/collaborators review this.