Skip to content

Make ChannelsLiveServerTestCase compatible with Django 5.2 (#2148) #2160

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dee077
Copy link

@dee077 dee077 commented May 20, 2025

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!

self._server_process.start()
self._server_process.ready.wait()
self._port = self._server_process.port.value
if VERSION >= (5, 2):
Copy link

@pandafy pandafy May 21, 2025

Choose a reason for hiding this comment

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

From the point of maintenance and avoiding repetitive code, can we do this instead?

class ChannelsLiveServerTestCase(TransactionTestCase):
    # original definition (unchanged)

if VERSION >= (5, 2):
    ChannelsLiveServerTestCase._pre_setup = classmethod(ChannelsLiveServerTestCase._pre_setup)

I found that the QuerySet.as_manager uses similar definition, albeit for different reasons
https://github.com/django/django/blob/8c279113864c31cef539c6384a683e6ed832adca/django/db/models/query.py#L311-L320

Let's also add a comment explaining that this is required because of changes in Django 5.2 and add a TODO to remove this workaround while dropping support for older Django version (<5.2).

Copy link
Contributor

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

To test this, you have to create a test sample app. Should I add a minimal app in this PR as well?

I think we should do this. What do you think @pandafy?

@pandafy
Copy link

pandafy commented May 22, 2025

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.

@dee077
Copy link
Author

dee077 commented May 22, 2025

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?

@carltongibson
Copy link
Member

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

@dee077 dee077 force-pushed the issues/2148-ChannelsLiveServerTestCase-Django52-compatible branch 2 times, most recently from 2fd8d25 to adf1ba2 Compare May 30, 2025 14:30
@dee077
Copy link
Author

dee077 commented May 30, 2025

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.

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.

ChannelsLiveServerTestCase fails with django 5.2
4 participants