Opened 5 years ago

Closed 5 years ago

#17928 closed Uncategorized (wontfix)

Tests with import time dependency on DB fail

Reported by: Malcolm Box Owned by: nobody
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If a test has an import time dependency on a database table, the test fails as at the point the test suite is created, the test databases have not been

This is caused by the following around line 357 of test/simple.py:

        self.setup_test_environment()
        suite = self.build_suite(test_labels, extra_tests)
        old_config = self.setup_databases()
        result = self.run_suite(suite)
        self.teardown_databases(old_config)

build_suite() is called before the databases are set up. There doesn't seem to be any dependency on the suite in setup_databases(), so this can be fixed by moving the setup_databases() call before the build_suite()

Change History (5)

comment:1 Changed 5 years ago by Carl Meyer

Resolution: needsinfo
Status: newclosed

Do you have a legitimate use case for an import-time database query? In non-test code, those are almost always bugs, as they result in data that never gets updated until a process restart; so it's a good thing if they fail under test.

Marking needsinfo, as without more detail I don't think this is a valid use case that should be supported. If you feel you have a strong use case that can't easily be handled any other way, reopen. My inclination is to close this wontfix.

comment:2 in reply to:  1 Changed 5 years ago by Malcolm Box

Resolution: needsinfo
Status: closedreopened

Replying to carljm:

Do you have a legitimate use case for an import-time database query? In non-test code, those are almost always bugs, as they result in data that never gets updated until a process restart; so it's a good thing if they fail under test.

I believe I do have a valid use case. The code under test uses it to read the domain name info from the Sites table (DOMAIN_NAME = Site.objects.get_current().domain). I know that the domain is fixed, and since my site is scaling to 10K+ requests/s I can't afford a DB query on that path.

Thus the underlying use case is "read something from the database that you know doesn't change often, where you can't afford the DB lookup in line". The alternatives would be to cache this (hassle, more code), or to initialise the variable later (all sorts of interesting race conditions).

While it might be true that doing this is sometimes a cause of bugs, those should be caught by the user's test code explicitly checking it can change the values and have things work, not by a blanket ban.

comment:3 Changed 5 years ago by Aymeric Augustin

IMO the best way to handle this use case is to load the value lazily and to cache it.

SQL queries at import time always come back and bite you (at least in my experience).

comment:4 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Also note that switching the two lines as suggested in the summary would be backwards incompatible for people who have customized the test runner.

Last edited 5 years ago by Aymeric Augustin (previous) (diff)

comment:5 Changed 5 years ago by Carl Meyer

Resolution: wontfix
Status: reopenedclosed

Marking wontfix, as this is a backwards-incompatible change without sufficient justification; it doesn't permit anything that isn't better handled with lazy-loading/caching, and it opens the door for significant bugs to go undetected. Import-time database queries are not supported. (Note also that it's not just the test-runner; putting an import-time database query in the wrong place in your codebase can easily cause circular import troubles).

Note: See TracTickets for help on using tickets.
Back to Top