chore: add comprehensive test suites for async django - #147
Conversation
There was a problem hiding this comment.
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
- [Medium]
test_auth_logoutis 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." - [Low]
get_test_resultstimeout reduced from 60s to 30s — the Django suites run many more tests than binding suites; 30s may be tight under CI load. - [Low]
test_request_path_and_content_typehas a dead code branch —echo_methodnever returns apathkey, so theif "path" in payloadbranch never executes. The test only checks Content-Type, despite the name.
|
Review posted on PR #147. Here's a summary of my findings: The I posted 3 inline comments:
|
204186a to
df8e1e2
Compare
dom96
left a comment
There was a problem hiding this comment.
I didn't review this in depth, but had a look at a few and they look good.
I do wonder though: is there an existing Django test suite that we could/should be running instead?
|
I looked at the django codebase to see how they are running unittest, but it was a bit tricky to port them as is as there were additional test requirements, test runners, etc. So I ended up generating tests myself (I gave the codebase to AI and ask it to scan and port the reasonable amount of tests). |
|
Thanks for the review! |
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.