Opened 5 weeks ago

Last modified 3 weeks ago

#36083 assigned Bug

LiveServerTestCase fails in parallel test runner if django.contrib.auth.backends has not yet been imported — at Version 7

Reported by: Adam Zapletal Owned by:
Component: Testing framework Version: dev
Severity: Normal Keywords: TransactionTestCase setupclass available_apps
Cc: Adam Zapletal, Adam Johnson Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Adam Zapletal)

The full test suite run with multiple threads passes for me, but when I run the tests.file_storage.tests file in isolation (./runtests.py file_storage.tests), I get a failure. It passes in isolation if I run the tests with only one thread (./runtests.py --parallel 1 file_storage.tests). This happened to others when I asked about it in the Django Discord server, but not everyone. For what it's worth, I'm on the latest macOS running Python v3.12.

When a failure happens, I get the following error:

django.core.exceptions.ImproperlyConfigured: AUTH_USER_MODEL refers to model 'auth.User' that has not been installed

I have narrowed the issue down to the fact that this test file contains a class that inherits from LiveServerTestCase. It seems like any test file that mixes LiveServerTestCase and something more normal like unittest.TestCase will fail for me when run in isolation with more than one thread.

Here's a minimal example if anyone is interested:

from unittest import TestCase

from django.test import LiveServerTestCase


class TestCaseTests(TestCase):
    def test(self):
        self.assertEqual(2, 2)


class LiveServerTestCaseTests(LiveServerTestCase):
    available_apps = []

    def test(self):
        self.assertEqual(2, 2)

I have a fix, but I'm not sure if it's a good one. Setting available_apps = ['django.contrib.auth'] on the LiveServerTestCase-based class fixes it as one would expect from the error message, but I wonder if this is hiding something like a race condition. I wonder why it passes the full test suite when run with multiple threads, but it can't pass in isolation when run with multiple threads.

Should I open a pull request to fix the test in the way I mentioned above, or is something deeper going on here? It could be that I'm misunderstanding something about how LiveServerTestCase is isolated during test runs.

Change History (7)

comment:1 by Jacob Walls, 5 weeks ago

Keywords: TransactionTestCase setupclass available_apps added
Severity: NormalRelease blocker
Summary: tests.file_storage.tests can fail when run in isolationTransactionTestCase available_apps not set correctly for parallel test runner
Triage Stage: UnreviewedAccepted

Thanks, I remember replying on Discord that I could reproduce this.

Bisected to a060a22ee2dde7aa29a5a29120087c4864887325. Hey, that was me!

Looks like some debugging is needed around why apps.set_available_apps(cls.available_apps) is not being called at a point that works for the parallel test runner.

Would you like to look into this a little bit? I'm marking as a release blocker, and since the alpha is scheduled to go out this week, I can make some time to investigate as well. Having a couple people look at it will be helpful.


