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

Fix/1531 add unix domain socket #1629

Merged
merged 19 commits into from Feb 13, 2018

Conversation

3 participants
@jradtilbrook
Contributor

jradtilbrook commented Jul 4, 2017

This PR fixes #1531 - allowing MPD extension to listen on a unix socket as well as network socket. I've tried to comply with the development guide.
Any and all feedback appreciated.

@kingosticks

I also get an unhandled exception in the maximum_connections_exceeded case:

Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/gi/overrides/GLib.py", line 727, in <lambda>
    func_fdtransform = lambda _, cond, *data: callback(channel, cond, *data)
  File "/work/nsteel/projects/mopidy-dev/mopidy/mopidy/internal/network.py", line 122, in handle_connection
    self.reject_connection(sock, addr)
  File "/work/nsteel/projects/mopidy-dev/mopidy/mopidy/internal/network.py", line 144, in reject_connection
    logger.warning('Rejected connection from [%s]:%s', addr[0], addr[1])
IndexError: string index out of range

A test to catch that would be good.

@@ -35,17 +37,17 @@ def __init__(self, config, core):
super(MpdFrontend, self).__init__()
self.hostname = network.format_hostname(config['mpd']['hostname'])
self.port = config['mpd']['port']
self.port = config['mpd']['port'] if 'port' in config['mpd'] else None

This comment has been minimized.

@kingosticks

kingosticks Jul 4, 2017

Member

Do you need this change?

This comment has been minimized.

@jradtilbrook

jradtilbrook Jul 5, 2017

Contributor

This was to get around the optional port config value above

This comment has been minimized.

@kingosticks

kingosticks Jul 5, 2017

Member

Anything defined in the config's schema should always be defined in the dictionary (and set to None if not otherwise assigned a value by any other means). So I don't think you need this.

@@ -19,7 +19,7 @@ def get_default_config(self):
def get_config_schema(self):
schema = super(Extension, self).get_config_schema()
schema['hostname'] = config.Hostname()
schema['port'] = config.Port()
schema['port'] = config.Port(optional=True)

This comment has been minimized.

@kingosticks

kingosticks Jul 4, 2017

Member

If we have this optional then we need some tests to verify that's OK. I'm getting:

ERROR    2017-07-04 15:52:46,569 [20285:MpdFrontend-14] pykka
  Unhandled exception in MpdFrontend (urn:uuid:e87ce82e-f8b3-41ef-b9a1-5062480a5e92):
Traceback (most recent call last):
  File "/work/nsteel/projects/mopidy-dev/pykka/pykka/actor.py", line 192, in _actor_loop
    self.on_start()
  File "/work/nsteel/projects/mopidy-dev/mopidy/mopidy/mpd/actor.py", line 83, in on_start
    self.zeroconf_service.publish()
  File "/work/nsteel/projects/mopidy-dev/mopidy/mopidy/zeroconf.py", line 115, in publish
    self.domain, self.host, dbus.UInt16(self.port),
TypeError: int() argument must be a string or a number, not 'NoneType'

In this particular case, does it make sense to advertise on zeroconf when it's a unix socket?

This comment has been minimized.

@jradtilbrook

jradtilbrook Jul 5, 2017

Contributor

Yeah I suppose not. I'll add tests and remove the Zeroconf related stuff if using a unix socket.
Is there any notion of a config being one of two values? Eg.

schema['hostname'] = config.Hostname() or config.UnixSocket()

If not, should there be or should I just wrap both of these into the Hostname class like I have already?

This comment has been minimized.

@kingosticks

kingosticks Jul 5, 2017

Member

I don't believe the above snippet is supported. We do want to validate a Unix socket path in the exact same way we do any other path, it'd be nice to reuse that validation code. Ultimately, unix sockets are probably valid everywhere that a hostname would be so I'd say wrapping is ok. Any thoughts @adamcik, @jodal?

This comment has been minimized.

@kingosticks

kingosticks Oct 20, 2017

Member

So I tried out the following for Hostname.deserialize to try and reuse the existing Path validation code. What do you think?

    def deserialize(self, value, display=False):
        validators.validate_required(value, self._required)
        if not value.strip():
            return None
        socket_path = path.get_unix_socket_path(value)
        if socket_path is not None:
            return Path(not self._required).deserialize(socket_path)
        try:
            socket.getaddrinfo(value, None)
        except socket.error:
            raise ValueError('must be a resolveable hostname or valid IP')
        return value

