Replace `requests` with `httpx`

[sarayourfriend]

Fixes #2039

This was surprisingly more straightforward than I anticipated, but there are a few quirks worth explaining in the PR, which I'll leave as inline comments.

This PR definitely supersedes #2040, so I will close that PR in favour of this one. In which case, this also fixes #1960.

PR Checklist:

• [x] All new features have been tested
• [x] All new features have been documented
• [x] I have read the CONTRIBUTING.md file
• [x] I will abide by the code of conduct

[sarayourfriend]

Hmmm, I'm not understanding where the coverage issue is coming from. When I run the test locally I can reproduce the same issue with 223->230... but if I put a breakpoint, the TooManyRedirects branch is definitely exercised in the test that mocks that exception (test_get_connection_error).

Maybe I'm misunderstanding what 223->230 means in this failure :thinking:

[sarayourfriend]

Wow - they weren't kidding when they said that the httpx API is mostly requests-compatible... I'm surprised it's this compatible.

It's pretty nice!

Regarding the coverage failure - the case it's reporting is the "bucket else" - when the exception type isn't any of the three cases that have been explicitly reported. I'm not sure if it's actually possible to have an exception other than those three types; but regardless, there's no way for coverage to do that reasoning because exceptions aren't type described by Python code.

Aha! Thank you for the explanation.

The isinstance check order does not matter in this case, so I've pushed a change to match your suggestion with TransportError as the default else case.

---

As an aside, if httpx introduces a fourth kind of RequestError, that else branch starts to handle more than just the TransportError. Ideally, the check would be exhaustive, and report when the technically possible but practically (currently) impossible case occurs. Is the better thing to do to add an else that re-raises the unknown error type instead of swallowing it into the logs, or transforms into a Briefcase error that explicitly indicates an unknown error occurred and to file a bug report with logs attached? Not sure how defensive you prefer to be in these kinds of situations. The stakes in this particular case are pretty low, as are the chances it ever matters, and the error is generic enough that a user might check the logs anyway, so truly exhaustive checks is maybe going overboard.

[sarayourfriend]

Looks like the failing test found an unhandled edge case: https://github.com/beeware/briefcase/actions/runs/11411470307/job/31756731325?pr=2041#step:27:511

It should be fixed with this patch, I'll push that, but only after revisiting the tests to figure out how to accurately exercise that case:

diff --git a/src/briefcase/integrations/file.py b/src/briefcase/integrations/file.py
index 81922290..002bd4f2 100644
--- a/src/briefcase/integrations/file.py
+++ b/src/briefcase/integrations/file.py
@@ -256,6 +256,7 @@ class File(Tool):
             with temp_file:
                 total = response.headers.get("content-length")
                 if total is None:
+                    response.read()
                     temp_file.write(response.content)
                 else:
                     progress_bar = self.tools.input.progress_bar()

The current tests don't mock httpx.Response correctly, so never catch this case. They mock response.content directly with bytes, but it's a property in the real httpx.Response class. This gets to a limitation of mocking httpx in this way that I was a little worried about when refactoring, and which pytest-httpx or pook might be better suited to handle, they let the testing code get away from mocking the request/response cycle and act more like integration tests (but eliminating the outbound network requests).

However, a less invasive change would be to use a real response object and only mock (with wraps) the particular methods that need asserting on. I'll try to make that work.

[sarayourfriend]

Okay, https://github.com/beeware/briefcase/pull/2041/commits/5ffe0f13c087892633b5e763e1b5af021e9195a9 refactors one of the file__download tests to reduce the extent of mocking and expose the bug. The following commit, https://github.com/beeware/briefcase/pull/2041/commits/568644d7371ad0024e73db00e7eeefe6228649d3, addresses the bug.

I'm going to convert this to a draft because I'd like to refactor the rest of the tests in test_File__download.py to match the more thinly mocked approach of the test I refactored in 5ffe0f13c087892633b5e763e1b5af021e9195a9, and make sure to look for other tests that would benefit from the same treatment with respect to HTTP client mocking.

To clarify what I said before about pook and pytest-httpx, they significantly improve the ergonomics of writing tests like this. Compare the setup of the refactored test in 5ffe0f13c087892633b5e763e1b5af021e9195a9 to the pook version, (which is very similar to the pytest-httpx but slightly more comprehensive in its requirements by default):

# one shot
pook.get("https://example.com/support?useful=Yes").reply(200).body(b"all content")

# chunked
pook.get("https://example.com/support?useful=Yes").reply(200).body([b"chunk 1;", b"chunk 2;", b"chunk 3;"], chunked=True)

Briefcase's usage is so minimal, that it doesn't feel worth pulling in a test dependency for just this test_File__download.py suite, but something to keep in mind if the need to mock HTTP request/response cycles grows.

Anyway, I'll work on the rest of this later this weekend or early next week :slightly_smiling_face:

[freakboy3742]

Not sure how defensive you prefer to be in these kinds of situations. The stakes in this particular case are pretty low, as are the chances it ever matters, and the error is generic enough that a user might check the logs anyway, so truly exhaustive checks is maybe going overboard.

In this case, the error message in the default case is already framed in a "maybe...?", so if the eventuality you've flagged ever happens, the error won't be wrong, just not as helpful as it could be under that specific failure mode, and we can easily adapt. If the error message pointed at a specific cause and mitigation, I'd probably prefer explicit and distinct bucket handling.

Briefcase's usage is so minimal, that it doesn't feel worth pulling in a test dependency for just this test_File__download.py suite, but something to keep in mind if the need to mock HTTP request/response cycles grows.

Good call. I'm not opposed to introducing a dependency if it's going to radically improve testing; but given we've already got 98% of the testing of downloads that we need without the dependency, and we've pretty much hit the limit of the downloading that Briefcase will need to do, if we can live without the extra dependency, that would be my preference for now.