Code

Opened 6 years ago

Last modified 4 weeks ago

#7835 new New feature

Provide the ability for model definitions that are only availably during testing

Reported by: russellm Owned by: kkubasik
Component: Testing framework Version: master
Severity: Normal Keywords: feature test models
Cc: martin@…, oliver@…, t.django@…, steingrd@…, kmike84@…, adrianlopezcalvo@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

A current limitation of the unit test framework is that there is no capacity to define 'test models' - that is, models that are only required for the purposes of testing. A regular installation would not know anything about these models - only a test database would have access to them.

There are several existing applications that have a need for this capability: For example:

  • contrib.admin: you can't test the way admin handles models without some models to handle.
  • contrib.databrowse: you can't test the way the browser works without having models to browse
  • Django Evolution: you can't evolve models without having some models to evolve.


The easiest way to work around this at present is to have a standalone test project which exercises the required functionality. However, these tests aren't integrated into the automated test suite, so they.

Another option is to do some app_cache munging during the test - this works, but is very messy.

Django should provide a way for model definitions to be defined as part of the test definition, synchronized as part of the test setup, populated and manipulated during test execution, and destroyed along with the test database.

Attachments (3)

7835.test_extra_apps.diff (3.0 KB) - added by julien 5 years ago.
Path with suggested new API (installed_apps and extra_apps)
7835.test_extra_apps.2.diff (13.0 KB) - added by julien 5 years ago.
patch+test+doc
minimal-abstract-1.6-testcase.tar.bz2 (3.2 KB) - added by JocelynD 2 months ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 6 years ago by jamesturk

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 6 years ago by russellm

  • Triage Stage changed from Design decision needed to Accepted

comment:3 Changed 5 years ago by peritus <peritus@…>

Workaround:

import sys

if sys.argv[1] == 'test':
    # This is a bit hacky, see
    # http://code.djangoproject.com/ticket/7835
    from django.db import models

    class Poll(models.Model):
        # .. ..

comment:4 follow-up: Changed 5 years ago by julien

A discussion came about on the user-list about this: http://groups.google.com/group/django-users/browse_thread/thread/559aa0a2d074a7b5

I wrote a patch based on Russell's feedback. The proposed API is as follows:

class MyMetaTest(TestCase):
    installed_apps = ['fakeapp','otherapp']
    extra_apps = ('yetanotherapp',)

    def test_stuff(self):
      ... 
  • installed_apps and extra_apps can either be tuple or a list. They can coexist or be used individually.
  • installed_apps overrides the settings INSTALLED_APPS.
  • extra_apps adds the given apps either to INSTALLED_APPS or to installed_apps if it also exists.

Now, responding specifically to Russell's remarks:

"Obviously, the test applications need to be:

  1. Added to INSTALLED APPS and the app cache on startup
  2. Installed in the app cache before the syncdb caused by the pre-test database flush takes effect. You shouldn't need to manually invoke syncdb.
  3. Removed from INSTALLED_APPS and the app cache on teardown"

My answers below:

  1. Done
  2. Done, but I think syncdb still needs to be invoked to create the extra tables, otherwise flush will raise an exception saying it cannot find those tables.
  3. INSTALLED_APPS is properly restored, but I could not find a way to unload the apps from the cache. Should an extra method be written in the AppStore class (unload_app() and/or unregister_models())?

Changed 5 years ago by julien

Path with suggested new API (installed_apps and extra_apps)

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by russellm

Replying to julien:

  1. Done, but I think syncdb still needs to be invoked to create the extra tables, otherwise flush will raise an exception saying it cannot find those tables.

This reveals a slightly larger bug, which I've logged as #9717. The right approach here is to fix #9717, not work around the issue.

  1. INSTALLED_APPS is properly restored, but I could not find a way to unload the apps from the cache. Should an extra method be written in the AppStore class (unload_app() and/or unregister_models())?

I would be highly surprised if you could deliver this patch without adding some cache cleanup methods to the AppStore.

Also - I know this is early days, but just as a friendly reminder before I turn into the Patch Hulk: Patch need tests! Patch need docs! Aaargh! Hulk angry!!... (oh dear... too late! :-)