I also moved that regex code to a path.get_unix_socket_path() helper since it occurs in the internal.network code too.

This comment has been minimized.

@jradtilbrook

jradtilbrook Oct 22, 2017

Contributor

Yeah okay that's much more modular. So just the regex match and group extraction is contained in that helper method? Then delegating path expansion and validation to the Path class?

@@ -25,7 +27,13 @@ def __init__(self, connection, config=None, core=None, uri_map=None):
session=self, config=config, core=core, uri_map=uri_map)
def on_start(self):
logger.info('New MPD connection from [%s]:%s', self.host, self.port)
if self.connection.sock.type == socket.AF_UNIX:

This comment has been minimized.

@kingosticks

kingosticks Jul 4, 2017

Member

As above. Also applies to on_line_received too (which has not been handled).

@@ -60,7 +62,15 @@ def _setup_server(self, config, core):
'MPD server startup failed: %s' %
encoding.locale_decode(error))
logger.info('MPD server running at [%s]:%s', self.hostname, self.port)
if server.server_socket.type == socket.AF_UNIX:
logger.info('MPD server running at %s', self.hostname)

This comment has been minimized.

@kingosticks

kingosticks Jul 4, 2017

Member

Maybe we could have a helper method to format the string for us since it occurs in a few places?

This comment has been minimized.

@jradtilbrook

jradtilbrook Jul 5, 2017

Contributor

I was thinking that but wanted to get the rest of this out for review as early as possible. I'll add a method for printing this sort of thing in a DRYer fashion

@@ -64,6 +64,7 @@ See :ref:`config` for general help on configuring Mopidy.
.. confval:: mpd/hostname
Which address the MPD server should bind to.
This can be a network address or a file path.

This comment has been minimized.

@kingosticks

kingosticks Jul 4, 2017

Member

Should be clearer that by file path we are talking about a UNIX domain socket.

This comment has been minimized.

@jradtilbrook

jradtilbrook Jul 5, 2017

Contributor

Sure, will do! To save another possible commit loop, how does this sound?
This can be a network address or the path to a Unix socket. Paths are expanded as in the Core configuration.

This comment has been minimized.

@kingosticks

kingosticks Jul 5, 2017

Member

I'd be happy with just the first sentence.

@@ -73,6 +74,8 @@ See :ref:`config` for general help on configuring Mopidy.
Listens on all IPv4 interfaces
``::``
Listens on all interfaces, both IPv4 and IPv6
``~/.mopidy/socket``
Listen on the socket in your configuration directory

This comment has been minimized.

@kingosticks

kingosticks Jul 4, 2017

Member

Maybe something more generic here? For example: /path/to/unix/socket.sock ? And adjust the wording appropriately.

This comment has been minimized.

@jradtilbrook

jradtilbrook Jul 5, 2017

Contributor

No worries, how about:
Listen on the Unix socket at the specified path

This comment has been minimized.

@kingosticks

kingosticks Jul 5, 2017

Member

Sounds good.

sock.setblocking.assert_called_once_with(False)
sock.bind.assert_called_once_with((sentinel.host, sentinel.port))
sock.bind.assert_called_once_with((str(sentinel.host), sentinel.port))

This comment has been minimized.

@kingosticks

kingosticks Jul 4, 2017

Member

Could probably do with a version of this for the unix case also since bind() is in both code paths.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jul 4, 2017

Since we are dealing with paths there might also be some unicode/bytes issues here, I don't recall what we are supposed to be doing w.r.t that these days.

@@ -258,6 +258,8 @@ def deserialize(self, value, display=False):
validators.validate_required(value, self._required)
if not value.strip():
return None
if re.search('/', value):
return path.expand_path(value)

This comment has been minimized.

@jradtilbrook

jradtilbrook Jul 5, 2017

Contributor

I was tossing up whether to add this here or to create another config type such as Socket or HostnameOrPath to encompass both Hostname config values as well as Paths. Let me know what you think

This comment has been minimized.

@kingosticks

kingosticks Jul 5, 2017

Member

Also, what do you think about requiring Unix domain sockets to start with 'unix:' like apache and nginx configs? This would remove the hard requirement for a slash when you are specifying foo.sock in the current working directory whilst still being unambiguous.

This comment has been minimized.

@jradtilbrook

jradtilbrook Jul 5, 2017

Contributor

That sounds good I'll do that

@@ -64,6 +64,7 @@ See :ref:`config` for general help on configuring Mopidy.
.. confval:: mpd/hostname
Which address the MPD server should bind to.
This can be a network address or a file path.

