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.

Sign up for GitHub

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

Jump to bottom

test: do not swallow OpenSSL support error #2042

Closed
wants to merge 1 commit into from
Closed

test: do not swallow OpenSSL support error #2042

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 23, 2015

All of the CI tests have OpenSSL enabled so the catch() block (gracefully swallowing the error if is not enabled) is unexercised.

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

@ChALkeR ChALkeR added the test Issues and PRs related to the tests. label Jun 23, 2015
@Trott Trott added the crypto Issues and PRs related to the crypto subsystem. label Jun 23, 2015
@mscdex
Copy link
Contributor

mscdex commented Jun 23, 2015

So I guess this means we aren't or haven't been regularly testing building without openssl?

@Trott
Copy link
Member Author

Trott commented Jun 23, 2015

The CI came back all green, so yes, that would appear to be the case. I don't know if that whole "check for OpenSSL" part was from before crypto was firmly established in core, or if an issue should be opened to test alternate builds, or something else. But it seems like removing the error-swallowing makes sense, at least for the CI. If anything in tls fails, it seems the test should blow up, rather than be skipped. (So now it will blow up in the subsequent code in the test.)

@brendanashworth
Copy link
Contributor

It isn't about the CI having OpenSSL all enabled or not (it may - I don't know), but the test is skipped regardless because of an above check. Looks like I missed this one in 999fbe9.

@jbergstroem
Copy link
Member

@mscdex I ran a few builds some month ago without openssl support and had the test suite pass. I test every now and then as part of the gentoo iojs packaging as well.

@Trott
Copy link
Member Author

Trott commented Jun 23, 2015

@brendanashworth Ah, I see. So, this change purges some dead code, hooray and all that, but the other comments I made are not well-founded. Is that about right?

@brendanashworth
Copy link
Contributor

@Trott seems right :-), LGTM.

@jbergstroem
Copy link
Member

LGTM; should've removed this while adding the new crypto check.

Trott added a commit to Trott/io.js that referenced this pull request Jun 25, 2015
@Trott
test: do not swallow OpenSSL support error
4d5089e 
PR-URL: nodejs#2042
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@Trott
Copy link
Member Author

Trott commented Jun 25, 2015

Merged in 4d5089e

@Trott Trott closed this Jun 25, 2015
@rvagg rvagg mentioned this pull request Jun 30, 2015
Closed
mscdex pushed a commit to mscdex/io.js that referenced this pull request Jul 9, 2015
@Trott @mscdex
test: do not swallow OpenSSL support error
e9ee293 
PR-URL: nodejs#2042
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@Trott Trott deleted the test-tls-support branch January 9, 2022 22:02
@aliscco aliscco mentioned this pull request Apr 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Trott @mscdex @brendanashworth @jbergstroem @ChALkeR