add the ZAPI request id on RequestError message #52
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
- Coverage 43.28% 42.93% -0.36%
==========================================
Files 10 10
Lines 365 368 +3
Branches 53 54 +1
==========================================
Hits 158 158
- Misses 205 208 +3
Partials 2 2
|
|
Tox is failling for 3.7. I suppose removing it would be apt. #53 |
tests/test_client.py
Outdated
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @mock.patch("zyte_api.aio.client._post_func") |
There was a problem hiding this comment.
As for the test, it could be nice to move the mockserver implementation from scrapy-zyte-api to python-zyte-api (see https://github.com/scrapy-plugins/scrapy-zyte-api/blob/137050522c14e146ed6bb28acc31dfde3fbdb6dd/tests/mockserver.py#L55); this would allow writing tests without using mocks.
Please let me know if you want to give it a try, or if you prefer for it to be handled separately; I'm fine with merging without it.
There was a problem hiding this comment.
I think this won't be needed with the latest changes since it's simpler now and doesn't need mocks.
| @@ -23,4 +26,5 @@ def parsed(self): | |||
|
|
|||
| def __str__(self): | |||
| return f"RequestError: {self.status}, message={self.message}, " \ | |||
There was a problem hiding this comment.
it seems request_id is going to be included twice here: in self.message, and in self.request_id as well.
tests/test_client.py
Outdated
|
|
||
| @pytest.mark.asyncio | ||
| @mock.patch("zyte_api.aio.client._post_func") | ||
| async def test_request_raw_error(mock_post): |
There was a problem hiding this comment.
It seems we can check the complete str representation of the error in this test, maybe in addition to the existing checks. Do you see any downsides to it?
There was a problem hiding this comment.
You're right. 👍
What do you think about not including the request_id in the RequestError.message and then we update https://github.com/scrapy-plugins/scrapy-zyte-api/blob/main/scrapy_zyte_api/handler.py#L223-L224 to ensure request_id is logged via er.request_id?
There was a problem hiding this comment.
We'd need to ensure to bump min python-zyte-api version in this case.
There was a problem hiding this comment.
Awesome. This makes it much simpler. e288c75
Will have another PR for python-zyte-api after this is merged.
Alternative to scrapy-plugins/scrapy-zyte-api#142