Switch httpretty for responses #1046
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
This was a quick experiment to see how good ChatGPT is at this kind of replacement. It did OK. It introduced its own bug, which it then fixed when prompted, making:
into:
|
StevenMaude
commented
Dec 11, 2025
tests/conftest.py
Outdated
| yield _httpretty | ||
| _httpretty.disable() | ||
| def http_responses(): | ||
| with responses.RequestsMock(assert_all_requests_are_fired=False) as rsps: |
Contributor
Author
There was a problem hiding this comment.
thought: The override on assert_all_requests_are_fired is required because not all the mocked responses are used for some tests:
================================================================= short test summary info ==================================================================
ERROR tests/reports/test_job_server.py::test_get_published_html_from_job_server - AssertionError: Not all requests have been executed [('GET', 'https://jobs.opensafely.org/org/project/workspace/published/file_id/')]
ERROR tests/reports/test_job_server.py::test_last_updated - AssertionError: Not all requests have been executed [('GET', 'https://jobs.opensafely.org/org/project/workspace/published/file_id/')]
ERROR tests/reports/test_job_server.py::test_report_last_updated_after_fetching - AssertionError: Not all requests have been executed [('GET', 'https://jobs.opensafely.org/org/project/workspace/published/file_id/')]
ERROR tests/reports/test_models.py::test_report_uses_github - AssertionError: Not all requests have been executed [('GET', 'http://example.com/')]
ERROR tests/reports/test_models.py::test_report_with_published_job_server_url_adds_trailing_slash - AssertionError: Not all requests have been executed [('HEAD', 'http://example.com/published/foo')]
========================================================= 98 passed, 1 warning, 5 errors in 19.55s =========================================================
It may be one or more of:
- there are responses added because of misunderstandings of the author at the time
- the behaviour has changed subsequently
- the tests is not really testing what was intended
Contributor
Author
There was a problem hiding this comment.
thought: I've fixed these up for now
- Some responses were either mistakenly added or the behaviour has since changed.
- Others weren't being retrieved because of caching.
We may want to rethink how we use the caching in tests, or ensure that the caches are created per-test, instead of being a global state.
* `test_report_uses_github` only checks for a URL stored on the report, not that the URL is accessed. *
This is a little bit messy, but some tests were not actually using the mocked HTTP responses provided in tests. `JobServerReport`, by default, uses a `requests_cache` session that stores HTTP responses. This, by default, does not have a default expiry time for requests, as `expires_after` is set to `-1`. Some of the tests were therefore using the cached responses on `JobServerReport(report).get_html()`, which is probably not was intended: * the expected value was being provided anyway, with a response that used it, so the intent was that the response should populate the resulting data value * it would mean that the behaviour of tests could be influenced by the ordering of other tests (the result gets cached and then reused; and sharing of state this way seems like it can create or mask other problems) This was masked by all the expected HTML content being the same here, and there's an argument that perhaps the content should be the name of the test function or something else unique. Alternative fixes: * name the cache and therefore have a unique cache for each test (however, this is currently set as a class attribute, configured by environment variable; it would be better if we could also specify it when creating the object, maybe) * maybe make the caching less aggressive by default, and use a frozen time to expire the cache.
We can do this now, having fixed the tests.
StevenMaude
force-pushed
the
steve/replace-httpretty-with-responses
branch
from
de2780a to
5aea67d
Compare
Contributor
Author
|
thought: Alice used mocket as a replacement in a similar task earlier this year. I don't know too much about
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1014.