But how do you test a testing patch? One suggestion - migrate some of the existing Django tests to use the new framework. For example, contrib.admin has tests currently stored in the system tests that should probably be application tests. If you migrate the admin tests, this will demonstrate that your patch works and has sufficient capabilities for a real-world testing situation.

One issue that the documentation should address - some sort of convention to avoid application name clashes. manage.py validate will prevent model and table clashes, but if I name one of my test applications 'sites' or 'auth', all hell could break lose. You might want to suggest some form of convention around test application naming - for example, naming all test applications 'test_foo', rather than just 'foo'. Alterntaively, you could enforce this programatically by registering test applications with a test_ prefix (this could raise other complications though - it might be easier to stick with the convention).

comment:6 in reply to: ↑ 5 Changed 5 years ago by julien

Replying to russellm:

This reveals a slightly larger bug, which I've logged as #9717. The right approach here is to fix #9717, not work around the issue.

Ok, I'll look at that one, see if I can help.

I would be highly surprised if you could deliver this patch without adding some cache cleanup methods to the AppStore.

Will look at that too.

Also - I know this is early days, but just as a friendly reminder before I turn into the Patch Hulk: Patch need tests! Patch need docs! Aaargh! Hulk angry!!... (oh dear... too late! :-)

Aahhh!! The Patch Hulk is back!!! :) In fact, I didn't want to write too much doc/tests before settling on the API. What's your view on this? Is the suggested API ok, or should it be different?

As you suggest, I'll try to migrate some admin tests. For the doc, I agree with you that some clear and well-explained conventions would be preferable. I'll introduce that in the next patch.

comment:7 Changed 5 years ago by russellm

Deferring documentation is ok; writing good docs takes a lot of time, and it takes a lot of effort to rework if a big design change is made. I think the API is stable enough at this point to warrant making a start on documentation.

However, deferring tests isn't ok. Tests are how you prove to me that the code works, that you've though about all the nasty ways that things can go wrong, and that you are handling those exceptions.

comment:8 Changed 5 years ago by julien

Point taken about the tests ;) I had tested in-house with some of my apps, but it's true that without tests it's hard to prove to others that it works. I also wasn't sure how to write tests for this, but migrating admin tests seem like a good one.

Another question. Shouldn't ticket #9717 be merged with this one? After all, the issue emerged from this ticket and no one's really complained about that separately before (correct me if wrong). The same thing applies for adding unloading methods to AppStore, which at this stage wouldn't justify a ticket on its own.

My only concern is that all these issues (flush, AppStore and test apps) need to be fixed all at once. And while I'm happy to try to fix them (and for anyone else who'd be interested to contribute), managing 2 sets of tickets/patches would be more painful to handle.

What do you think? Just let me know, and I'll comply to whatever you decide ;)

comment:9 Changed 5 years ago by russellm

No - #9717 shouldn't be merged, that's why I opened another ticket. Consider - #9717 can be fixed without requiring a fix for this ticket; likewise, this ticket could be closed without fixing #9717. Hence, they are separate tickets. However, I would say that fixing #9717 is a reasonable pre-requisite for merging this ticket, as any fix for this ticket will need to work around the problem identified by #9717.

There is also the branch management issue; a patch for #9717 will need to be applied to the v1.0.X branch because it is a bug fix; this ticket describes a feature addition, so it will only be applied to trunk.

Sure, not many people have complained about this problem, but that doesn't mean nobody has had the problem. It is entirely possible that people have been having the problem but not understanding the cause.

Regarding two patches being difficult to handle: If you provide a standalone patch for #9717, I will get it into the trunk with haste, as it is a clearly identified bug with existing code.

Changed 5 years ago by julien

patch+test+doc

comment:10 follow-up: Changed 5 years ago by julien

So I have finally migrated the tests for contrib.comments which seems a more doable intermediary step before migrating huge tests like the admin's. I also wrote a piece of documentation to present the new API and a suggested convention for avoiding app name clashes.

The only thing that was requested and which I haven't done is unloading the test apps from the AppCache. But, is it really necessary? It will be highly recommended to developers to follow a certain convention to avoid name clashes. If they do so, then unloading the models/apps from the cache won't be crucial to do. If they don't do so, then there might be a whole lot of unpredicted conflicts at several other levels anyway. Ideas?

comment:11 follow-up: Changed 5 years ago by julien

