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.

Sign up for GitHub

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

Jump to bottom

tests: implement readiness check before yielding local-http(s) test servers #12050

Merged
merged 19 commits into from
Mar 8, 2024
Merged

tests: implement readiness check before yielding local-http(s) test servers #12050

merged 19 commits into from
Mar 8, 2024

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Mar 7, 2024 •
edited
Loading

Feature or Bugfix

  • Bugfix

Purpose

  • Intended to prevent a flaky test failure from causing false-positive failure notices in Sphinx's continuous integration tests on GitHub Actions.

Detail

  • Implements a socket-based readiness check on each test HTTP(S) server created for use during unit tests before yielding it for use by the corresponding test case.

Relates

@jayaddison jayaddison mentioned this pull request Mar 7, 2024
Closed
@jayaddison jayaddison changed the title tests: increase timeout for linkcheck local-https test server tests: implement healthcheck before yielding local-http(s) test servers Mar 7, 2024
@jayaddison jayaddison changed the title tests: implement healthcheck before yielding local-http(s) test servers tests: implement readiness check before yielding local-http(s) test servers Mar 7, 2024
@jayaddison
Copy link
Contributor Author

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.

@jayaddison jayaddison requested a review from picnixz March 7, 2024 11:38
picnixz
picnixz reviewed Mar 7, 2024
View reviewed changes
.github/workflows/main.yml Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
@picnixz
Copy link
Member

picnixz commented Mar 7, 2024

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

jayaddison and others added 7 commits March 7, 2024 17:16
@jayaddison @picnixz
Code review: change variable name 'HOST' to 'ADDRESS'
95dd097 
Co-authored-by: Bénédikt Tran <[email protected]>
@jayaddison @picnixz
Code review: use a context-manager to handle client socket connection…
9720039 
… resource cleanup

Co-authored-by: Bénédikt Tran <[email protected]>
@jayaddison
Revert "Code review: use a context-manager to handle client socket co…
1442999 
…nnection resource cleanup"

This reverts commit 9720039.
@jayaddison
tests: attempt to improve commentary and link it more closely with th…
6b2f11b 
…e code lines
@jayaddison
Revert "ci: fixup: relocate temporary 'if' condition"
df5687e 
This reverts commit 6bb4797.
@jayaddison
Revert "ci: run a matrix of Windows tests, and temporarily disable Ub…
ed1144d 
…untu tests"

This reverts commit 4364c6c.
@jayaddison
tests: nitpick: add full-stops to comment sentences (despite their qu…
7719f1c 
…estionable grammar).
picnixz
picnixz reviewed Mar 7, 2024
View reviewed changes
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.
Copy link
Member

@picnixz picnixz Mar 7, 2024 •
edited
Loading

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)

Copy link
Contributor Author

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.

@picnixz
Copy link
Member

picnixz commented Mar 8, 2024

Is it fine to merge? I don't have anything else to say

@jayaddison
Copy link
Contributor Author

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.

@jayaddison
Copy link
Contributor Author

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!)

@picnixz picnixz merged commit 1bfddf8 into sphinx-doc:master Mar 8, 2024
22 checks passed
@picnixz
Copy link
Member

picnixz commented Mar 8, 2024

Thank you!

@jayaddison
Copy link
Contributor Author

And thank you for the review and merge!

@jayaddison jayaddison deleted the issue-12038/increase-https-selfsigned-test-timeout branch March 8, 2024 10:37
@jayaddison
Copy link
Contributor Author

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.

The same error message appeared again in #12065.

@jayaddison jayaddison mentioned this pull request Mar 13, 2024
Merged
jayaddison added a commit to jayaddison/sphinx that referenced this pull request Mar 20, 2024
@jayaddison
[CI/CD] implement readiness check before yielding local-http(s) test …
9897bb5 
…servers (sphinx-doc#12050)

Co-authored-by: Bénédikt Tran <[email protected]>
(cherry picked from commit 1bfddf8)

Conflicts:
	CHANGES.rst
	tests/utils.py
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2024
@AA-Turner AA-Turner added this to the 7.3.0 milestone Jul 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@picnixz picnixz picnixz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
7.3.0
Development

Successfully merging this pull request may close these issues.

flaky unit test: test_build_linkcheck.test_connect_to_selfsigned_fails
3 participants
@jayaddison @picnixz @AA-Turner