-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
tests: implement readiness check before yielding local-http(s) test servers #12050
tests: implement readiness check before yielding local-http(s) test servers #12050
Conversation
This reverts commit 396b8df.
…) servers for use in test suite
I'm going to push a few more commits for this momentarily, including a changelog entry. But this code also needs to be evaluated on a large-batch of Windows CI unit test builds, to check that it's reliable. |
If you want to check whether it's reliable or not and you don't have access to Windows, just commit a space diff, then commit without the space and again just to trigger the CI/CD |
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
… resource cleanup Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
…nnection resource cleanup" This reverts commit 9720039.
This reverts commit 6bb4797.
…untu tests" This reverts commit 4364c6c.
…estionable grammar).
tests/utils.py
Outdated
@@ -45,7 +50,8 @@ def server(handler): | |||
server_thread = thread_class(handler, daemon=True) | |||
server_thread.start() | |||
try: | |||
yield server_thread | |||
socket.create_connection(ADDRESS, timeout=0.5).close() # attempt connection. |
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.
If you want to end your sentences with periods you should also start with a capital letter (I think it's fine not to have a period for non-capitalized small comments in general)
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.
Ok, that's reasonable :)
The full-stop was partly to reduce the chance that one or other of the lines could be deleted, leaving the other one parse as valid but without the relevant context.
Is it fine to merge? I don't have anything else to say |
I count 50 Python test builds on Windows without seeing the failure occur again - that gives me reasonable (although not complete) confidence that it's OK to merge. Even so, I'll monitor the test results on mainline and PRs for the next few weeks. |
Shorter answer: yes, I think so. (but after this many attempts I still won't be surprised if another Windows test failure does occur somehow!) |
Thank you! |
And thank you for the review and merge! |
The same error message appeared again in #12065. |
…servers (sphinx-doc#12050) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> (cherry picked from commit 1bfddf8) Conflicts: CHANGES.rst tests/utils.py
Feature or Bugfix
Purpose
Detail
socket
-based readiness check on each test HTTP(S) server created for use during unit tests beforeyield
ing it for use by the corresponding test case.Relates