Oh, another note. In the code patch, I still need to run syncdb to create potential new tables for the test apps' models. I don't think this can really be avoided. And I don't think this is a problem either.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by russellm

Replying to julien:

Oh, another note. In the code patch, I still need to run syncdb to create potential new tables for the test apps' models. I don't think this can really be avoided. And I don't think this is a problem either.

The problem isn't (just) one of code neatness - it's the execution time for the tests. On my machine, the full Django's system test suite takes about 5 minutes to execute for SQLite; almost 10 minutes to run for Postgres. I haven't run the Oracle tests myself, but I've been lead to believe that it goes from "go make yourself a cup of coffee" to "go make yourself a 9-course degustation banquet". I'm going to be very picky about introducing anything that has the potential to make this situation worse.

Syncdb isn't a no-op when there is nothing to do. Given that there is a already a syncdb being called as part of the flush, my initial reaction is that you shouldn't need another one. This may mean that there are some other modifications that need to be made; I'm not opposed to making such changes, if they're required.

If an additional call to syncdb is completely unavoidable, then so be it; however, I'm not yet convinced that it is unavoidable. Feel free to convince me :-)

comment:13 in reply to: ↑ 10 Changed 5 years ago by russellm

Replying to julien:

The only thing that was requested and which I haven't done is unloading the test apps from the AppCache. But, is it really necessary?

Again, feel free to convince me otherwise, but my initial reaction is "yes". Tests shouldn't have side effects after their execution; stale entries sticking around the app cache have the potential to introduce this sort of side effects.

comment:14 in reply to: ↑ 12 Changed 5 years ago by julien

Replying to russellm:

If an additional call to syncdb is completely unavoidable, then so be it; however, I'm not yet convinced that it is unavoidable. Feel free to convince me :-)

The proposed API allows one to add *any* app to INSTALLED_APPS, that is, some that may have already been synced (e.g. the common ones like contenttypes or auth) and some that probably haven't been synced yet (typically the app's internal test apps). Therefore I think that a synchronisation is necessary because we can't predict which apps have been synced or not yet.

You say that "there is a already a syncdb being called as part of the flush". This is not exactly true because my patch simply won't work without an explicit call to syncdb: flush does not create tables for the dynamically added apps which haven't been synced yet.

But even so, I don't think it would have such a latency impact. Assuming that this new API ever gets checked in, I presume only (or mostly) the contrib apps would bother using it so they become more self-contained (I'm talking in the context of the Django test suite). And, with the boost improvement scheduled for 1.1 (all tests run in one transaction), these considerations might well be negligible anyway.

Finally (and maybe more importantly) I am not sure how to go without syncdb :) Any hint?

comment:15 Changed 5 years ago by julien

Replying to russellm:

Again, feel free to convince me otherwise, but my initial reaction is "yes". Tests shouldn't have side effects after their execution; stale entries sticking around the app cache have the potential to introduce this sort of side effects.

I don't know if I can convince you for this one either, but I have one question: if the tests introduce such side effects, wouldn't that mean that the AppCache is buggy in the first place?

The test framework already relies on many conventions. As long as one sticks to those (and assuming the AppCache is not buggy), then I tend to think that everything would go fine. Obviously, if one is not careful enough and does not follow the conventions, then things could crash miserably.

Also, considering the proposed API, how would we know which app to unload, since we may include some apps that have already been loaded by the test suite (e.g. the most common ones like contenttypes or auth).

All these considerations depend on whether on not the suggested API is adequate. You haven't commented much on that yet. Could you share your thoughts on the quality of the API and how it could be improved?

comment:16 Changed 5 years ago by anonymous

  • Cc martin@… added

comment:17 Changed 5 years ago by anonymous

  • Cc oliver@… added

comment:18 Changed 5 years ago by gonz

  • Cc gonz added

comment:19 Changed 5 years ago by kkubasik

  • Owner changed from nobody to kkubasik

comment:20 Changed 5 years ago by bmathieu

The following is not a proposed changed since it is a hack, but here is what I have done to have models during testing:

In my app I have a folder "tests". It contains "testingmodels.py". In my test case (defined in tests/init.py) I have redefined _pre_setup() like this:

import sys
import new
from django.db.models import loading
from django.core.management import call_command
from django.test import TestCase

from . import testingmodels

