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

Support for HTTPS testing #50

Merged
merged 16 commits into from
Apr 10, 2019
Merged

Conversation

okin
Copy link
Contributor

@okin okin commented Apr 1, 2019

This adds support for testing with HTTPS.
Based on the work of @judeaugustinej in PR #22 to fix #21.

It adds fixtures https_port, https_server, https_client and secure_base_url.

Even though it works I'd like things to be easier to use in regard of base_url, because this introduces another fixture secure_base_url. I'd like to have only the base_url fixture available but am currently not sure if or how this could be implemented.
Feedback in this regard is more than welcome!

Options for SSL have to be given through an fixture ssl_options in a way like currently app is used.

@okin okin marked this pull request as ready for review April 2, 2019 06:58
@okin okin changed the title WIP: Support for HTTPS testing Support for HTTPS testing Apr 2, 2019
@vidartf
Copy link
Collaborator

vidartf commented Apr 4, 2019

I'd like to have only the base_url fixture available but am currently not sure if or how this could be implemented.

I think you can use request.fixturenames in base_url and see if http[s]_server is in there.

test/create_cert.py Outdated Show resolved Hide resolved
@okin
Copy link
Contributor Author

okin commented Apr 7, 2019

@vidartf thanks for the tip with request.fixturenames. I was able to get it together with this.

Let me know what you think!

NamedTemporaryFiles cannot be reopen while still open on Windows. By closing it first, we can use it. So we have to manage deletion manually.
@vidartf
Copy link
Collaborator

vidartf commented Apr 8, 2019

Note: I added a commit for making create_cert.py work on Windows. See commit message for details.

Copy link
Collaborator

@vidartf vidartf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, pending your review of my commit! The only possible improvement I can see would be to give a warning if a user requests a mismatched secure/non-secure server/client/port tuple. Could be an easy typo that would be hard to catch, but I'm not sure if it is worth the effort though, so I will leave that as a possible follow-up PR (ping if you want it).

@okin
Copy link
Contributor Author

okin commented Apr 8, 2019

Note: I added a commit for making create_cert.py work on Windows. See commit message for details.

I'm fine with this.

The only possible improvement I can see would be to give a warning if a user requests a mismatched secure/non-secure server/client/port tuple. Could be an easy typo that would be hard to catch, but I'm not sure if it is worth the effort though, so I will leave that as a possible follow-up PR (ping if you want it).

I would skip this for now and wait how users are picking up the new feature. If this becomes a common mistake I think such a warning should be implemented.
Will add an issue so this won't be forgotten.

@vidartf vidartf merged commit 3b48ac3 into eugeniy:master Apr 10, 2019
@okin okin deleted the feature/https_support branch April 10, 2019 16:58
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.

Support for https testing
3 participants