Code

Opened 2 years ago

Closed 20 months ago

#17966 closed Bug (fixed)

Tests fail with trivial usage of AUTH_PROFILE_MODULE

Reported by: slacy Owned by: nobody
Component: contrib.auth Version: 1.4
Severity: Normal Keywords:
Cc: tiramiseb, rob@…, taylor@…, osirius@…, scotty@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I started a totally blank project using 1.4 release (Version in Ticket in 1.4-rc2 because there's no 1.4 release in the ticket system yet).

I added a new app called "account" with a models.py that looks like this:

from django.db import models
from django.contrib.auth.models import User
    
class Account(models.Model):
    user = models.ForeignKey(User)

I put "account" in INSTALLED_APPS
I put "AUTH_PROFILE_MODULE = 'account.Account'" in settings.py

I ran python ./manage.py test and got:

ERROR: test_site_profile_not_available (django.contrib.auth.tests.models.ProfileTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/slacy/src/tmp/env/local/lib/python2.7/site-packages/django/contrib/auth/tests/models.py", line 29, in test_site_profile_not_available
    del settings.AUTH_PROFILE_MODULE
  File "/home/slacy/src/tmp/env/local/lib/python2.7/site-packages/django/utils/functional.py", line 215, in __delattr__
    delattr(self._wrapped, name)
AttributeError: AUTH_PROFILE_MODULE

----------------------------------------------------------------------
Ran 417 tests in 10.302s

FAILED (errors=1, skipped=1)
Destroying test database for alias 'default'...

Attachments (1)

17966-1.diff (2.2 KB) - added by claudep 2 years ago.
Possible fix

Download all attachments as: .zip

Change History (30)

comment:1 Changed 2 years ago by slacy

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to aagugstin
  • Patch needs improvement unset

I've used git bisect to narrow this down to commit(s):

e669c483ffcd8313b51a43f602ba2c8c74fd78bd
c88db46afdcacef8504c5831254b6d5592ac0cf0

Both of which are by aaugustin

comment:2 Changed 2 years ago by slacy

  • Owner changed from aagugstin to aaugustin

comment:3 Changed 2 years ago by slacy

  • Version changed from 1.4-rc-2 to 1.4

comment:4 Changed 2 years ago by aaugustin

  • Owner changed from aaugustin to nobody

I'm probably going to take a look at this, but the common practice here is *not* to assign tickets to other people. Assigning a ticket to oneself means "I'm currently working on a patch", and in this case, it isn't true.

Changed 2 years ago by claudep

Possible fix

comment:5 Changed 2 years ago by aaugustin

  • Component changed from Uncategorized to contrib.auth
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

I can see how this happened -- mixing override_settings and code to achieve the same effect manually in setUp and tearDown probably isn't a good idea.

We should refactor the test suite to use override_settings consistently. This is a much broader effort than fixing this specific bug.

It's also a n-th instance of the usual problem: "tests of contrib apps aren't sufficiently isolated"...

comment:6 Changed 2 years ago by rob@…

I'm experiencing this issue also, on a completely blank project with AUTH_PROFILE_MODULE set.

comment:7 Changed 2 years ago by tiramiseb

  • Cc tiramiseb added

comment:8 Changed 2 years ago by rob@…

  • Cc rob@… added

comment:9 Changed 2 years ago by taylorfausak

  • Cc taylor@… added

comment:10 follow-ups: Changed 2 years ago by SamBull

  • Cc osirius@… added

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

comment:11 in reply to: ↑ 10 Changed 2 years ago by rob@…

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

I'm in the same situation, and would really like to migrate. I've been trying to figure out a workaround for this, and can't get the test to pass. I certainly don't want to use a custom version of Django in production.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 2 years ago by aaugustin

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

You should only test your own apps, not Django's contrib apps: ./manage.py test myapp1 myapp2 ...

comment:13 in reply to: ↑ 12 ; follow-up: Changed 2 years ago by SamBull

Replying to aaugustin:

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

You should only test your own apps, not Django's contrib apps: ./manage.py test myapp1 myapp2 ...

Huh. That's not true is it? Some of django's tests are integration tests that help us verify that we're using everything correctly (providing essential templates, settings, etc.).

Is there any way to set the AUTH_PROFILE_MODULE setting without causing this test to error out?

Also, I take it this failure isn't getting caught by django's test suite, so shouldn't that be updated too?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 2 years ago by carljm

Replying to SamBull:

Replying to aaugustin:

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

You should only test your own apps, not Django's contrib apps: ./manage.py test myapp1 myapp2 ...

Huh. That's not true is it? Some of django's tests are integration tests that help us verify that we're using everything correctly (providing essential templates, settings, etc.).

That was the theory at one time, but it doesn't work very well in practice. There are so many ways to integrate contrib or other third-party apps, oftentimes with customizations, wrapped views, etc., and it's almost impossible for a reusable-app author to write tests that will reliably pass with all functional customized integrations. Over time, as "failing test" reports (just like this one) come in against contrib apps and are fixed, the tests become more and more isolated from the settings of the project they are running in (provide their own templates, temporarily override settings to known values, etc), and eventually lose any value they may have once had as integration tests.

Fundamentally, integration tests are the responsibility of the integrator, who is the only party who knows how their specific integration is supposed to work.

For Django 1.5 it is likely that the default test runner will not run tests from external apps by default. So yes, I would say that changing your continuous integration tests now to match that practice would be sensible, and I don't believe that in practice you'll lose significant value in terms of coverage of your code and settings.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 2 years ago by rob@…

Replying to carljm:

Replying to SamBull:

Replying to aaugustin:

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

You should only test your own apps, not Django's contrib apps: ./manage.py test myapp1 myapp2 ...

Huh. That's not true is it? Some of django's tests are integration tests that help us verify that we're using everything correctly (providing essential templates, settings, etc.).

That was the theory at one time, but it doesn't work very well in practice. There are so many ways to integrate contrib or other third-party apps, oftentimes with customizations, wrapped views, etc., and it's almost impossible for a reusable-app author to write tests that will reliably pass with all functional customized integrations. Over time, as "failing test" reports (just like this one) come in against contrib apps and are fixed, the tests become more and more isolated from the settings of the project they are running in (provide their own templates, temporarily override settings to known values, etc), and eventually lose any value they may have once had as integration tests.

Fundamentally, integration tests are the responsibility of the integrator, who is the only party who knows how their specific integration is supposed to work.

For Django 1.5 it is likely that the default test runner will not run tests from external apps by default. So yes, I would say that changing your continuous integration tests now to match that practice would be sensible, and I don't believe that in practice you'll lose significant value in terms of coverage of your code and settings.

I find that often the strange edge-cases are caught by running the tests in contrib, which my own tests may not necessarily cover (though this should obviously be rectified, if such a situation is discovered).

For example, the behaviour of custom middleware on login/logout views. I'd feel somewhat uncomfortable excluding Django's tests from the build (is there even a way to do this - or must you specify each of your own apps explicitly on the command line?)

comment:16 in reply to: ↑ 15 Changed 2 years ago by carljm

I replied on #17365, as this discussion is really more on-topic there.

comment:17 Changed 22 months ago by scotty@…

  • Cc scotty@… added

comment:18 Changed 22 months ago by robgolding63

I've opened a pull request with a fix for this one: https://github.com/django/django/pull/36.

I know the use of GitHub is new to the Django workflow and some things still need to be worked out, but I figured having it there waiting is the easiest way to show the changes. It's just a single commit, which I think fits with Django's current procedures.

comment:19 follow-up: Changed 22 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In [531878302735e6a2b36d82b584947bbf8eae8111]:

Fixed #17966 -- Isolated ProfileTestCase from custom AUTH_PROFILE_MODULE

Thanks Rob Golding for helping on the patch.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 22 months ago by dokterbob

Replying to Claude Paroz <claude@…>:

In [531878302735e6a2b36d82b584947bbf8eae8111]:

Fixed #17966 -- Isolated ProfileTestCase from custom AUTH_PROFILE_MODULE

Thanks Rob Golding for helping on the patch.

The patch seems to fix the initial problem but the tests now fail due to SiteProfileNotAvailable now not being raised in my specific setup. It seems as though override_settings is not doing quite what it ought to do:

  • With AUTH_PROFILE_MODULE commented out in my settings, the test runs fine.
  • When I comment out the full @override_settings line the test runs fine.
  • When I remove the AUTH_PROFILE_MODULE override, the test fails with AttributeError: AUTH_PROFILE_MODULE as in the original bug report.
  • When I remove the USE_TZ override, I get the SiteProfileNotAvailable exception not being raised.

No doubt this is a weird and difficult problem. Let me know if more feedback is required - I personally have no real clue what's going on here and do not have the time to delve much deeper but would love to help this bug get fixed.

Lastly, it seems that Rob Golding's original patch from the pull request passes the unittest just fine.

comment:21 Changed 22 months ago by claudep

Please provide the necessary settings to be able to reproduce your problem. Please try also to debug the test to see why SiteProfileNotAvailable is not raised.

comment:22 in reply to: ↑ 20 Changed 20 months ago by anonymous

Replying to dokterbob:

Replying to Claude Paroz <claude@…>:

In [531878302735e6a2b36d82b584947bbf8eae8111]:

Fixed #17966 -- Isolated ProfileTestCase from custom AUTH_PROFILE_MODULE

Thanks Rob Golding for helping on the patch.

The patch seems to fix the initial problem but the tests now fail due to SiteProfileNotAvailable now not being raised in my specific setup. It seems as though override_settings is not doing quite what it ought to do:

  • With AUTH_PROFILE_MODULE commented out in my settings, the test runs fine.
  • When I comment out the full @override_settings line the test runs fine.
  • When I remove the AUTH_PROFILE_MODULE override, the test fails with AttributeError: AUTH_PROFILE_MODULE as in the original bug report.
  • When I remove the USE_TZ override, I get the SiteProfileNotAvailable exception not being raised.

No doubt this is a weird and difficult problem. Let me know if more feedback is required - I personally have no real clue what's going on here and do not have the time to delve much deeper but would love to help this bug get fixed.

Lastly, it seems that Rob Golding's original patch from the pull request passes the unittest just fine.

Just wanted to note that I'm having the same issue on Python 2.7, Django 1.5.0. What seems to be occurring is that prior to line 15: del settings.AUTH_PROFILE_MODULE, settings.AUTH_PROFILE_MODULE reports itself as an empty string. However, after the del call, settings.AUTH_PROFILE_MODULE reports itself as the model defined in my settings.py. As a result, user.get_profile() is able to run fine and thus causes a test failure by not raising an error.

I was able to replicate the issue by doing the following:

At this point, running python manage.py test exhibits the problematic behavior described above where settings.AUTH_PROFILE_MODULE defaults to what I defined in settings.py after the attribute is deleted.

comment:23 Changed 20 months ago by claudep

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:24 Changed 20 months ago by claudep

This problem is a symptom of #18824.

comment:25 Changed 20 months ago by robgolding63

As far as I can see, moving the separate tests to their own functions would remove the need to use del settings.AUTH_PROFILE_MODULE, and so would work around this issue. As it stands there are 3 tests sitting in the same function (test_site_profile_not_available), so it probably makes more sense to break them out?

comment:26 Changed 20 months ago by claudep

I don't think so. AUTH_PROFILE_MODULE can be set in the current project settings and therefore I don't see how we can avoid the del. However, you have a point in that the del would probably succeed if the test was not in an override_settings context. But then we should also restore it at the end of the test, exactly the sort of manipulation we want to avoid with override_settings. That's why I still favour the solution proposed in #18824.

comment:27 Changed 20 months ago by robgolding63

My thinking was that override_settings is a class decorator, so by having AUTH_PROFILE_MODULE set to the empty string (or None), it's effectively unset at the start of each method call. We could then set it as required, or just leave it unset in this particular case.

Perhaps the only way that would work, though, is if the tests were in separate classes altogether - which isn't exactly ideal.

In any case, implementing #18824 will no doubt be useful in other places too - so that's probably the best course of action.

comment:28 Changed 20 months ago by alan.johnson@…

I'm a rather new Django developer, and I'm trying to build a site in a test-driven way...is the takeaway of this that tests will always fail when following the UserProfile instructions from the docs? Is there a workaround?

comment:29 Changed 20 months ago by claudep

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

This should be resolved after commit [22f85b9057825b5b4139abf5fd7e8c4ba0d16981].

@alan.johnson@…:
See comment:12

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.