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
Open
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ jobs:
fail-fast: false
matrix:
script:
- name: With Async
args: "use_async=y"
- name: With Celery
args: "use_celery=y use_compressor=y"
- name: With Gulp
Expand Down
8 changes: 7 additions & 1 deletion hooks/post_gen_project.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,13 @@ def remove_celery_files():
def remove_async_files():
file_names = [
os.path.join("config", "asgi.py"),
os.path.join("config", "websocket.py"),
os.path.join("{{cookiecutter.project_slug}}", "users", "websocket.py"),
os.path.join(
"{{cookiecutter.project_slug}}", "users", "tests", "async_server.py"
),
os.path.join(
"{{cookiecutter.project_slug}}", "users", "tests", "test_socket.py"
),
]
for file_name in file_names:
os.remove(file_name)
Expand Down
2 changes: 1 addition & 1 deletion {{cookiecutter.project_slug}}/config/asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
# application = HelloWorldApplication(application)

# Import websocket application here, so apps from django_application are loaded first
from config.websocket import websocket_application # noqa isort:skip
from {{ cookiecutter.project_slug }}.users.websocket import websocket_application # noqa isort:skip


async def application(scope, receive, send):
Expand Down
5 changes: 5 additions & 0 deletions {{cookiecutter.project_slug}}/config/settings/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
# https://docs.djangoproject.com/en/dev/ref/settings/#test-runner
TEST_RUNNER = "django.test.runner.DiscoverRunner"

{%- if cookiecutter.use_async == 'y' %}
# Needed for socket testing that also needs HTTP to go along with it
ALLOWED_HOSTS = ["localhost", "0.0.0.0", "127.0.0.1"]
{%- endif %}

# PASSWORDS
# ------------------------------------------------------------------------------
# https://docs.djangoproject.com/en/dev/ref/settings/#password-hashers
Expand Down
1 change: 1 addition & 0 deletions {{cookiecutter.project_slug}}/requirements/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ flower==1.0.0 # https://github.com/mher/flower
{%- endif %}
{%- if cookiecutter.use_async == 'y' %}
uvicorn[standard]==0.16.0 # https://github.com/encode/uvicorn
websockets==10.1 # https://github.com/aaugustin/websockets
{%- endif %}

# Django
Expand Down
7 changes: 5 additions & 2 deletions {{cookiecutter.project_slug}}/requirements/local.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@ watchgod==0.7 # https://github.com/samuelcolvin/watchgod
# ------------------------------------------------------------------------------
mypy==0.930 # https://github.com/python/mypy
django-stubs==1.9.0 # https://github.com/typeddjango/django-stubs
pytest==6.2.5 # https://github.com/pytest-dev/pytest
pytest-sugar==0.9.4 # https://github.com/Frozenball/pytest-sugar
{%- if cookiecutter.use_drf == "y" %}
djangorestframework-stubs==1.4.0 # https://github.com/typeddjango/djangorestframework-stubs
{%- endif %}
pytest==6.2.5 # https://github.com/pytest-dev/pytest
pytest-sugar==0.9.4 # https://github.com/Frozenball/pytest-sugar
{%- if cookiecutter.use_async == 'y' %}
pytest-timeout==1.4.2 # https://github.com/pytest-dev/pytest-timeout/
{%- endif %}

# Documentation
# ------------------------------------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import asyncio
import functools
import threading
import time
from contextlib import contextmanager

from uvicorn.config import Config
from uvicorn.main import ServerState
from uvicorn.protocols.http.h11_impl import H11Protocol
from uvicorn.protocols.websockets.websockets_impl import WebSocketProtocol


def run_loop(loop):
loop.run_forever()
loop.close()


@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.

asyncio.set_event_loop(None)
loop = asyncio.new_event_loop()
config = Config(app=app, ws=WebSocketProtocol)
server_state = ServerState()
protocol = functools.partial(H11Protocol, config=config, server_state=server_state)
create_server_task = loop.create_server(protocol, host="127.0.0.1")
server = loop.run_until_complete(create_server_task)
port = server.sockets[0].getsockname()[1] # type: ignore
url = "ws://127.0.0.1:{port}{path}".format(port=port, path=path)
try:
# Run the event loop in a new thread.
thread = threading.Thread(target=run_loop, args=[loop])
thread.start()
# Return the contextmanager state.
yield url
finally:
# Close the loop from our main thread.
while server_state.tasks:
time.sleep(0.01)
loop.call_soon_threadsafe(loop.stop)
thread.join()
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
from asyncio import new_event_loop

import pytest
from websockets import connect

from {{cookiecutter.project_slug}}.users.tests.async_server import run_server
from {{cookiecutter.project_slug}}.users.websocket import websocket_application as app


def test_accept_connection():
async def open_connection(url):
async with connect(url) as websocket:
return websocket.open

with run_server(app) as _url:
loop = new_event_loop()
is_open = loop.run_until_complete(open_connection(_url))
assert is_open
loop.close()


@pytest.mark.timeout(10)
def test_ping():
async def open_connection(url):
async with connect(url) as websocket:
await websocket.send("ping")
return await websocket.recv()

with run_server(app) as _url:
loop = new_event_loop()
received_message = loop.run_until_complete(open_connection(_url))
assert received_message == "pong"
loop.close()
Original file line number Diff line number Diff line change
@@ -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.

event = await receive()
if event["type"] == "websocket.connect":
# TODO Add authentication by reading scope
# and getting sessionid from cookie
await send({"type": "websocket.accept"})
else:
await send({"type": "websocket.close"})
return

while True:
event = await receive()

if event["type"] == "websocket.connect":
await send({"type": "websocket.accept"})

if event["type"] == "websocket.disconnect":
break

if event["type"] == "websocket.receive":
if event["text"] == "ping":
await send({"type": "websocket.send", "text": "pong!"})
await send({"type": "websocket.send", "text": "pong"})