Fix treq.request() argument handling#431
Conversation
adiroiban
left a comment
adiroiban
left a comment
There was a problem hiding this comment.
Looks good. Thanks!
Only very minor comments.
| _S = Union[bytes, str] | ||
|
|
||
| _URLType = Union[ | ||
| _SomeURL = Union[ |
There was a problem hiding this comment.
Maybe call it _TreqURL ... some seems strange to me
| for func in ( | ||
| treq.head, | ||
| treq.get, | ||
| treq.post, | ||
| treq.put, | ||
| treq.patch, | ||
| treq.delete, | ||
| ): |
There was a problem hiding this comment.
Maybe also assert the HTTP method used for each function
| for func in ( | |
| treq.head, | |
| treq.get, | |
| treq.post, | |
| treq.put, | |
| treq.patch, | |
| treq.delete, | |
| ): | |
| for func, method in ( | |
| (treq.head, 'HEAD'), | |
| (treq.get, 'GET'), | |
| (treq.post, 'POST'), | |
| (treq.put, 'PUT'), | |
| (treq.patch, 'PATCH'), | |
| (treq.delete, 'DELETE'), | |
| ): |
| self.requests += 1 | ||
| return defer.Deferred() | ||
| self.assertNoResult(d) | ||
| self.assertEqual(1, counter_agent.requests) |
There was a problem hiding this comment.
Not sure if the CounterAgent is good enough...
I was thinking to make sure that each function is associated
| self.assertEqual(1, counter_agent.requests) | |
| self.assertEqual([(method, "https://www.example.org/")], counter_agent.requests) |
There was a problem hiding this comment.
Sounds like a job for agent_spy()!
There was a problem hiding this comment.
True.
I am using this code for more than 10 years .... it's not a spy, but a rigged agent.
I am not 100% happy with it, but it does the job...
I would like to see a tighter integration with unittest.Test case, so for example when there is an assertion error, the agent will return a bad request, but will record a failure to the testcase
https://gist.github.com/adiroiban/f90582e01ff430a0d4ebe0f3a604cf0f
twm
left a comment
twm
left a comment
There was a problem hiding this comment.
Thank you for the review @adiroiban!
2 participants
Following up on #401, this fixes some bugs in the parameter handling of
treq.request(). This addresses that hole in the test suite.I still need to:
IndexableCookieJardocsRequestsCookieJar_HeadersType(as noted inline)I'm putting this PR up now just to be clear that we're not ready to release.