class MyTestCase(TestCase)

    def _pre_setup(self):
        # register tests models
        # we'll fake an app named 'my_test_app'
        module = new.module('my_test_app')
        module.__file__ = __file__ # somewhere django looks at __file__. Feed it.
        module.models = testingmodels
        testingmodels.__name__ = 'my_test_app.models'
        sys.modules['my_test_app'] = module

        # register fake app in django and create DB models
        from django.conf import settings
        settings.INSTALLED_APPS += ('my_test_app',)
        loading.load_app('my_test_app')
        call_command('syncdb', verbosity=0, interactive=False)

        return super(MyTestCase, self)._pre_setup()

Note: due to django constraints the app must contains an empty "models.py".

I'm using this with success under django 1.0.2. I can't say I would recommend it as a general solution, but maybe it can help or inspire someone?

comment:21 Changed 5 years ago by julien

Thanks for this. I thought I'd also remind here the hack (originally posted at http://groups.google.com/group/django-users/browse_thread/thread/559aa0a2d074a7b5) which I've successfully used in many applications, and which is reasonably succinct in terms of lines of code. This is in fact what served as a model for the patch I've posted in this ticket. Maybe that will be useful to someone until this ticket eventually gets fixed.

Sample file structure:

    myapp/
        tests/
            fakeapp/
                __init__.py
                models.py
            __init__.py
            models.py
            views.py
            urls.py

Here is the testing code (located in myapp/tests/__init__.py):

    import sys

    from django.test import TestCase
    from django.conf import settings
    from django.core.management import call_command
    from django.db.models.loading import load_app
    
    from fakeapp.models import FakeItem
    
    class TestMyApp(TestCase):
    
        def setUp(self):
            self.old_INSTALLED_APPS = settings.INSTALLED_APPS
            settings.INSTALLED_APPS = (
                'django.contrib.auth',
                'django.contrib.contenttypes',
                'myapp',
                'myapp.tests.fakeapp',
            )
            load_app('myapp.tests.fakeapp')
            call_command('syncdb', verbosity=0, interactive=False) #Create tables for fakeapp

        def tearDown(self):
            settings.INSTALLED_APPS = self.old_INSTALLED_APPS

        def test_blah(self):
            item = FakeItem.objects.create(name="blah")
            #Do some testing here...

comment:22 Changed 5 years ago by anonymous

  • Cc t.django@… added

comment:23 Changed 4 years ago by steingrd

  • Cc steingrd@… added

comment:24 Changed 3 years ago by carljm

So, I'm a little surprised this hasn't been mentioned here already, which makes me wonder if I'm missing something obvious, but: in the process of checking out #14677 (and associated django-users post, https://groups.google.com/group/django-users/browse_thread/thread/3574e98ac6d3a774/f9bfde1741155272), it appears to me that we already have a pretty good working solution for test-only models in trunk (and I'm wondering why I never thought of it). Apparently you can simply define models directly in your tests.py. Syncdb never imports tests.py, so those models won't get synced to the normal db, but they will get synced to the test database, and can be used in tests. I haven't done this extensively myself (yet), but I just tested and it works. Problem solved?

The only difference I can see between that and the new feature being discussed here is whether the test-only models live in the same app as the one under test, or in a special test-only app. But I can't think of any reasons having them in the same app would be a problem. It even seems potentially a bit cleaner.

Of course, we still may want a fix for #14677, then; which parallels #4470, which we're hoping will be fixed when Arthur Koziel's GSOC branch is merged to fix #3591? I don't know whether that branch will otherwise impact this strategy for test-only models.

comment:25 Changed 3 years ago by Ciantic

I don't know what is the current suggested syntax for this, but either way it should be defined inside the app, and not like in the example of description. Probably it should be in the __init__.py of app, with something like:

APP_LABEL='myauth'

I mean it should not be variable.

E.g. if we allow the app_label be variable, what happens to all permission checks? Someone installs the auth app as myauth and suddenly all permission checks stops working.

comment:26 Changed 3 years ago by Ciantic

Uh wrong ticket guys, sorry, I was talking about #3591.

comment:27 Changed 3 years ago by russellm

@carljm; you're right that it hasn't been mentioned, but it hasn't been neglected, either.

