channels icon indicating copy to clipboard operation
channels copied to clipboard

Make ChannelsLiveServerTestCase compatible with Django 5.2 (#2148)

Open dee077 opened this issue 6 months ago • 12 comments

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!

dee077 avatar May 20 '25 17:05 dee077

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.

pandafy avatar May 22 '25 06:05 pandafy

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 avatar May 22 '25 18:05 dee077

@dee077 That sounds like a good example yes. It exercises the basics but is not too complex.

carltongibson avatar May 23 '25 05:05 carltongibson

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 selenium in setup.cfg in test dependencies
  • Added chromedriver in ci

Let me know if any updates are required.

dee077 avatar May 30 '25 14:05 dee077

Hi @carltongibson, Please let me know if the current sample project works or if there are any changes you'd like me to make.

dee077 avatar Jun 04 '25 19:06 dee077

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! 🎁

carltongibson avatar Jun 05 '25 07:06 carltongibson

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

dee077 avatar Jun 07 '25 16:06 dee077

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.

devkral avatar Jun 11 '25 20:06 devkral

Updates

  • Used the fix for ChannelsLiveServerTestCase from #2164 and keeping the sample project and test from this PR.
  • Cleaned up the conftest.py as 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?

dee077 avatar Jun 12 '25 22:06 dee077

Hi @carltongibson, I have made the changes @devkral suggested. Please let me know if this works

dee077 avatar Jun 17 '25 15:06 dee077

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)

devkral avatar Jun 23 '25 08:06 devkral

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.

dee077 avatar Jun 26 '25 19:06 dee077

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.

carltongibson avatar Jul 10 '25 06:07 carltongibson

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.

nemesifier avatar Jul 10 '25 18:07 nemesifier

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.

carltongibson avatar Jul 27 '25 07:07 carltongibson

I'm having difficulty getting the texts passing locally.

OK, that bit is #2030/#2033 with a fix from @ashtonrobinson.

carltongibson avatar Jul 27 '25 08:07 carltongibson

@dee077 Can you please enable maintainer edits on this PR? Thanks!

carltongibson avatar Jul 27 '25 08:07 carltongibson

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 👍

carltongibson avatar Jul 27 '25 08:07 carltongibson

It's Sunday, so I didn't quite reason through why, but 👍

OK, I see: you pulled in @devkral's fix from #2164. ✅

carltongibson avatar Jul 27 '25 08:07 carltongibson

OK, resolved in #2172. Thanks all!

carltongibson avatar Jul 27 '25 10:07 carltongibson

Thank you veyr much @carltongibson :clap:

nemesifier avatar Jul 28 '25 15:07 nemesifier