Code

Opened 2 years ago

Closed 2 years ago

#17928 closed Uncategorized (wontfix)

Tests with import time dependency on DB fail

Reported by: boxm 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()

Attachments (0)

Change History (5)

comment:1 follow-up: Changed 2 years ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Status changed from new to closed

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 2 years ago by boxm

  • Resolution needsinfo deleted
  • Status changed from closed to reopened

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 2 years ago by aaugustin

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 2 years ago by aaugustin

  • 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 2 years ago by aaugustin (previous) (diff)

comment:5 Changed 2 years ago by carljm

  • Resolution set to wontfix
  • Status changed from reopened to closed

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).

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.