Allow sanic test client to bind to a random port #1376
Conversation
|
Hi Daniel - flake8 is fussing because of your line length. Can you fix so the checks will pass? |
Codecov Report
@@ Coverage Diff @@
## master #1376 +/- ##
=========================================
+ Coverage 91.31% 91.4% +0.09%
=========================================
Files 18 18
Lines 1773 1781 +8
Branches 336 337 +1
=========================================
+ Hits 1619 1628 +9
Misses 130 130
+ Partials 24 23 -1
Continue to review full report at Codecov.
|
|
Whoops, missed that. Should be all fixed up now. |
docs/sanic/testing.md
Outdated
| ## Using a random port | ||
|
|
||
| If you need to test using a random port instead of the default with | ||
| `SanicTestClient`, you can do so by specifying `port=None` |
There was a problem hiding this comment.
From a general usability standpoint, wouldn't it be good to reverse the behavior?
i.e. the port param defaults to None and picks a random port, however, you can specify a port and use for running test cases. IMHO, this would be more readable and easy to understand.
As an additional option, we can even add an ENV variable TEST_CLIENT_PORT and use that to provide a much dynamic port assignment.
There was a problem hiding this comment.
I like this idea but unless other people pipe up on it I think adding this functionality and changing the default can be addressed separately
There was a problem hiding this comment.
Yeah I think changing the default functionality should come with a different PR
sanic/testing.py
Outdated
| self.app = app | ||
| self.port = port | ||
|
|
||
| async def _local_request(self, method, uri, cookies=None, *args, **kwargs): | ||
| async def _local_request( | ||
| self, method, uri, sock=None, cookies=None, *args, **kwargs |
There was a problem hiding this comment.
My main issue here is that this function signature now has duplicate information, it seems like if we have all the information we need in the caller to infer the url then it should be the callers responsibility for doing so.The easiest way to do that would be self.port = sock.getsockname()[1] after 92, lazily setting the port should be fine but it may be worth thinking about. Another option though is we have a short circuit on 23 for uri's that look like urls, we could similarly do uri = "http://{sock[0]}:{sock[1]}{uri}".format(sock=sock.getaddrname(), uri=uri) at after 92 as well.
There was a problem hiding this comment.
I like the idea, but setting self.port causes multiple requests to bind the same port, which may fail if the OS assigned the random port to something else between requests. I think it would be better to set self.sock to None in __init__ and set and unset it from _collect_request() below.
There was a problem hiding this comment.
we could similarly do
uri = "http://{sock[0]}:{sock[1]}{uri}".format(sock=sock.getaddrname(), uri=uri)at after 92 as well.
I ended up going with that, more or less
docs/sanic/testing.md
Outdated
| ## Using a random port | ||
|
|
||
| If you need to test using a random port instead of the default with | ||
| `SanicTestClient`, you can do so by specifying `port=None` |
There was a problem hiding this comment.
I like this idea but unless other people pipe up on it I think adding this functionality and changing the default can be addressed separately
|
@relud Are you still pursuing this one? As for the question of changing the functionality, or whether there should be something separate ( |
|
@ahopkins yes, I'm still pursuing this, I just haven't had time to @abuckenheimer's request because of a very busy december. I'm planning to add the requested test and documentation next week after Tuesday. |
|
🤘 Happy New Year 🥳 |
|
Vote for PR, feature crashes our CI pipelines too. |
|
I'm moving to 19.6, hopefully we can get the changes requested implemented in the PR by then. @relud if you do get this done before 10-Mar ping me directly so I can get people to review and I'll merge it if the reviewers are satisifed. |
|
@sjsadowski I believe this has now been updated to address reviews. |
seemethere
added
needs review
I like to run tests with detox, but using
sanic.Sanic.test_clientcauses issues because of multiple servers trying to simultaneously bind tosanic.testing.PORT.This solves that problem by having
SanicTestClientusesocketto bind to a random port whenport=None.