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

Adding IgnoreSSLVerify option #113

Merged
merged 4 commits into from
Jun 20, 2019
Merged

Adding IgnoreSSLVerify option #113

merged 4 commits into from
Jun 20, 2019

Conversation

jgazeau
Copy link

@jgazeau jgazeau commented Mar 15, 2019

No description provided.

@jgazeau
Copy link
Author

jgazeau commented Mar 15, 2019

Patch to add IgnoreSSLVerify option (issue #112 )

@jgazeau jgazeau closed this Mar 15, 2019
@jgazeau jgazeau reopened this Mar 15, 2019
@wjdp
Copy link
Owner

wjdp commented Mar 16, 2019

PR makes sense, simple and effective! Unsure why the tests are failing, having a quick look now.

I've tweaked the README wording and would be nice to have a test case for this option.

@wjdp
Copy link
Owner

wjdp commented Mar 16, 2019

Is failing on master as well, some package or the Travis environment must have changed. Very odd.

@jgazeau
Copy link
Author

jgazeau commented Mar 18, 2019

It seems that they changed something in the golan url_test.go and add a new check for special char (check : golang/go@829c5df#diff-6f43cc724ae52e4acba8855ce391e144)
I agree that it would be nice to have a test to check the IgnoreSSLVerify option. I'll check this and see how to do it.

@wjdp
Copy link
Owner

wjdp commented Apr 1, 2019

@jgazeau Can you rebase this on master?

@codecov
Copy link

codecov bot commented Apr 1, 2019

Codecov Report

Merging #113 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
+ Coverage   86.53%   86.56%   +0.02%     
==========================================
  Files          20       20              
  Lines        1062     1064       +2     
==========================================
+ Hits          919      921       +2     
  Misses        136      136              
  Partials        7        7
Impacted Files Coverage Δ
htmltest/options.go 90.14% <100%> (+0.14%) ⬆️
htmltest/htmltest.go 97.84% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b5831f...c580c11. Read the comment docs.

@jgazeau
Copy link
Author

jgazeau commented Apr 1, 2019

@wjdp Rebase done.
Haven't checked to implement a test for this option yet.

@wjdp
Copy link
Owner

wjdp commented Apr 3, 2019

Thanks, I'll hold off merging until you've done a test case.

@jgazeau
Copy link
Author

jgazeau commented Apr 26, 2019

@wjdp,

I add two test cases (TestSelfSignedLink and TestSelfSignedLinkIgnoreSSLVerify) in the check-link_test.go.

  • TestSelfSignedLink is checking if a link with a selfsigned certificate is returning an error with the IgnoreSSLVerify=false
  • TestSelfSignedLinkIgnoreSSLVerify is checking if a link with a selfsigned certificate is not returning any error with the IgnoreSSLVerify=true

For the test I use the self-signed link of https://badssl.com/.

FYI: To be sure, I run one pipeline to check the job is throwing errors by swaping the IgnoreSSLVerify boolean value :
https://travis-ci.org/jgazeau/htmltest/jobs/524849941 (Line 764)

Is that Ok for you ?

@wjdp
Copy link
Owner

wjdp commented Jun 20, 2019

@jgazeau Whoops, sorry I missed your last comment. Thanks for the PR!

@wjdp wjdp merged commit f8a0ead into wjdp:master Jun 20, 2019
@wjdp wjdp mentioned this pull request Jun 20, 2019
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