channels
channels copied to clipboard
Make ChannelsLiveServerTestCase compatible with Django 5.2 (#2148)
Description
In Django 5.2, the _pre_setup method of SimpleTestCase (and consequently TransactionTestCase) has been changed to a classmethod.
See in references in Django 5.2 and Django 5.1
I’ve implemented a minimal, Django version based change without disrupting its existing architecture. Let me know if you require a different design solution.
To test this, you have to create a test sample app. Should I add a minimal app in this PR as well?
Closes #2148
Special Thanks
This work was completed as part of OpenWISP's contribution to Google Summer of Code 2025. Thanks to both organizations for their support!
To test this, you have to create a test sample app. Should I add a minimal app in this PR as well?
Yes, please add a small Django project here which can be used to test the ChannelsLiveServerTestCase class.
How to proceed?
Hey @carltongibson, I am thinking of creating a sample real-time counter app using Django and Channels.
I am thinking of adding a live counter app using django which will have an input field on sending some text message which will broadcast live to all connected clients and appears in the list below with a count of total messages and show the text message instantly in real time in the django admin and create selenium tests for this using the ChannelsLiveServerTestCase.
Before proceeding further, I’d love to get your thoughts. Do you think this example is a good fit, or would you recommend a different solution?
@dee077 That sounds like a good example yes. It exercises the basics but is not too complex.
Updates
- Added sample project as described above.
- Add more settings to run the sample project in
conftest.py - Added selenium tests for the sample project using
ChannelsLiveServerTestCase - Make every test use
@pytest.mark.django_db(transaction=True). Without transaction=True, some tests were conflicting with the WebSocket connection while running the Chrome WebDriver instance, causing below errors and failing the test suite.
scripts.js:12 WebSocket connection to 'ws://localhost:46483/ws/message/'
failed: Error during WebSocket handshake: net::ERR_CONNECTION_RESET
- Added
seleniuminsetup.cfgin test dependencies - Added
chromedriverin ci
Let me know if any updates are required.
Hi @carltongibson, Please let me know if the current sample project works or if there are any changes you'd like me to make.
Hi @dee077 — thanks for the ping. Last time I checked the test were failing... — running them again now.
Update: And they passed! 💃 — Let me take a proper look. Thanks for the effort! 🎁
Updates
- Minor space fixes suggested by @nemesifier
- Added attribution for the SeleniumMixin, adapted from OpenWISP
- Source link to the original mixin in openwisp-utils for reference and credit
I accidentally created another PR #2164 . We may should use the fix for LiveChannelsTestCase from there because it is cleaner and fixes also a hangup issue and your tests.
Updates
- Used the fix for
ChannelsLiveServerTestCasefrom #2164 and keeping the sample project and test from this PR. - Cleaned up the
conftest.pyas suggested.
I had one question regarding the fix from @devkral
I had considered using setUpClass() instead of _pre_setup() when working on the fix, but opted for a minimal change by conditionally converting _pre_setup to a @classmethod based on the Django version.
How does moving the _is_in_memory_db() check from _pre_setup() (which runs before each test) to setUpClass() (which runs once per class) not change the behavior?
Isn't there a possibility that the database configuration might change between tests?
Hi @carltongibson, I have made the changes @devkral suggested. Please let me know if this works
sry for the late answer: we just need to check it once. We keep the server open across all the tests in the test-suite. A server should be able to serve multiple connections.
This follows the new behavior of the django live-server testcase. It might be a bit too isolated (we use a process, so it has a different instance of the python interpreter) (maybe we cannot overwrite the settings in the single test methods but this is generally offtopic when using a live-server and can be worked around by using multiple testcases just in case the behavior appears)
Hi @carltongibson, @devkral, Are we planning to move forward with these changes? At the moment, I've created a full Django app as a sample project. Please let me know if you'd prefer a more minimal setup or if you have any other reservations about these changes that you'd like me to address.
Thanks for rebasing @nemesifier. @dee077 Just to let you know I'm swinging to a Channels release now (After the recent Daphne and Asgiref updates.) This is top of my list. Thanks for your patience.
Thanks for rebasing @nemesifier. @dee077 Just to let you know I'm swinging to a Channels release now (After the recent Daphne and Asgiref updates.) This is top of my list. Thanks for your patience.
Great to hear @carltongibson! When you're ready, let us know any requested changes and we'll follow up asap.
Hi @dee077 @nemesifier.
So this all looks good.
I'm having difficulty getting the texts passing locally. I have the chrome driver set up etc, but when I run the test suite, the migrations aren't run for the test database:
tox -e py313-dj52
...
django.db.utils.OperationalError: no such table: auth_user
===================================== short test summary info =====================================
FAILED tests/sample_project/tests/test_selenium.py::TestSampleApp::test_real_time_create_message - AssertionError: visibility_of_element_located of "#content-main" failed: Message:
FAILED tests/sample_project/tests/test_selenium.py::TestSampleApp::test_real_time_delete_message - AssertionError: visibility_of_element_located of "#content-main" failed: Message:
I like the test project - it's a cool addition. It might work well in an examples/ folder with a README, so users could install the dependencies, run it, and run the test suite, including selenium tests. If we did that, is there a much more minimal test we could have in the test suite to verify the fix here, I wonder? (Maybe not.)
Then, do we not need to account for @pandafy's https://github.com/django/channels/pull/2160#discussion_r2099658418? — Django 4.2 isn't quite EOL yet.
I'm having difficulty getting the texts passing locally.
OK, that bit is #2030/#2033 with a fix from @ashtonrobinson.
@dee077 Can you please enable maintainer edits on this PR? Thanks!
Then, do we not need to account for @pandafy's https://github.com/django/channels/pull/2160#discussion_r2099658418? — Django 4.2 isn't quite EOL yet.
Testing with Django 4.2, it looks like this isn't needed. It's Sunday, so I didn't quite reason through why, but 👍
It's Sunday, so I didn't quite reason through why, but 👍
OK, I see: you pulled in @devkral's fix from #2164. ✅
OK, resolved in #2172. Thanks all!
Thank you veyr much @carltongibson :clap: