-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
refector wait.HTTPStrategy #221
Conversation
islishude
commented
Jun 24, 2020
- add method and body for request
- add tls.Config for https
- use time.After instead of time.Sleep
Hey @islishude, thank you for this nice contribution! Do you think it would be possible to add a test for the new behavior, specially the TLSConfig part? Thanks! |
OK,I will add test cases for it |
Hi,the tests pass,please review @mdelapenya |
return | ||
} | ||
defer resp.Body.Close() | ||
if resp.StatusCode != http.StatusOK { |
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.
What do you think about adding an assertion for the pong
response? We'd like to check that the response matcher works as intended.
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 variable indicates a state code, and I consider the value of pong
to be a success state.
in practice, the server can return a response body with a state code representing the current server state,so we can know if the server is ready based on what is specifically returned.
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.
:) I meant about adding another assertion for the expected response (pong), which is part of the waitFor condition predefined when setting up the test in the response matcher. Of course this is not a requirement by the way, so I'm not opposing to it.
In the same hand, the PR as it's now could be merged, so I want to thank you for your contribution