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: refactor to remove variable sharing between tests and test webservers #11426

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented May 17, 2023

Feature or Bugfix

  • Refactoring

Purpose

Detail

  • Refactor to remove the sharing of the records variable between unit tests and their corresponding webserver request handler threads.
  • Allow each test to provide a callable that declares the success criteria, and have the webserver thread evaluate that and communicate a resulting success/failure code using HTTP status codes.

Relates

@jayaddison

This comment was marked as outdated.

@jayaddison jayaddison changed the title tests: linkcheck: refactor to remove variables shared across threads tests: linkcheck: refactor to remove variable sharing between tests and test webservers May 17, 2023
@jayaddison
Copy link
Contributor Author

I'm sorta unsatisfied and not entirely convinced by this explanation, but it does seem that the algorithm used by headers.as_string method is non-trivial complexity-wise; from the relevant documentation it's essentially an email.message.Message-like object, and the latter has had a few bugs and has been rewritten at least once.

Would that complexity be enough to put some of the unit tests near the client timeout threshold? ... I don't know, 0.05s seems like it should be a lot of compute-time budget. 🤷

I've removed a call to the method in 81ce0f0 and on a sample size of one, it allowed the two instances of test_linkcheck_request_headers on Py3.8 (two because we run the tests against two versions of docutils) to pass when they both failed on a previous version of the code (although I did add a merge-latest between the two).

@jayaddison jayaddison force-pushed the issue-11324-prep/linkcheck-tests-refactor-remove-threadshared-variables branch from 58d7b07 to 7099f3b Compare May 18, 2023 09:59
@jayaddison
Copy link
Contributor Author

Further suggestions and feedback welcome; otherwise I think that this should be ready for review (no further changes planned at the moment).

Copy link
Contributor

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

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

Looking good!

tests/test_build_linkcheck.py Show resolved Hide resolved
@jayaddison
Copy link
Contributor Author

Thank you @francoisfreitag!

tests/test_build_linkcheck.py Outdated Show resolved Hide resolved
tests/test_build_linkcheck.py Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member

AA-Turner commented Jul 20, 2023

More timeouts:

FAILED tests/test_build_linkcheck.py::test_defaults - assert 6 == 5
 +  where 6 = len(["links.rst:4: [broken] http://localhost:7777/#!bar: HTTPConnectionPool(host='localhost', port=7777): Read timed out. (read timeout=0.05)", "links.rst:7: [broken] http://localhost:7777#does-not-exist: Anchor 'does-not-exist' not found", 'links.rst:9: [broken] path/to/notfound: ', "links.rst:6: [broken] http://localhost:7777/#top: Anchor 'top' not found", 'links.rst:11: [broken] http://localhost:7777/image.png: 404 Client Error: Not Found for url: http://localhost:7777/image.png', 'links.rst:13: [broken] http://localhost:7777/image2.png: 404 Client Error: Not Found for url: http://localhost:7777/image2.png'])
 +    where ["links.rst:4: [broken] http://localhost:7777/#!bar: HTTPConnectionPool(host='localhost', port=7777): Read timed out. (read timeout=0.05)", "links.rst:7: [broken] http://localhost:7777#does-not-exist: Anchor 'does-not-exist' not found", 'links.rst:9: [broken] path/to/notfound: ', "links.rst:6: [broken] http://localhost:7777/#top: Anchor 'top' not found", 'links.rst:11: [broken] http://localhost:7777/image.png: 404 Client Error: Not Found for url: http://localhost:7777/image.png', 'links.rst:13: [broken] http://localhost:7777/image2.png: 404 Client Error: Not Found for url: http://localhost:7777/image2.png'] = <built-in method splitlines of str object at 0x55aa021f40d0>()
 +      where <built-in method splitlines of str object at 0x55aa021f40d0> = "links.rst:4: [broken] http://localhost:7777/#!bar: HTTPConnectionPool(host='localhost', port=7777): Read timed out. (read timeout=0.05)\nlinks.rst:7: [broken] http://localhost:7777#does-not-exist: Anchor 'does-not-exist' not found\nlinks.rst:9: [broken] path/to/notfound: \nlinks.rst:6: [broken] http://localhost:7777/#top: Anchor 'top' not found\nlinks.rst:11: [broken] http://localhost:7777/image.png: 404 Client Error: Not Found for url: http://localhost:7777/image.png\nlinks.rst:13: [broken] http://localhost:7777/image2.png: 404 Client Error: Not Found for url: http://localhost:7777/image2.png\n".splitlines

@AA-Turner AA-Turner merged commit bef7fc2 into sphinx-doc:master Jul 20, 2023
26 of 27 checks passed
@jayaddison
Copy link
Contributor Author

Thanks again @AA-Turner. Related to this and #11432: do you have any idea why it might be that the LaTeX-related continuous integration tests seem to be more susceptible to timeout failures?

@jayaddison jayaddison deleted the issue-11324-prep/linkcheck-tests-refactor-remove-threadshared-variables branch July 22, 2023 08:49
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky unit test: test_auth_header_uses_first_match
3 participants