To my mind, there are (a least) three features missing from the "put models in tests.py" technique:

  • Specify only specific models for a specific test (e.g., only have the Article model for this particular test)
  • Cleanup of contenttypes, permissions -- and most importantly -- the app cache itself.
  • Defining multiple test apps. For example, to test the admin, you need multiple apps to demonstrate the app index works.

My original motivation here was to provide a test environment that was rich enough to support tests for model migrations. For that, you need all three features.

comment:28 Changed 3 years ago by julien

To second Russell's comment, my own personal motivation with this ticket were (and still are) to reproduce all the conditions necessary to test the dependency of an app with another fictional, self-contained, app (i.e. not just to test the other apps' models, but also its urls, its views, its templatetags, etc.).

For what's it's worth, I've actually successfully used the hack above in multiple production apps (e.g. http://github.com/glamkit/glamkit-blogtools/blob/master/blogtools/tests/__init__.py#L70). The main theoretical thing that's missing for me is a proper cleanup of the appcache to make sure that nothing goes ugly while running the rest of your project's test suite (although in practice I've never run into anything problematic so far).

comment:29 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:30 Changed 3 years ago by kmike

  • Cc kmike84@… added
  • Easy pickings unset
  • UI/UX unset

comment:31 Changed 2 years ago by gonz

  • Cc gonz removed

comment:32 follow-up: Changed 7 months ago by adrian_lc

With 1.6 and the new discovery-runner the models in tests.py are not being installed anymore, so this feature would be way more useful now.

Just leaving an idea here, what about a decorator just like override_settings but for installing models?

The usage would be like:

class SomeTestModel(models.Model):
    # whatever


class AnotherTestModel(models.Model):
    # whatever


class YetAnotherTestModel(models.Model):
    # whatever


@test_models(SomeTestModel)
class SomeModelsTestCase(TestCase):

    @test_models(AnotherTestModel)
    def test_them(self):
        with test_models(YetAnotherTestModel):
            # The 3 models available here

Of course this wouldn't actually install the app, just models for testing custom fields or abstract models.

I actually have some code done but it's far from being correctly tested and I don't think I am qualified enough to provide a good implementation.

Anyway, anticipating some issues I encountered:

  • Main problem was the automatic rollback from TestCase. My database (postgres) rejects transactions with changes to a table followed by a table drop.
  • When applied to a test method (or as a context manager), I couldn't find a way to detect if the test function comes from a TransactionTestCase or a TestCase.
  • Finally managed something with restore_transaction_methods and disable_transaction_methods from django.test.testcases.
Last edited 7 months ago by adrian_lc (previous) (diff)

comment:33 Changed 7 months ago by adrian_lc

  • Cc adrianlopezcalvo@… added

comment:34 in reply to: ↑ 32 ; follow-up: Changed 2 months ago by JocelynD

Replying to adrian_lc:

With 1.6 and the new discovery-runner the models in tests.py are not being installed anymore, so this feature would be way more useful now.

I don't get it, I did a minimal test case (attached) with django 1.6:

  • a "buggy" project
  • a "tested" app
  • tested/models.py contains a single abstract model AbstractTestStat
  • tested/tests.py contains a TestStat model inheriting from AbstractTestStat
  • tested/tests.py contains a single test testing TestStat

Either

./manage.py test

or

./manage.py test tested

Works fine, populating the test db and succeeding test as expected.

Changed 2 months ago by JocelynD

comment:35 Changed 2 months ago by JocelynD

However, I don't know why but when using south, the "Test Models in tests.py" trick does not work (table is not created), so you have to put in your settings.py the following :

SOUTH_TESTS_MIGRATE = False

to disable the south migrations for the test db.

(which will anyway save you some cpu time...)

comment:36 in reply to: ↑ 34 Changed 4 weeks ago by glassglaze@…

Replying to JocelynD:

I don't get it, I did a minimal test case (attached) with django 1.6:

  • a "buggy" project
  • a "tested" app
  • tested/models.py contains a single abstract model AbstractTestStat
  • tested/tests.py contains a TestStat model inheriting from AbstractTestStat
  • tested/tests.py contains a single test testing TestStat

Works fine, populating the test db and succeeding test as expected.

It seems like you don't interact with db (save models to test db). I didn't manage to run tests. Do you think I've missed something.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from kkubasik to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.