Incoming websocket subscriptions to remote services should work

[EliAndrewC]

Now that #8 has been implemented and #28 is more than half done, it would be relatively trivial to address the following use case:

As a developer writing a Sideboard plugin, I want to write Javascript code which subscribes to a service call without needing to know or care whether that service is local or remote. In other words, if I'm writing a service which runs at example.com and subscribe to the foo.bar method, that subscription should be valid even if the foo service is configured as a remote service at example.org.

This requires a few relatively small changes to the websocket server code. Right now we simply say "oh, you want foo.bar so let me get the foo service and then call getattr to get the bar method off it and then call that with the arguments you provided." After this change, that will be slightly more complicated:

• get the foo service and see whether it's a local or remote service
• if it's a local service OR if this is call that does not expect a subscription (i.e. it provides a callback but not a client field), do exactly what we do now
• if it's a remote service with a provided client field, get the underlying websocket and pass along the same subscription request we got and set it up so that responses get passed back to the client (be sure to properly handle the unsubscribe action as well)

This won't even end up being very much code, although it sounds difficult to write server tests for. (The more I look over the server tests the more I feel like 90% of them could be done with mocking, though it's not something I would have thought of attempting before learning Angular.)

[EliAndrewC]

This is the next thing I'm tackling. As I go, I'll take a look at how to unit test more efficiently. @robdennis and I are basically in agreement that having tests which run 100 times faster is preferable even if they offer marginally fewer assurances. So we probably don't need to spin up CherryPy for nearly as many tests as we're doing. Since this is a ticket which involves websockets, it's probably worth looking at the tests.

[EliAndrewC]

So writing tests with mock and pytest seem to be a lot easier than I expected. Like here's some basic testing of WebSocketDispatcher.send that I just wrote:

@pytest.fixture
def ws(monkeypatch):
    monkeypatch.setattr(WebSocket, 'send', Mock())
    monkeypatch.setattr(WebSocketDispatcher, 'check_authentication', lambda self: 'mock_user')
    return WebSocketDispatcher(None)

def test_basic_send(ws):
    ws.send(foo='bar', baz=None)
    WebSocket.send.assert_called_with(ANY, '{"foo":"bar"}')

def test_send_client_caching(ws):
    ws.send(client='xxx', data=123)
    ws.send(client='xxx', data=123)
    ws.send(client='yyy', data=321)
    assert WebSocket.send.call_count == 2

def test_no_send_caching_without_client(ws):
    ws.send(data=123)
    ws.send(data=123)
    ws.send(data=321)
    assert WebSocket.send.call_count == 3

def test_callback_based_caching(ws):
    ws.send(client='xxx', callback='yyy', data=123)
    ws.send(client='xxx', callback='yyy', data=123)
    assert WebSocket.send.call_count == 1
    ws.send(client='xxx', callback='zzz', data=123)
    assert WebSocket.send.call_count == 2
    ws.send(client='xxx', callback='yyy', data=123)
    ws.send(client='xxx', callback='zzz', data=123)
    assert WebSocket.send.call_count == 2
    ws.send(client='aaa', callback='yyy', data=123)
    assert WebSocket.send.call_count == 3
    ws.send(client='xxx', callback='yyy', data=321)
    assert WebSocket.send.call_count == 4
    ws.send(client='xxx', callback='zzz', data=123)
    assert WebSocket.send.call_count == 4

Those tests run super-fast, and I like how they look. This style of test does provide less confidence than a more full-stack "unit" test, but the tradeoff seems well worth it, and we can always implement some e2e tests for that, or even tests that call out to some Heroku-hosted resources for multi-server testing.

I'm going to keep chugging along on fixing the tests as part of this ticket, since this is a ticket which even our spin-up-CherryPy tests would be hard-pressed to really test, whereas mocking actually makes it more testable.

As seen in the code sample in this comment, I'll be using pytest idioms for the testing, which means that I will probably be partially addressing #13 as I go, so I've assigned that to myself.

[EliAndrewC]

Ha, already found a bug in testing. The originating_clients feature assumes that client values are globally unique, even though that's not how any of our clients are written.

This suggests two things:

• In order for this feature to work, we should probably change our client implementations to use globally unique IDs
• I may have been wrong about the advantages of full stack testing over unit testing. @ftobia in particular is welcome to say "I told you so"

[ftobia]

Nice catch. As a rule, I only say "I told you so" to people unwilling to admit it themselves. I'm glad that you might start coming around to the view that unit testing should be done on all the things.

[EliAndrewC]

Annoyingly, the unit tests (and functional tests) all pass on my machine but not on Travis. Not sure what's up with that, but I'll look into it. Also there's a change I meant to include but forgot about before pushing, so I need to update this pull request anyway.

[EliAndrewC]

Whoops! So I accidentally made a local commit in master but in order to avoid pushing to master, I immediately made a branch and then pushed that instead. But then I made an update to that branch and did a push... which pushed both the branch AND master. Apparently I should have done a git reset --hard before doing the second push (or made sure to only push the branch).

I'll look into how to fix this; I think the correct answer is to just do a reset and head and push that.