-
Notifications
You must be signed in to change notification settings - Fork 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
Update reqwest to v0.10 #892
Conversation
linux-stable on CI appears to be exhibiting an issue that I've been encountering locally (macOS 10.15.2), where some I haven't been able to figure out what the cause of this is, so I can't say whether it's something about how I've implemented these changes (or something about reqwest 0.10) or an issue with the remote servers (e.g., rate limiting from too many requests in a short period of time). It would probably be better to use a website under your control, so we can rule out remote issues. If it's possible to do these tests locally (e.g., using local HTML files) and avoid going out over the network altogether, that would be ideal. |
Maybe we can ping the getzola site instead of google. If we can easily mock reqwest then it's better to not have any network calls at all. |
This commit originally used the getzola.org docs Installation page in place of the help.github.com URLs (in an attempt to resolve this issue) but sometimes the test would still hang. It looks like mockito can be used to create HTTP mocks for testing, so I'll see about reworking these tests later when I get a chance, to see if that helps. If it's an issue with this implementation or reqwest 0.10, then this will at least make that more apparent. For what it's worth, I'm using the blocking client here because the non-blocking client's |
Using HTTP mocks (via Mockito) seems to provide consistent tests (no hanging), so I'll create a separate PR once I've finished converting tests that use HTTP requests (edit: now up at #898). Once that's merged, I'll rebase this PR and hopefully that will sort out this test issue for good. The other thing that needs to be addressed is the linux-1.35 (Rust 1.36.0) build failure.
This doesn't seem to fail on Rust stable, so if we want to keep this particular test, we'll likely want to use a newer version of Rust (and maybe rename |
411dc56
to
ee6411f
Compare
For what it's worth, this only failed on CI this time around because the This is the same intermittent issue that we encountered before but this test is harder to HTTP mock, since the links are in the content. Basically, the I've tried to come up with a solution to this but I haven't been able to figure out a way to get the
I've banged my head against this for a bit, so I'm going to shelve it for the moment. If you figure out a way to mock this test, I would definitely welcome it. |
ee6411f
to
e45cf5c
Compare
reqwest v0.10 was released earlier today, so I've updated this PR and marked it ready for review. The only thing to note is that the aforementioned issue with the |
Hmm unless we can mock things globally I don't know if it's possible :/ I've created #908 to not forget it Thanks a lot for the work! |
This is a draft PR to update reqwest to v0.10, which uses hyper 0.13. It's necessary to merge this before we can merge a PR to migrate the serve command to use hyper (since the hyper versions need to align).
reqwest v0.10 is not yet released but it should be coming soon. The current commit uses the master branch of the reqwest repo, since the latest alpha release uses alpha versions of hyper and tokio. I don't think anything notable will change between now and release, so it should be easy to wrap up this PR when the time comes.
This commit also updates some URLs in the link_checker tests which ended up redirecting (e.g., https://google.com -> https://www.google.com).
Past that, reqwest has recently added back support for rustls (via hyper-rustls). If you have any interest in using rustls, I can easily modify the Cargo.toml files to use it (rather than native-tls).
Let me know if you find anything that could be improved or otherwise needs to be changed.