-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add smoke tests for stunnel #688
base: main
Are you sure you want to change the base?
Conversation
fab5f6c
to
c6e256a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, but it needs a rebase.
c6e256a
to
f34c405
Compare
f34c405
to
ed1a8f6
Compare
"STUNNEL_DEBUG": log_level, | ||
"STUNNEL_SERVICE_NAME": "neverssl", | ||
"STUNNEL_ACCEPT": f"0.0.0.0:{_CTR_PORT}", | ||
"STUNNEL_CONNECT": "neverssl.com:80", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we trust this won't cause random failures in CI?
ed1a8f6
to
7621fb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpick, otherwise LGTM.
resp = get_request_to_stunnel() | ||
assert resp.status_code == 200 | ||
assert ( | ||
"This website is for when you try to open Facebook, Google, Amazon, etc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This website is for when you try to open Facebook, Google, Amazon, etc" | |
assert "neverssl" in resp.text.lower() |
Making assumptions on external resources we have no control over is always difficult. I would limit the assumption to a minimum, so that the test doesn't fail when someone slightly changes the text on neverssl.com. We already check for status code 200, I think this check doesn't need to be extensive here.
[CI:TOXENVS] stunnel
for SUSE/BCI-dockerfile-generator#2110