RE: your original proposal, I think we have another option, which would be to do this, but I think you're right this is just masking the deeper issue:

  • tests/file_storage/tests.py

    diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py
    index 5f0024b81a..86429ec3a8 100644
    a b from django.test import (  
    3030    SimpleTestCase,
    3131    TestCase,
    3232    ignore_warnings,
     33    modify_settings,
    3334    override_settings,
    3435)
    3536from django.test.utils import requires_tz_support
    class ContentFileStorageTestCase(unittest.TestCase):  
    12331234
    12341235
    12351236@override_settings(ROOT_URLCONF="file_storage.urls")
     1237@modify_settings(MIDDLEWARE={"remove": "django.contrib.auth.middleware.AuthenticationMiddleware"})
    12361238class FileLikeObjectTestCase(LiveServerTestCase):
    12371239    """
    12381240    Test file-like objects (#15644).

comment:2 by Jacob Walls, 5 weeks ago

available_apps is a private API, so we may have some discretion about whether to block the alpha over it, but I'm hoping a fix develops to make deciding that moot.

comment:3 by Jacob Walls, 5 weeks ago

Summary: TransactionTestCase available_apps not set correctly for parallel test runnerLiveServerTestCase fails in parallel test runner if django.contrib.auth.backends has not yet been imported

This was a fun one. Here's what I found:

  • The commit I blamed exposed this by lifting the first call to set_available_apps() up into TransactionTestCase.setUpClass() (via _pre_setup()), and so the order of operations in LiveServerTestCase became different.
  • The reported issue goes away by moving super().setUpClass() to the bottom of LiveServerTestCase.setUpClass().

Why?

  • The check_middleware system check registered by django.contrib.auth has the side effect of importing django.contrib.auth.backends
  • The parallel test runner does not run the system checks per-process
  • When a parallel test runner worker tries to setUpClass() a LiveServerTestCase, it may be importing django.contrib.auth.backends for the first time and thus running get_user_model()
  • get_user_model() will fail (as designed) if django.contrib.auth is not in available_apps, which is now the case based on the change in the order of operations

I tested two solutions, both of which work:

  1. Running system checks inside each worker, to ensure the side effect described above is executed in each process:
    • django/test/runner.py

      diff --git a/django/test/runner.py b/django/test/runner.py
      index b83cd37343..360bba4fd7 100644
      a b def _init_worker(  
      419419    process_setup_args=None,
      420420    debug_mode=None,
      421421    used_aliases=None,
       422    verbosity=1,
      422423):
      423424    """
      424425    Switch to databases dedicated to this worker.
      def _init_worker(  
      442443            process_setup(*process_setup_args)
      443444        django.setup()
      444445        setup_test_environment(debug=debug_mode)
       446        call_command("check", verbosity=verbosity, databases=used_aliases)
      445447
      446448    db_aliases = used_aliases if used_aliases is not None else connections
      447449    for alias in db_aliases:
      class ParallelTestSuite(unittest.TestSuite):  
      496498    runner_class = RemoteTestRunner
      497499
      498500    def __init__(
      499         self, subsuites, processes, failfast=False, debug_mode=False, buffer=False
       501        self,
       502        subsuites,
       503        processes,
       504        failfast=False,
       505        debug_mode=False,
       506        buffer=False,
       507        verbosity=1,
      500508    ):
      501509        self.subsuites = subsuites
      502510        self.processes = processes
      class ParallelTestSuite(unittest.TestSuite):  
      506514        self.initial_settings = None
      507515        self.serialized_contents = None
      508516        self.used_aliases = None
       517        self.verbosity = verbosity
      509518        super().__init__()
      510519
      511520    def run(self, result):
      class ParallelTestSuite(unittest.TestSuite):  
      536545                self.process_setup_args,
      537546                self.debug_mode,
      538547                self.used_aliases,
       548                self.verbosity,
      539549            ],
      540550        )
      541551        args = [
      class DiscoverRunner:  
      983993                    self.failfast,
      984994                    self.debug_mode,
      985995                    self.buffer,
       996                    self.verbosity,
      986997                )
      987998        return suite
  2. Adjusting the order of operations in LiveServerTestCase.setUpClass() to act as before:
    • django/test/testcases.py

      diff --git a/django/test/testcases.py b/django/test/testcases.py
      index 36366bd777..d641bb3c6b 100644
      a b class LiveServerTestCase(TransactionTestCase):  
      18171817
      18181818    @classmethod
      18191819    def setUpClass(cls):
      1820         super().setUpClass()
      18211820        cls.enterClassContext(
      18221821            modify_settings(ALLOWED_HOSTS={"append": cls.allowed_host})
      18231822        )
      18241823        cls._start_server_thread()
       1824        super().setUpClass()
      18251825
      18261826    @classmethod
      18271827    def _start_server_thread(cls):

Then of course there is:

  1. Fiddle with available_apps and/or MIDDLEWARE so that LiveServerTestCase subclasses always either have contrib.auth in available_apps or else no middleware that relies on it.

