django / channels

Developer-friendly asynchrony for Django
https://channels.readthedocs.io
BSD 3-Clause "New" or "Revised" License
6.01k stars 793 forks source link

`TEST_CONFIG` key has code using it but seems to be broken and no longer documented #2058

Open fish-face opened 7 months ago

fish-face commented 7 months ago

I saw this in documentation for 1.x without realising. Tried to use it and nothing changed - checked the code and there's still code for it but as far as I can tell nothing every calls make_test_backend. I suspect this is dead code. I did then dig a little more and it seems maybe you were supposed to use this with ChannelsLiveServerTestCase. As far as I can tell this still doesn't work; I can't use this TestCase directly (I need to inherit from another one) but I copied across the _pre_setup and _post_teardown methods just to make sure - but it's clear that they don't do anything to create a different channels backend in the current process, and I can't imagine there's anything happening in the spawned process either.

However, the original use case for TEST_CONFIG still exists: to avoid interfering with running dev instances. My plan was to have config like this:

CHANNEL_LAYERS = {
    'default': {
        'BACKEND': 'channels_redis.core.RedisChannelLayer',
        'CONFIG': {
            'hosts': [('redis', 6379)],
            'prefix': 'app:channels',
        },
        'TEST_CONFIG': {
            'hosts': [('redis', 6379)],
            'prefix': 'app:channels_test',
        },
    },
}

So that, similar to the test database, we have a completely different set of keys in redis - but don't have to run a separate redis instance.

Just to make sure, I ran MONITOR on the redis-cli to check that my hacky test case wasn't using the new prefix and indeed it was not - app:channels appears and not app:channels_test in the keys.

I think either the dead code ought to be removed or, preferably, the functionality to have a separate test config should be fixed or if in fact it does work and I'm just using it wrong, it ought to be documented again!

I will expand on a couple of points in case they're useful:

  1. we have no need to run these tests with a live server - if that was ever a limitation, it would be preferable to avoid it.
  2. we already need a custom subclass of TestCase to handle special setup and teardown (specifically for django-tenants if you're curious). There's an obvious advantage to bundling this functionality up in a TestCase subclass, but it makes sense to me to expose the functions such a subclass uses as its own API so that other subclasses can still make use of it cleanly and without lots of repeated code.
carltongibson commented 7 months ago

Hi, thanks for the report, yes.

A quick grep, it looks like make_test_backend() is just never used.

I'm not sure what to say: I'd just use a different CHANNEL_LAYERS setting for tests, rather than branch on the same settings. This seems simpler… 🤔

So, I guess I'd lean towards removing it. Do you have an idea for making it work?

fish-face commented 7 months ago

I was unable to find what changed, or any bug report about it not working, so while at first I thought it'd just be a matter of reinstating whatever used to exist, I couldn't find what that was, and I'm not sure if it was ever working as intended.

To be sure, a wholly different settings file would do the job, but if it can be done reasonably it makes sense to do so for the same reason that django creates a separate test database.

On Tue, 21 Nov 2023 at 15:19, Carlton Gibson @.***> wrote:

Hi, thanks for the report, yes.

A quick grep, it looks like make_test_backend() is just never used.

I'm not sure what to say: I'd just use a different CHANNEL_LAYERS setting for tests, rather than branch on the same settings. This seems simpler… 🤔

So, I guess I'd lean towards removing it. Do you have an idea for making it work?

— Reply to this email directly, view it on GitHub https://github.com/django/channels/issues/2058#issuecomment-1821128231, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7SILVWDD34VSJGHAL6UBLYFTBANAVCNFSM6AAAAAA7UUNTV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGEZDQMRTGE . You are receiving this because you authored the thread.Message ID: @.***>