This comment has been minimized.

@jradtilbrook

jradtilbrook Jul 5, 2017

Contributor

Sure, will do! To save another possible commit loop, how does this sound?
This can be a network address or the path to a Unix socket. Paths are expanded as in the Core configuration.

@@ -73,6 +74,8 @@ See :ref:`config` for general help on configuring Mopidy.
Listens on all IPv4 interfaces
``::``
Listens on all interfaces, both IPv4 and IPv6
``~/.mopidy/socket``
Listen on the socket in your configuration directory

This comment has been minimized.

@jradtilbrook

jradtilbrook Jul 5, 2017

Contributor

No worries, how about:
Listen on the Unix socket at the specified path

@jradtilbrook

This comment has been minimized.

Contributor

jradtilbrook commented Aug 19, 2017

So wow, it took me a long time but I've finally gotten time to get back to this and implement the changes/fixes. Hopefully it looks all good now. Let me know what you think

@jradtilbrook

This comment has been minimized.

Contributor

jradtilbrook commented Oct 20, 2017

I believe I have made all the requested changes. I have also been running this for the last few months without any issues.
Any chance of getting this merged?

@kingosticks

This comment has been minimized.

Member

kingosticks commented Oct 20, 2017

Sorry I didn't get back to you on this but as chance would have it I actually started looking at it again just last night. I have a few thoughts on the server code following some debugging I did getting Mopidy (nearly) running on Windows, since socket.AF_UNIX is not defined on that platform. I also experimented with using the Path type within the Hostname validation which I think might be worthwhile. That work is on my home computer, I'll try to share it finish the review later.

@kingosticks

Sorry for the delay!

return socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
def format_socket_connection_string(sock):

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

Nitpicking here, but you could call this something like format_socket_name so it matches the other similar one below.

@@ -258,6 +258,9 @@ def deserialize(self, value, display=False):
validators.validate_required(value, self._required)
if not value.strip():
return None
socket_path = path.get_unix_socket_path(value)
if socket_path:

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

We should to do this if socket_path is not None so that anything starting with 'unix:' gets the Path treatment, even if it's an empty path (that way the user sees a more sensible error message).

This comment has been minimized.

@jradtilbrook

jradtilbrook Oct 23, 2017

Contributor

Fair enough. In my defence, I was trying to be more pythonic but you argument makes more sense

@@ -47,6 +48,13 @@ def get_or_create_file(file_path, mkdir=True, content=None):
return file_path
def get_unix_socket_path(socket_path):
match = re.search('^unix:(.*)', socket_path)
if match:

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

Again, nitpicking but maybe we should rejig this so the None case is a bit more explicit?

if not match:
    return None:
return match.group(1)
@@ -258,6 +258,9 @@ def deserialize(self, value, display=False):
validators.validate_required(value, self._required)
if not value.strip():
return None
socket_path = path.get_unix_socket_path(value)
if socket_path:
return Path(not self._required).deserialize(socket_path)

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

This loses the 'unix:' prefix so the only way to identify unix socket paths after this point is to check for isinstance(value, ExpandedPath). Alternatively you could just return 'unix:' + Path(...). I think both are reasonable but maybe the latter is more bulletproof?

def create_server_socket(self, host, port):
sock = create_socket()
socket_path = path.get_unix_socket_path(host)
if socket_path: # host is a path so use unix socket

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

is not None

@@ -140,7 +179,11 @@ class Connection(object):
def __init__(self, protocol, protocol_kwargs, sock, addr, timeout):
sock.setblocking(False)
self.host, self.port = addr[:2] # IPv6 has larger addr
if (sock.family == socket.AF_UNIX):

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

We could avoid bothering Connection with socket types if we handle this in Server.accept_connection(). Something like:

def accept_connection(self):
        try:
            sock, addr = self.server_socket.accept()
            if is_unix_socket(sock):
                addr = (sock.getsockname(), None)
            return sock, addr

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

is_unix_socket is helper where we could also check that socket has the AF_UNIX attribute (i.e. not on Windows).

logger.info('New MPD connection from [%s]:%s', self.host, self.port)
logger.info(
'New MPD connection from %s',
network.format_socket_connection_string(self.connection.sock))

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

What about defining a Connection.__str__ function all these are bit prettier and we don't expose the socket?

def on_start(self):
if self.zeroconf_name:
if (self.zeroconf_name and
self.server.server_socket.family != socket.AF_UNIX):

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

