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

Use mock requests in unit tests #169

Closed
JWCook opened this issue Feb 25, 2021 · 8 comments · Fixed by #189, #208 or #211
Closed

Use mock requests in unit tests #169

JWCook opened this issue Feb 25, 2021 · 8 comments · Fixed by #189, #208 or #211
Labels
bug logistics CI builds, project config, refactoring, and other logistical details tests Unit and integration tests
Milestone

Comments

@JWCook
Copy link
Member

JWCook commented Feb 25, 2021

It looks like the last time a build was run was about 1.5 years ago. After dusting it off running it again, a few unit tests are broken: https://travis-ci.org/github/reclosedev/requests-cache/jobs/760375526

Since no significant code changes have been made since then, it could be due to a change in one or more dependencies.

@JWCook JWCook added the bug label Feb 25, 2021
@JWCook JWCook added this to the v0.6 milestone Feb 25, 2021
@JWCook
Copy link
Member Author

JWCook commented Feb 25, 2021

Update: it appears that all 4 of the failing tests are ones that call https://httpbin.org/relative-redirect. All of the redirect endpoints on httpbin.org appear to be returning 404s now. See: postmanlabs/httpbin#617

The best solution to this is probably to mock out responses instead of depending on an external service (which makes these integration tests, not unit tests).

@libbkmz
Copy link
Contributor

libbkmz commented Feb 26, 2021

Working on the PoC for this with pook library. What your thoughts on this? Any ideas?
I hope, I will send a PR for you to have a look at soon.

@JWCook
Copy link
Member Author

JWCook commented Feb 26, 2021

Yes, a PR for that would be great! Normally I would use requests-mock for this, but currently that's not compatible with requests-cache because they both try to patch requests.Session (also mentioned in issue #87). If pook has a different mocking mechanism that's compatible with requests-cache, that would be perfect.

@libbkmz
Copy link
Contributor

libbkmz commented Feb 26, 2021

Yeah, it looks good and working, however, this issue is frustrating me: h2non/pook#72 (comment)
So, I can't mix tests with and without pook now =\

@JWCook
Copy link
Member Author

JWCook commented Feb 26, 2021

Hm, what would you think about just mocking out all requests, then? I don't think unit tests should be making real HTTP requests anyway, so I would consider that an improvement.

@JWCook
Copy link
Member Author

JWCook commented Mar 16, 2021

Fyi, I got some tests working using requests-mock. I added some examples here. I will plan on updating the rest of the unit tests to use requests-mock soon.

@JWCook JWCook changed the title Fix broken unit tests Use mock requests in unit tests Mar 16, 2021
@JWCook
Copy link
Member Author

JWCook commented Mar 21, 2021

Using the httpbin Docker image may be a useful option for integration tests:
#189 (comment)

JWCook added a commit that referenced this issue Mar 22, 2021
Refactor cached response info & initialization into a separate class

Closes #99, #148, #186 , #187, #188, and does some of the work for #169 and #184.

## Summary
The goal of this PR is to reduce code complexity, and hopefully make things a little easier for others who want to contribute. Some of the logic around getting and saving responses had gotten a bit complicated as more features have been added, mainly in:
* `CachedSession.request()`
* `CachedSession.send()`
* `BaseCache.set_response()` (and associated helper methods)
* `BaseCache.get_response()` (and associated helper methods)

I've consolidated most of the logic around expiration, serializiation compatibility, and other response object handling into a separate class, `CachedResponse`. This simplifies the above methods, removes most of the code duplication, and fixes a few other issues. According to [radon](https://radon.readthedocs.io/), all classes/modules/methods except two have an 'A' maintainability rating (CC <= 5).

## Changes
**Short version:** Most of the important bits are here: [requests_cache/response.py](https://github.com/reclosedev/requests-cache/blob/b0f2c132fc320c9b1e32e839add63240b2805bbb/requests_cache/response.py)

### Serialization/Deserialization
* Response creation time and expiration time are stored in `CachedResponse`, so the timestamp from the `(response, timestamp)` tuple is no longer needed
* `CachedResponse.is_expired` can be used to indicate if an old response was returned as the result of an error with `old_data_on_error=True` (#99)
* Replace `_RawStore`  with `CachedHTTPResponse` class to wrap raw responses, and:
    * Maintain support for streaming requests (#68)
    * Improve handling for generator usage
    * Add support for use with `pandas.read_csv()` and similar readers (#148)
    * Add support for use as a context manager (#148)
    * Add support for `decode_content` param
* Fix streaming requests when used with memory backend (#188)
* Verified that `PreparedRequest.body` is always encoded in utf-8, so no need to detect encoding
    *  Re: [todo note here](https://github.com/reclosedev/requests-cache/blob/08886efe1841aae9c6a5e133008b69296d23b8b9/requests_cache/backends/base.py#L238); see [requests.PreparedRequest.prepare_body()](https://github.com/psf/requests/blob/54336568789e0ec41f70c800a96384e4fac58dd9/requests/models.py#L455)

### Expiration
* Add optional `expire_after` param to `CachedSession.remove_old_responses()`
* Wrap temporary `_request_expire_after` in a contextmanager
* Remove `expires_before` param from remove_old_entries, and always use the current time
* Remove `relative_to` param from `CachedSession._determine_expiration_datetime` for unit testing and patch `datetime.now` in unit tests instead
* Rename `response.expire_after` and `response.cache_date` to `expires` and `created_at`, respectively, based on browser caching

### Docs
* Add type annotations to public methods in `CachedSession`, `CachedResponse`, and `BaseCache`
* Add some more docstrings and code comments
* Add intersphinx links for `urllib` classes & methods
* Update user guide for using requests-cache with `requests_mock.Adapter`

### Tests
* Add an update tests for all new and changed code, and add coverage for additional edge cases
* Add fixture for combining requests-cache with requests-mock, using a temporary SQLite db in `/tmp`
* Refactor all tests in `test_cache` as pytest-style functions instead of `unittest.TestCase` methods
* Update most tests to use requests-mock instead of using httpbin.org (see #169)

### Misc
* Split some of the largest functions into multiple smaller functions
* Make use of dict ordering from python3.6+ in _normalize_parameters()
* Fix linting issues raised by flake8

## Backwards-compatibility
These changes don't break compatibility with code using requests-cache <= 0.5.2, but they aren't compatible with previously cached data due to the different serialization format. Retrieving a previously cached response will fail quietly and simply fetch and cache a new response. Since the docs already warn about this potentially happening with new releases, I don't think this is a problem.
@JWCook JWCook reopened this Mar 22, 2021
@JWCook
Copy link
Member Author

JWCook commented Mar 30, 2021

There are just a few items left for this issue, marked with TODOs in the test modules.

@JWCook JWCook added logistics CI builds, project config, refactoring, and other logistical details and removed help-wanted labels Apr 1, 2021
@JWCook JWCook added the tests Unit and integration tests label Apr 1, 2021
@JWCook JWCook linked a pull request Apr 1, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug logistics CI builds, project config, refactoring, and other logistical details tests Unit and integration tests
Projects
None yet
2 participants