Option 1 feels like a workaround for the fact that a certain system check has a side effect. It's a more invasive diff, and it adds performance drag.
Option 2 feels like the simplest thing to do for now to preserve prior behavior, although it's a workaround for the fact that a system check has a side effect
Option 3 feels more semantically correct but would need some thought around the design to make it ergonomic (fiddling with this in every subclass doesn't seem right)

Adam, what do you think? Would you like to prepare a patch with a regression test?

If folks cannot reproduce the issue, I would venture a guess that it's due to the difference between "spawn" and "fork" defaults. I tested on MacOS where the default is spawn.

Last edited 5 weeks ago by Jacob Walls (previous) (diff)

comment:4 by Adam Zapletal, 5 weeks ago

Great job researching this, and thanks for the quick response!

For context, I found this issue while working on #10244, and this is not my area of expertise in the codebase. However, here are my initial thoughts on the options you presented:

  1. Yes, the diff is a bit unfortunate, but running system checks in each worker seems more correct to me if I'm understanding the design correctly. It seems like we'd want to run them in every context or none.
  2. This seems like a simple band-aid fix that would work for now if this ticket has a deadline. I'm not sure of all the implications here.
  3. I agree that this seems correct in a way similar to the first option. We don't want to evade system checks on accident.

I'm a bit thrown off by the fact that the system check in question, via ‎_subclass_index, has the side effect of importing a module. I'm wondering the following:

  1. Are there other system checks with side effects?
  2. Is there a way to implement this system check without the side effect?

I'm not familiar enough with the system check framework to know what's reasonable here, though I'd be interested to know.


This may be irrelevant, but if I grep the test suite for uses of LiveServerTestCase, I find the following:

  1. Tests testing aspects LiveServerTestCase itself (and StaticLiveServerTestCase)
  2. A test class for the startproject command that sets available_apps correctly
  3. The file storage test class that this ticket is about, which sets available_apps to an empty list (for context, available_apps was set here.)

So we can say that the test class in question is the only one in the whole test suite that uses LiveServerTestCase but doesn't include django.contrib.auth in available_apps. I'm not sure what that implies, but this test is an outlier in multiple ways. Should the test itself be questioned?

comment:5 by Jacob Walls, 5 weeks ago

Thanks Adam.

Is there a way to implement this system check without the side effect?

To find out whether a middleware class declared by a string is a subclass of something else, I think we have to import the string.

I may have skewed things by using the phrase "side effect": I think it's good to admit flexibility in the order by which modules are imported. As the long as the module-level caches are designed well, then in practice it's not really a problem. It does raise test isolation concerns.

The test framework goes to great effort to reset the database layer between tests, but the python layer is left up to the user. It might be worthy to try out adding an --isolate-imports flag to the test runner in a third-party package that would reset sys.modules between test case classes. (I don't know if there's a test framework out there that already implements such a thing.)

Here is where I would cc Adam Johnson if I had the right permission bit.

Should the test itself be questioned?

Thanks for the grep -- it's true Django itself only has one test case class in this situation, but a user could be doing this. I think there would have been a similar problem with SeleniumTestCase if it were supported by the parallel test runner. (It doesn't today but might in the future.)

This seems like a simple band-aid fix that would work for now if this ticket has a deadline.

I tenatively marked this as a release blocker since it smelled like a regression in Django 5.2: your test case class breaks if it depends on django.contrib.auth via LiveServerTestCase (or SeleniumTestCase if they have a way to run it in parallel or skip checks) but sets available_apps = []. The wrinkle being that available_apps is a private API we can change at will. I'll let the fellows speak to urgency.

Option 3 feels more semantically correct but would need some thought around the design to make it ergonomic (fiddling with this in every subclass doesn't seem right)

We could also just doc that LiveServerTestCase subclasses should be careful to to include auth in available_apps, given we've already documented this as an advanced/private feature. Or add fanciness to to LiveServerTestCase._pre_setup() to sniff for this misconfiguration, but the complexity tradeoff looks bad if we need to be resilient against subclasses of the auth middleware (hence the whole reason for _subclass_index in the system check in the first place).

  1. This seems like a simple band-aid fix that would work for now if this ticket has a deadline. I'm not sure of all the implications here.

Re: shuffling the cheese around in LiveServerTestCase.setUpClass(), I guess it has the benefit of clarifying that available_apps is really only about signals and the database layer, and we could say that it's not a way to adjust the app registry for other purposes like skipping imports.


I'm fine with any of the three options. Would like to hear from others.

comment:6 by Adam Zapletal, 4 weeks ago

I appreciate your insight here. Please know that I wasn't questioning your approach...I'm just trying to understand the problem!


One point of clarification on this comment:

Thanks for the grep -- it's true Django itself only has one test case class in this situation, but a user could be doing this. I think there would have been a similar problem with SeleniumTestCase if it were supported by the parallel test runner. (It doesn't today but might in the future.)

I was assuming that in a real project (not Django itself), the user wouldn't have this problem. They'd be opting into setting available_apps on their test class, so we can expect them to do it correctly. The attribute is only required in Django's test suite.

When I remove the linked check from runtests.py or set available_apps to None, the test in question passes.


I'd like to hear from others as well. Do you want to ping people on this ticket, or should we open a forum thread?

comment:7 by Adam Zapletal, 4 weeks ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.
Back to Top