I'm not keen on exposing low level socket stuff here. What if we just check if self.port is None? We can call something in the constructor to set that based on the hostname e.g.

self.hostname, self.port = network.get_server_address(config['mpd']['hostname']), config['mpd']['port'])

This comment has been minimized.

@jradtilbrook

jradtilbrook Nov 4, 2017

Contributor

What about using the is_unix_socket helper from above? That makes more sense to me rather than checking that the port is None

This comment has been minimized.

@kingosticks

kingosticks Nov 4, 2017

Member

Not sure. I was thinking in the general case of if you don't have a port you don't want to advertise. But perhaps that isn't true? I'm. Not sure if zeroconf works outside of normal socket connections.

This comment has been minimized.

@kingosticks

kingosticks Nov 4, 2017

Member

Github seems to have swallowed this comment up. It's not a big deal either way.

@@ -42,3 +42,38 @@ def test_idle_hooked_up_correctly(event, expected):
assert not send_mock.call_args
else:
send_mock.assert_called_once_with(mock.ANY, expected)
@pytest.mark.parametrize("event,expected", [

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

If we really want this coverage you can just add extra hostname and port parameters to the previous test rather than duplicate all this code. Is this all just to test that making port optional is OK? Perhaps something more targeted would be better. And since zeroconf is None here this doesn't actually test the path that threw the exception in the first review.

This comment has been minimized.

@jradtilbrook

jradtilbrook Nov 4, 2017

Contributor

Oh yes I forgot about this one. Do you mean adding the hostname to the parameter decorator? So something like:

-@pytest.mark.parametrize("event,expected", [
-    (['track_playback_paused', 'tl_track', 'time_position'], None),
-    (['track_playback_resumed', 'tl_track', 'time_position'], None),
-    (['track_playback_started', 'tl_track'], None),
-    (['track_playback_ended', 'tl_track', 'time_position'], None),
-    (['playback_state_changed', 'old_state', 'new_state'], 'player'),
-    (['tracklist_changed'], 'playlist'),
-    (['playlists_loaded'], 'stored_playlist'),
-    (['playlist_changed', 'playlist'], 'stored_playlist'),
-    (['playlist_deleted', 'uri'], 'stored_playlist'),
-    (['options_changed'], 'options'),
-    (['volume_changed', 'volume'], 'mixer'),
-    (['mute_changed', 'mute'], 'output'),
-    (['seeked', 'time_position'], 'player'),
-    (['stream_title_changed', 'title'], 'playlist'),
+@pytest.mark.parametrize("event,expected,hostname", [
+    (['track_playback_paused', 'tl_track', 'time_position'], None, 'foobar'),
+    (['track_playback_resumed', 'tl_track', 'time_position'], None, 'foobar'),
+    (['track_playback_started', 'tl_track'], None, 'foobar'),
+    (['track_playback_ended', 'tl_track', 'time_position'], None, 'foobar'),
+    (['playback_state_changed', 'old_state', 'new_state'], 'player', 'foobar'),
+    (['tracklist_changed'], 'playlist', 'foobar'),
+    (['playlists_loaded'], 'stored_playlist', 'foobar'),
+    (['playlist_changed', 'playlist'], 'stored_playlist', 'foobar'),
+    (['playlist_deleted', 'uri'], 'stored_playlist', 'foobar'),
+    (['options_changed'], 'options', 'foobar'),
+    (['volume_changed', 'volume'], 'mixer', 'foobar'),
+    (['mute_changed', 'mute'], 'output', 'foobar'),
+    (['seeked', 'time_position'], 'player', 'foobar'),
+    (['stream_title_changed', 'title'], 'playlist', 'foobar'),
+
+    (['track_playback_paused', 'tl_track', 'time_position'], None,
+        'unix:foobar'),
+    (['track_playback_resumed', 'tl_track', 'time_position'], None,
+        'unix:foobar'),
+    (['track_playback_started', 'tl_track'], None, 'unix:foobar'),
+    (['track_playback_ended', 'tl_track', 'time_position'], None,
+        'unix:foobar'),
+    (['playback_state_changed', 'old_state', 'new_state'], 'player',
+        'unix:foobar'),
+    (['tracklist_changed'], 'playlist', 'unix:foobar'),
+    (['playlists_loaded'], 'stored_playlist', 'unix:foobar'),
+    (['playlist_changed', 'playlist'], 'stored_playlist', 'unix:foobar'),
+    (['playlist_deleted', 'uri'], 'stored_playlist', 'unix:foobar'),
+    (['options_changed'], 'options', 'unix:foobar'),
+    (['volume_changed', 'volume'], 'mixer', 'unix:foobar'),
+    (['mute_changed', 'mute'], 'output', 'unix:foobar'),
+    (['seeked', 'time_position'], 'player', 'unix:foobar'),
+    (['stream_title_changed', 'title'], 'playlist', 'unix:foobar'),
 ])
-def test_idle_hooked_up_correctly(event, expected):
-    config = {'mpd': {'hostname': 'foobar',
+def test_idle_hooked_up_correctly(event, expected, hostname):
+    config = {'mpd': {'hostname': hostname,
def test_correctly_no_match_socket_path(self):
self.assertIsNone(
path.get_unix_socket_path('127.0.0.1'),
None

This comment has been minimized.

@kingosticks

kingosticks Oct 22, 2017

Member

assertIsNone takes one argument. Doesn't it?!

@jradtilbrook

This comment has been minimized.

Contributor

jradtilbrook commented Nov 4, 2017

I think I've covered all your requested changes now. Whenever you have time again, let me know what you think.
In the meantime, the tests are failing because a new version of flake8 with E722 do not use bare except has arisen. I think its out of scope for the PR but I can open another to fix the error, then rebase/merge that in once its resolved

@kingosticks

If you can sort my last comment this looks brilliant. Like those tests you've added too. Thanks for all this effort.

@@ -179,13 +189,13 @@ class Connection(object):
def __init__(self, protocol, protocol_kwargs, sock, addr, timeout):
sock.setblocking(False)
if (sock.family == socket.AF_UNIX):
if is_unix_socket(sock):

This comment has been minimized.

@kingosticks

kingosticks Nov 4, 2017

Member

With the changes you just made to accept_connection you don't need this condition at all anymore.

This comment has been minimized.

@jradtilbrook

jradtilbrook Nov 4, 2017

Contributor

Do you mean:

-        if is_unix_socket(sock):
-            self.host = sock.getsockname()
-            self.port = None
-        else:
-            self.host, self.port = addr[:2]  # IPv6 has larger addr
+        self.host, self.port = addr[:2]  # IPv6 has larger addr

I think the extra verbosity of the current code makes more sense though. I'll wait to commit and push until you confirm you'd prefer it this way.

This comment has been minimized.

@kingosticks

kingosticks Nov 4, 2017

Member

I personally don't think it needs to worry about this extra special case since it fits in the general case

This comment has been minimized.

@jradtilbrook

jradtilbrook Nov 4, 2017

Contributor

Okay cool I'll push those changes

@kingosticks

This comment has been minimized.

Member

kingosticks commented Nov 4, 2017

Oh and I guess that rebase too

jradtilbrook added some commits Jul 4, 2017

mpd: add functionality for unix domain socket
Update log lines and clean up the socket file upon exit
mpd: modify Hostname config type and add test
The Hostname config type now supports a Unix socket path prefixed with
`unix:`
mpd: disable zeroconf if using unix socket
Also add test to for optional port setting
mpd: More verbose if statements and names
Also add test for removing socket path on shutdown
mpd: some cleaning up after review
Refactor duplicate test to better use parameterization.
Also remove redundant unix socket check.

@jradtilbrook jradtilbrook force-pushed the jradtilbrook:fix/1531-add-unix-domain-socket branch from 17ddb88 to 6992cb8 Nov 4, 2017

@kingosticks

Sorry, couple more thoughts today.

(['mute_changed', 'mute'], 'output'),
(['seeked', 'time_position'], 'player'),
(['stream_title_changed', 'title'], 'playlist'),
@pytest.mark.parametrize("event,expected,hostname", [

This comment has been minimized.

@kingosticks

kingosticks Nov 6, 2017

Member

I apologise for my earlier comment on this, it should have been clearer and just said this probably that isn't worth having. The idle stuff has nothing to do with what type of socket we have and this test doesn't even create one. If adding tests here we should concentrate on testing the changes i.e. the frontend can still start under the new config condition (port now optional), checking if starting zeroconf is skipped and server.stop() gets called.

This comment has been minimized.

@jradtilbrook

jradtilbrook Dec 18, 2017

Contributor

No worries. I have removed this test

sock.bind(socket_path)
else:
sock = create_tcp_socket()
sock.bind((host, port))

This comment has been minimized.

@kingosticks

kingosticks Nov 6, 2017

Member

Now that Port is optional, you get an un-handled exception from this if you run with mopidy -o mpd/hostname=:: -o mpd/port=. This is obviously a mistake in the user's config but we still need to handle it. It's shame it can't be done in the config validation where it belongs without having Port be non-optional again (which you could technically do even though it doesn't always make sense anymore..). What do you think?

This comment has been minimized.

@jradtilbrook

jradtilbrook Nov 6, 2017

Contributor

Sorry, I'm not following. Do you mean make the port setting non-optional again?

This comment has been minimized.

@kingosticks

kingosticks Nov 6, 2017

Member

That would be one option, the alternative is to explicitly error if the port is not a number when socket is not Unix.

This comment has been minimized.

@jradtilbrook

jradtilbrook Nov 8, 2017

Contributor

Yeah I think option two is better? Happy for this to raise an exception here if port is None/empty? I'm looking through all the exceptions and wondering which type to use though. Current thoughts are ValueError, ValidationError, or BackendError?

This comment has been minimized.

@jradtilbrook

jradtilbrook Dec 18, 2017

Contributor

I opted for the helper method from validation.check_integer which in-turn throws a ValidationError. Let me know if there is a more preferable way

jradtilbrook added some commits Nov 6, 2017

@jradtilbrook

This comment has been minimized.

Contributor

jradtilbrook commented Dec 18, 2017

I think the failing tests are due to changes updates to the dependent pip packages. Have you considered adding version constraints to the dev-requirements.txt file to reduce the chance of this happening? I could probably fix them here, but it's a little out of scope

@jradtilbrook

This comment has been minimized.

Contributor

jradtilbrook commented Feb 11, 2018

I've pushed changes that should fix the new flake8 violations but I'm still hesitant about fixing the other failing tests because its in code unrelated and untouched by this PR. Could you advise what to do? I'd like to get this PR off my pending list since it's been a long time coming 😉

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 11, 2018

Hey Yeh I've been meaning to sort it out. And yes let's get this done and dusted. I can't look at it today but I will do tomorrow evening.

@@ -19,17 +21,18 @@ def setUp(self): # noqa: N802
def test_init_calls_create_server_socket(self):
network.Server.__init__(
self.mock, sentinel.host, sentinel.port, sentinel.protocol)
self.mock, sentinel.host, 1234, sentinel.protocol)

This comment has been minimized.

@kingosticks

kingosticks Feb 12, 2018

Member

It's been so long.. was there a reason for replacing all the sentinel port values with 1234 in these tests?

This comment has been minimized.

@jradtilbrook

jradtilbrook Feb 13, 2018

Contributor

Yeah for me too. I believe it was because I added integer checking here as part of making the port optional but ensuring its an integer when specified

This comment has been minimized.

@jradtilbrook

jradtilbrook Feb 13, 2018

Contributor

I've pushed changes that use sentinel where possible. I think there are actually only three cases where it must be an integer for testing purposes

This comment has been minimized.

@kingosticks

kingosticks Feb 13, 2018

Member

That makes sense, thanks. I guess having validation down in this code is a bit unusual for us, I don't think we have anywhere else where the config setting's requirement is conditional on another bit of our config. For another day, I think.

@@ -25,6 +25,9 @@ def queue_send(self, data):
lines = (line for line in data.split('\n') if line)
self.response.extend(lines)
def getsockname(self):

This comment has been minimized.

@kingosticks

kingosticks Feb 12, 2018

Member

I can't see where this is used, still needed?

This comment has been minimized.

@jradtilbrook

jradtilbrook Feb 13, 2018

Contributor

Good point. Removed 😄

This comment has been minimized.

@kingosticks
@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 13, 2018

Looks good to me. Those unrelated tests are fixed with pinning pytest<3.3.0. Probably worth squashing these commits when I merge.

@jradtilbrook

This comment has been minimized.

Contributor

jradtilbrook commented Feb 13, 2018

Okay cool - I assumed it was a test versioning issue. Haha yeah for sure

@kingosticks kingosticks merged commit b4c98ec into mopidy:develop Feb 13, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 13, 2018

Thanks for all your work and patience. I've just realised our hostname validation is a bit messed up for the http server (it'll accept unsupported unix socket paths - oops!) but the best fix there is to just support them (#1660)

@jradtilbrook

This comment has been minimized.

Contributor

jradtilbrook commented Feb 13, 2018

No worries. Thanks for accepting.
I'm a little confused about what you mean by the fix but I'll carry this conversation over to the new issue

@jodal jodal added this to the v2.2 milestone Mar 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment