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: fix crypto-binary-default bad crypto check #1141

Conversation

brendanashworth
Copy link
Contributor

This commit fixes a small bug introduced in 671fbd5
that caused the test to not be run. crypto was properly
checked, but since tls was not imported, a TypeError
would be thrown in the try {} catch {} block and
falsely reported as no crypto.

This is now fixed.

cc @jbergstroem

@jbergstroem
Copy link
Member

Ah, nice catch. Thanks for fixing this. I'd give you LGTM but you'll have to wait for next week :-)

Did you look for other tests that did similar stuff? Otherwise I can give it a go.

@brendanashworth
Copy link
Contributor Author

@jbergstroem haha. I did a check for TLS related ones like this one and I couldn't find any, but that doesn't mean there aren't others. I wasn't stalking your commit btw, I just happened across it while doing something else.

@jbergstroem
Copy link
Member

I'm planning to run through the test suite for other reasons shortly. I'll have one eye open for this.

@brendanashworth
Copy link
Contributor Author

Now that you're a collaborator, does this LGTY? @jbergstroem

@jbergstroem
Copy link
Member

Looking closer at it - it doesn't look like context is used at all in the test?

@brendanashworth
Copy link
Contributor Author

Ah, looks like its predecessor, credentials, was never used since the test was introduced ~3 years ago.

I'll push, removing it altogether.

This commit fixes a small bug introduced in 671fbd5
that caused the test to not be run. crypto was properly
checked, but since tls was not imported, a TypeError
would be thrown in the `try {} catch {}` block and
falsely reported as no crypto.

This is now fixed.
@brendanashworth
Copy link
Contributor Author

Updated. PTAL @jbergstroem

@jbergstroem
Copy link
Member

LGTM :shipit:

brendanashworth added a commit that referenced this pull request Mar 22, 2015
This commit fixes a small bug introduced in 671fbd5
that caused the test to not be run. crypto was properly
checked, but since tls was not imported, a TypeError
would be thrown in the `try {} catch {}` block and
falsely reported as no crypto.

This is now fixed.

PR-URL: #1141
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@brendanashworth
Copy link
Contributor Author

Thanks, merged in 999fbe9.

@rvagg rvagg mentioned this pull request Mar 22, 2015
@brendanashworth brendanashworth deleted the fix-test-crypto-binary-default branch April 6, 2015 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants