Skip to content
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

Add websocket unittests and put in users app #2921

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Andrew-Chen-Wang
Copy link
Contributor

@Andrew-Chen-Wang Andrew-Chen-Wang commented Nov 1, 2020

Description

Add unit tests to websockets and other uvicorn dependencies for speedup. I've used this in production for a month now, and my CI is very happy with it. The things that were added that you can take note of:

Add websocket unittests and packages
* Added pytest-timeout in case a websocket is stuck in a while loop
* Added unittests for testing websocket connection and pinging
* Moved websocket.py to users app since users may want to utilize other Django apps
* Added additional "Cython" dependencies to base.txt requirements file to be performant
* Added ALLOWED_HOSTS to be several different hosts for websocket testing purposes
* Added TODO for developers to add authentication/authorization for websocket handling using the scope. This is currently implemented very nicely with CSRF and Session cache checking at Velnota.com, but the amount of security checking and other files prevents me from adding my knowledge to this PR. It's possible to also use Django Channels, but I still find it infuriating to use sometimes since I support one-socket-per-user connections

Checklist:

  • I've made sure that tests/test_cookiecutter_generation.py is updated accordingly (especially if adding or updating a template option)
  • I've updated the documentation or confirm that my change doesn't require any updates

Rationale

Everything needs unit tests!

Edit: sorry about the large number of commits. I somehow messed up my git master branch, but I just force pushed it with a remote tracking this repo.

@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Nov 1, 2020

@browniebroke Is there a reason the changelog commits keep popping up? Perhaps I need to rebase master? Nvm, just hard reset master...

@Andrew-Chen-Wang Andrew-Chen-Wang changed the title Revised websockets Revised websockets: add unit tests and make it better Nov 1, 2020
@Andrew-Chen-Wang Andrew-Chen-Wang changed the title Revised websockets: add unit tests and make it better Add websocket unittests and put in users app Nov 1, 2020
@Andrew-Chen-Wang
Copy link
Contributor Author

So the unittest passes based on bare metal, but it can't pass mypy

@Andrew-Chen-Wang
Copy link
Contributor Author

@browniebroke This PR should also be ready. The last commit is stuck in Travis, but all I did was remove a whitespace. The previous commit (i.e. b0be833) is the one that fixed this.

Copy link
Member

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a lot of experience with websocket unit testing, but it feel like we're testing at the wrong level...

Do you have some resources to share on best practices in this area? It's quite new to Django, so maybe it's just a matter of waiting for ASGI to mature in 3.1 or 3.2.

tests/test_bare.sh Outdated Show resolved Hide resolved
tests/test_docker.sh Outdated Show resolved Hide resolved
@@ -1,13 +1,18 @@
async def websocket_application(scope, receive, send):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we need to move this file. Is it solely for having a place to put the tests?

I think I'm missing how it would grow in a full application... In a WSGI app, the wsgi.py isn't usually tested, and act as a project-level file, it doesn't belong to any Django app. I thought this websocket.py would also be falling into such category, but by moving it in the users app, I expect it to be specialised to the users-related functionality, but it's not the case yet...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's moved so that you can access models, like the User model. In my latest project, from ASGI, I created a URLRouter that would allow multiple "websocket_application" with some extra security features (CSRF + Session). websocket.py was meant to handle all the application code, but my own project started to fill up with multiple different "handle" methods on every message sent through the socket to the server.

The ASGI file points towards the websocket application if the scheme is wss.



@contextmanager
def run_server(app, path="/"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a lot of boilerplate 😞 Could we extract the main logic into smaller async functions and use pytest-asyncio to test them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that was my initial thought too while developing this. But in my own project code, I do use pytest-asyncio but for a different purpose.

When a text is transmitted to the server, I categorize it based on the JSON passed through. You can see this every time GitHub sends a notification to you, the client. It's JSON encoded. For example, GitHub may send:

{"subscribe": "large base 64"}

What we'd do is do a switch/case with that key/value "subscribe" (e.g. this is how GitHub auto updates this GitHub issue whenever something new happens). The problem then becomes how many different categories do you want to manage. That's when I split it into a coroutine; when you split it into a coroutine, that's when you can properly "unit" test.

@browniebroke browniebroke self-assigned this Dec 22, 2021
Andrew-Chen-Wang and others added 8 commits December 24, 2021 10:47
* Added pytest-timeout in case a websocket is stuck in a while loop
* Added unittests for testing websocket connection and pinging
* Moved websocket.py to users app since users may want to utilize other Django apps
* Added additional "Cython" dependencies to base.txt requirements file to be performant
* Added ALLOWED_HOSTS to be several different hosts for websocket testing purposes
* Added TODO for developers to add authentication/authorization for websocket handling using the scope. This is currently implemented very nicely with CSRF and Session cache checking at Velnota.com, but the amount of security checking and other files prevents me from adding my knowledge to this PR. It's possible to also use Django Channels, but I still find it infuriating to use sometimes since I support one-socket-per-user connections

Signed-off-by: Andrew-Chen-Wang <acwangpython@gmail.com>
Signed-off-by: Andrew-Chen-Wang <acwangpython@gmail.com>
* The problem I found (that's why the unittest failed) was that WSProto was not denying an incoming connection if my session authentication failed. Instead, WSProto didn't understand Uvicorn which led to an exception raised. Once encode/uvicorn#811 is fixed, we can switch back to WSProto since websockets seems unmaintained

Signed-off-by: Andrew-Chen-Wang <acwangpython@gmail.com>
@browniebroke
Copy link
Member

I don't remeber which version of Django we were using when you started this PR, but Django 3.1 added async views as well as the ability to test async code.

I was also thinking that we could review our vanilla websocket code and make it a bit more structured using django-channels (#1058), testing seems nicer: https://channels.readthedocs.io/en/latest/topics/testing.html

That's obviously a different piece of work, and I appreciate you might not have time to do it, I was wondering if you tried it on one of your projects?

@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Dec 24, 2021

It looks like I was developing this with Django 3.0 since we just upgrade to Django 3.1 recently. I don't really have the time to work on this. Working on the async ORM atm.

I have not used channels since I like working with the low level async application. I think the testing in channels is more like the client testing in Django views; I don't exactly follow that and I have an example below. I could restructure this code such that someone could more easily test it. The change is:

import json

# Note: I had to reimplement some portions
from django.core.handlers.asgi import ASGIRequest

async def websocket_application(scope, receive, send):
    # For anyone curious about my workflow:
    request = ASGIRequest(scope)
    session_authentication(request)
    if not request.user:
        await send({"type": "websocket.close"})
        return

    ...

    if event["type"] == "websocket.receive":
        try:
            await MESSAGE_TYPES[json.loads(event["message"])["type"]](event, scope, receive, send)
        except KeyError:
            pass

MESSAGE_TYPES = {
    "text message": text_message,
    "gamble": gamble
}

async def text_message(*args)
    ...

async def gamble(*args)
    ...

@pytest.mark.asyncio
async def test_text_message():
    event = {}
    await text_message(event)

@pytest.mark.asyncio
async def test_gamble():
    await gamble()

This is how typical websocket testing would look like for me: modular test cases for different methods of events received. This way, I don't actually have to test a connection itself (which is what the current test cases show). I haven't actually looked at event in its raw form in awhile, so apologies if the code is not exactly accurate.

This technique seems like it's something used by the industry. I recall looking into GitHub networking in the browser dev tab, and their websocket requests perform the same JSON serialization as I do when a client sends a message to the server.

I think the only added benefit of channels is that they include authentication middleware which may be useful. I always just did it myself since it was 7 lines of code (with JWT), but for session cookie authentication, it definitely was a lot more work. I'm not really a fan of the classes of channels since that gives the impression that you can save class attributes but ignores the fact that a user may disconnect and reconnect. That's obviously when sessions come in, but many don't consider that.


I don't really have the time, but hopefully this is a good start for anyone who'd like to take this up for a spin.

For anyone curious, this is how most of my projects work with websockets: I do a receiver-sender method where on socket connection, I create two concurrent tasks: one for listening to Redis PubSub (for global message receiving; for instance, if a user deletes their account, you publish a message to the user's subscription channel telling the websocket to close) and another for listening to client websocket receive. These are two tasks as both require a while/infinite for loop. At the end of the client websocket connection, you'll simply cancel the task that's performing the receiver for the Redis PubSub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants