Fixes #59: Test server uses an ephemeral port instead of binding to 8282#65
Fixes #59: Test server uses an ephemeral port instead of binding to 8282#65RobRuana merged 2 commits intomagfest:masterfrom
Conversation
| # Ideally we could tell cherrypy to listen on port 0, and then inspect | ||
| # it to determine what port it's using, but cherrypy doesn't support that | ||
| # yet (other parts of cherrypy will try to use the port defined in | ||
| # 'cherrypy.server.socket_port' and end up failing on startup). |
There was a problem hiding this comment.
Great comment! However, as a style note, anytime you have a giant comment right at the beginning of a function, it should probably be a triple-quoted docstring.
|
|
||
|
|
||
| available_port = get_available_port() | ||
| config['cherrypy']['server.socket_port'] = available_port |
There was a problem hiding this comment.
Just curious, why are you updating config in two ways here?
There was a problem hiding this comment.
By the time this code is executed, we'll have already populated cherrypy.config with what's in our config file, so the configuration will already be living in two places, each of which we need to update.
There was a problem hiding this comment.
I thought that might be the case. I guess we then still use that value in our ConfigObj elsewhere?
There was a problem hiding this comment.
I'm not sure of all the places it's being used, but I didn't want another developer in the future to try using it and wonder why it's not working. I wanted to make sure that config['cherrypy']['server.socket_port'] always contains the value that we'd expect (principle of least astonishment).
|
+1 other than the one style comment about using a docstring rather than a series of repeated |
4 participants
No description provided.