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

[tests] linkcheck: bind each test HTTP server to a unique port per-testcase. #12126

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Mar 18, 2024

Feature or Bugfix

  • Refactoring

Purpose

  • Reduce the possibility that linkcheck builder tests can interfere with each other.
  • Allow the linkcheck builder tests to run in arbitrary order.

Detail

  • Delegate network port binding to Python by requesting port number 0, as illustrated in the Python documentation's socketserver sample code for ThreadingMixIn.
  • Return a localhost:{port} address for each test that it can make linkcheck builder requests to.
  • Dynamically rewrites source hyperlinks that refer to the address localhost:7777 during linkcheck tests to remap these to the test-specific HTTP(S) server.
  • Removes the filelock dependency from our test dependencies since as this should no longer be required with the above changes in place.

Relates

@jayaddison jayaddison changed the title Issue 12122/linkcheck tests httpserver port isolation [tests] linkcheck: bind each test HTTP server to a unique port per-testcase. Mar 18, 2024
tests/utils.py Outdated Show resolved Hide resolved
.ruff.toml Outdated Show resolved Hide resolved
jayaddison and others added 2 commits March 22, 2024 10:56
@jayaddison jayaddison requested a review from picnixz March 24, 2024 21:28
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Sometimes I want to use a specific port (either because I don't trust Python or my OS, or I want to check that it fails because I know the port is busy) so I'd suggest this.

I'm sorry I didn't suggest this before by the way.

.gitignore Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
jayaddison and others added 2 commits March 25, 2024 08:22
…ts; add a changelog entry instead.

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
tests/utils.py Outdated Show resolved Hide resolved
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
@jayaddison
Copy link
Contributor Author

Stick to common test ports (xxxx for x = 5,6,7,8) possibly with xxx(x+1) and xxx(x-1) but don't use other ports.

@picnixz going back to check that I haven't overlooked things: was there a compelling reason to prefer using only recognizable-as-maybe-test port numbers like these? I think the port 0 dynamic allocation is a neat solution, but it does conflict with that numbering scheme suggestion.

@picnixz
Copy link
Member

picnixz commented Apr 3, 2024

was there a compelling reason to prefer using only recognizable-as-maybe-test port numbers like these

Not really. It's just that I think it would have been easier to debug in general.

@jayaddison
Copy link
Contributor Author

was there a compelling reason to prefer using only recognizable-as-maybe-test port numbers like these

Not really. It's just that I think it would have been easier to debug in general.

In situations where an individual test is being troublesome, and repeating it and perhaps inspecting the network requests to try to debug that, this seems very true.

Perhaps an environment variable (TEST_HTTP_PORT) could override the default port number when set? It's a small amount of additional complexity, but would allow use of a static port number without changing the codebase locally.

@picnixz
Copy link
Member

picnixz commented Apr 4, 2024

TEST_HTTP_PORT

Not really, if you have a port issue, you would just explicitly use the port (that's why I asked you that you could explicitly change the port number if needed). We could consider it in another PR if we see that we need it.

By the way, I'm updating the branch, waiting for everything to work and then LGTM.

@picnixz
Copy link
Member

picnixz commented Apr 4, 2024

Thank you !

@picnixz picnixz merged commit df3cde6 into sphinx-doc:master Apr 4, 2024
22 checks passed
@jayaddison
Copy link
Contributor Author

Thank you @picnixz for the code reviews and suggestions 👍

@jayaddison jayaddison deleted the issue-12122/linkcheck-tests-httpserver-port-isolation branch April 4, 2024 09:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
builder:linkcheck dependencies Pull requests that update a dependency file internals:refactoring python Pull requests that update Python code type:proposal a feature suggestion type:tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tests] linkcheck builder unit tests fail sporadically when run using unpredictable orderings.
2 participants