Skip to content

chore: add comprehensive test suites for async django#147

Open
ryanking13 wants to merge 8 commits into
mainfrom
gyeongjae/django-async-test
Open

chore: add comprehensive test suites for async django#147
ryanking13 wants to merge 8 commits into
mainfrom
gyeongjae/django-async-test

Conversation

@ryanking13

@ryanking13 ryanking13 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Adds comprehensive test suites for async django. It tests common django features and plugins and also DRF, which is a de-facto framework for django. Because of this, the Diff of this PR is pretty big, but most of them are just straightforward (AI-generated) test cases. I'll left a comment for the files that needs some human eye.

Note that django's WSGI-compatible handler is not covered in this PR and #145 needs to be merged to test it.

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm Bonk, and I've done a quick review of your PR.

This PR makes asgi.py more robust (proper lifespan protocol, null-body status handling, defensive .get() on ASGI messages, WebSocket onclose bug fix) and adds a comprehensive Django async test suite with shared test infrastructure extracted from test_bindings.py.

Issues

  1. [Medium] test_auth_logout is a false positive — the test never sends the session cookie after logout, so it can't distinguish "logout worked" from "we just made a fresh anonymous request."
  2. [Low] get_test_results timeout reduced from 60s to 30s — the Django suites run many more tests than binding suites; 30s may be tight under CI load.
  3. [Low] test_request_path_and_content_type has a dead code branchecho_method never returns a path key, so the if "path" in payload branch never executes. The test only checks Content-Type, despite the name.

Comment thread packages/cli/tests/web-frameworks-test/django-async/src/test_auth.py Outdated
Comment thread packages/cli/tests/conftest.py Outdated
@ask-bonk

ask-bonk Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review posted on PR #147. Here's a summary of my findings:

The asgi.py changes are solid — proper lifespan protocol with graceful fallback for apps that don't support it, defensive .get() on ASGI message fields, null-body status handling, and a real bug fix for WebSocket (server.onopen = oncloseserver.onclose = onclose). The test infrastructure refactoring into shared conftest.py helpers is clean.

I posted 3 inline comments:

  1. [Medium] test_auth_logout false positive — after calling logout, the test fetches /auth/user/ without the session cookie, so it would pass even if logout was a no-op. Suggested sending the old session cookie to verify invalidation.

  2. [Low] get_test_results timeout — reduced from 60s to 30s in the shared helper, but Django suites are heavier; flagged as a potential flaky test source.

  3. [Low] Dead code in test_request_path_and_content_type — the if "path" in payload branch never executes since echo_method doesn't return a path key. Suggested simplifying.

github run

Comment thread packages/runtime-sdk/src/asgi.py
Comment thread packages/runtime-sdk/src/asgi.py Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

common functions in this file are moved to conftest.py so we can reuse.

@ryanking13 ryanking13 force-pushed the gyeongjae/django-async-test branch 2 times, most recently from 204186a to df8e1e2 Compare July 6, 2026 09:33
@ryanking13 ryanking13 changed the title fix: make asgi.py implementation more robust + add comprehensive test suites for async django chore: add comprehensive test suites for async django Jul 9, 2026
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.

3 participants