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

test_linkcheck_allowed_redirects: enable HTTP HEAD request support #11323

Conversation

jayaddison
Copy link
Contributor

This is intended to reduce the number of HTTP requests within this particular test to closer-to-the-norm compared to other tests within the test_build_linkcheck.py module; this test has been failing intermittently and it seems possible that timeouts due to HTTP roundtrip traffic are a contributing factor.

Feature or Bugfix

  • Bugfix (maybe)

Purpose

  • Intended to reduce flakiness in the test_linkcheck_allowed_redirects test -- or to rule out another potential source of that problem
  • Also intended to avoid increasing the linkcheck request timeout thresholds for this (and other) tests

Detail

  • Should reduce the number of requests made by the unit test to the test HTTP server by half. The server is tiny and basic, and I don't exactly understand why reducing the total request count should have any effect on the duration of the individual requests - but it's an idea to explore, and the logic expressed by the test should remain valid.

Relates

…ps by enabling support for HEAD requests

This is intended to reduce the number of HTTP requests within this particular test to closer-to-the-norm compared to other tests within the test_build_linkcheck.py module; this test has been failing intermittently and it seems possible that timeouts due to HTTP roundtrip traffic are a contributing factor.
@jayaddison
Copy link
Contributor Author

Thanks for the review @francoisfreitag. After doing more testing, I'm not really convinced that this would fix the test flakiness.

I'd be OK with it being merged.. but if there's no particular reason for it, then I'd probably tend towards closing it unmerged, since otherwise it'll pollute the commit history.

Either way, I'll run a few more tests in jayaddison#1 and perhaps that'll discover more -- or perhaps not :) That has been my Friday, 2023-04-14..

@jayaddison
Copy link
Contributor Author

(the approach I prefer now, since I couldn't find any particular reason for a performance regression, is simply to increase the timeout thresholds slightly further in #11326)

@francoisfreitag
Copy link
Contributor

My understanding of this change : allowing HEAD saves the linkcheck behavior of retrying with GET after HEAD failed, which makes the test faster. Since the method is not relevant for the test, might as well use HEAD.

@jayaddison
Copy link
Contributor Author

That is true that this change should provide a very, very small runtime improvement for the tests, yes.

@jayaddison jayaddison marked this pull request as ready for review April 14, 2023 15:51
@jayaddison
Copy link
Contributor Author

Nothing about the experimental evaluation data collected in jayaddison#1 disproved this fix -- that is, no test failures occurred in the linkcheck tests with this change applied -- but I still don't think that the value (reduced test runtime -- extremely marginal) is worth the additional investigation and history overhead.

I'll close this pull request -- if someone later feels that this change is particularly valuable (perhaps it turns out that actually it does fix the problem comprehensively, somehow), then as long as there's supporting evidence for that, I've no problem with it being merged.

Roughly speaking: I don't mind being wrong, but I'd like to understand why. In this case I feel like I'm wrong, but the data hasn't yet confirmed it. So I'll choose not to proceed here.

@jayaddison jayaddison closed this Apr 14, 2023
@jayaddison jayaddison deleted the issue-11299/reduce-flaky-test-request-roundtrips branch April 14, 2023 22:21
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 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.

None yet

2 participants