I can see 5 scenarios that need to be accounted for:
1. A default pre-Django 1.6 project
2. A pre-Django 1.6 project with TEST_RUNNER set to DiscoverRunner
3. A pre-Django 1.6 project with TEST_RUNNER set to something custom
4. A default Django 1.6 project
5. A Django 1.5 project with TEST_RUNNER set to something custom
The management command does the following:
{{{
from django.conf import settings
new_default = 'django.test.runner.DiscoverRunner'
test_runner_setting = getattr(settings, 'TEST_RUNNER', new_default)
if test_runner_setting == new_default:
... output warning ...
}}}
That is, it retrieves the current setting value of TEST_RUNNER, and if it
is DiscoverRunner, it raises a warning. This behavior:
* Is correct for case 1
* Is possibly incorrect for case 3 and 5, as there's no guarantee the
custom test runner subclasses DiscoverRunner (and it probably won't)
* Incorrectly raises an error for case 2 and 4.
There are test cases for the current implementation, but they aren't very
solid - they're operating inside a test environment that is itself force
setting TEST_RUNNER.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Old description:
> The 1.6 compatibility checks (executed by the new check management
> command) don't appear to perform an accurate check.
>
> I can see 5 scenarios that need to be accounted for:
>
> 1. A default pre-Django 1.6 project
> 2. A pre-Django 1.6 project with TEST_RUNNER set to DiscoverRunner
> 3. A pre-Django 1.6 project with TEST_RUNNER set to something custom
> 4. A default Django 1.6 project
> 5. A Django 1.5 project with TEST_RUNNER set to something custom
>
> The management command does the following:
> {{{
> from django.conf import settings
> new_default = 'django.test.runner.DiscoverRunner'
> test_runner_setting = getattr(settings, 'TEST_RUNNER', new_default)
>
> if test_runner_setting == new_default:
> ... output warning ...
> }}}
>
> That is, it retrieves the current setting value of TEST_RUNNER, and if it
> is DiscoverRunner, it raises a warning. This behavior:
> * Is correct for case 1
> * Is possibly incorrect for case 3 and 5, as there's no guarantee the
> custom test runner subclasses DiscoverRunner (and it probably won't)
> * Incorrectly raises an error for case 2 and 4.
>
> There are test cases for the current implementation, but they aren't very
> solid - they're operating inside a test environment that is itself force
> setting TEST_RUNNER.
New description:
The 1.6 compatibility checks (executed by the new check management
command) don't appear to perform an accurate check.
I can see 5 scenarios that need to be accounted for:
1. A default pre-Django 1.6 project
2. A pre-Django 1.6 project with TEST_RUNNER set to DiscoverRunner
3. A pre-Django 1.6 project with TEST_RUNNER set to something custom
4. A default Django 1.6 project
5. A Django 1.6 project with TEST_RUNNER set to something custom
The management command does the following:
{{{
from django.conf import settings
new_default = 'django.test.runner.DiscoverRunner'
test_runner_setting = getattr(settings, 'TEST_RUNNER', new_default)
if test_runner_setting == new_default:
... output warning ...
}}}
That is, it retrieves the current setting value of TEST_RUNNER, and if it
is DiscoverRunner, it raises a warning. This behavior:
* Is correct for case 1
* Is possibly incorrect for case 3 and 5, as there's no guarantee the
custom test runner subclasses DiscoverRunner (and it probably won't)
* Incorrectly raises an error for case 2 and 4.
There are test cases for the current implementation, but they aren't very
solid - they're operating inside a test environment that is itself force
setting TEST_RUNNER.
--
Comment (by shai):
Note similar discussion on mailing list:
[https://groups.google.com/forum/#!topic/django-developers/reoiSYbPviQ
Django 1.6 and migrating to the new test runner: note on user experience].
The discussion is not about the "check" command, but essentially asking to
incorporate some "check" functionality into the "test" command.
Note in particular that my suggestion to add a settings knob specifying
the Django version the project is expecting (which would default to 1.5,
with 1.6 in the default project templates) should also allow easy
resolution of this bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:1>
Comment (by carljm):
For cases 2, 3 and 5, if someone has a custom `TEST_RUNNER` set, the
changed default test runner has no impact on them, so there is no point in
warning. Any changes they will ever see will be due to their own changes
to the `TEST_RUNNER` setting, not due to upgrading Django. So I think the
handling there is correct.
That leaves case 4. Basically I think we need a setting like the one shai
mentions in order to handle this correctly, if the "check" command is
intended to be something that you can run anytime and should give you a
clean slate. When this check was added, the check command was envisioned
as something you'd run once at upgrade time that would alert you to
anything you might need to know about, so false alarms on fresh 1.6
projects were not considered a big problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:2>
Comment (by russellm):
@carljm -- I'm not sure I agree on case 2,3 and 5.
Consider case 2. A user has an old 1.5 project. They run check, and they
get the error. They update their project. They run check again, and they
get the same error. As an end user, this could be easily interpreted as
"Whatever I did didn't work, so what do I need to fix".
In case 3, they're always going to get the error. At the very least, there
should be a *different* error message, saying "Django doesn't know what to
do here"; but it should be possible to do some issubclass checks.
Case 5 *should* never happen -- there's no reason to run check on a 1.6
project -- but if you do, you're going to get an error. Looking longer
term (and the reason why this came to my attention), the GSoC validation
project means this check will be run as part of the prestart checks for
syncdb and runserver, so it's important that we can correctly identify
that there's nothing wrong with the current project.
The crux of this problem is that we can't tell the difference between
"Letting the default fall through" and "Explicitly set". If we modify the
global_setting default to something identifiable, we have a way around
this problem. I should have a chance to look at this today; I'll report
back with a patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:3>
Comment (by carljm):
Hi Russ - I think I must be missing something key, because I'm having
trouble making sense of your response :-)
Replying to [comment:3 russellm]:
> Consider case 2. A user has an old 1.5 project. They run check, and they
get the error. They update their project. They run check again, and they
get the same error. As an end user, this could be easily interpreted as
"Whatever I did didn't work, so what do I need to fix".
How would a user "run check and get the error" on "an old 1.5 project",
considering that this check didn't exist in 1.5?
When you say a "A pre-Django 1.6 project with TEST_RUNNER set to
DiscoverRunner" I made the assumption that you were talking about the
externally-packaged `DiscoverRunner`, because the internal-to-Django
`DiscoverRunner` does not exist pre-1.6. If you're talking about the
externally-packaged one, its users obviously use a different `TEST_RUNNER`
setting, so they wouldn't get the error. If you're talking about Django's
new `DiscoverRunner`, I'm not sure how case 2 is possible.
> In case 3, they're always going to get the error. At the very least,
there should be a *different* error message, saying "Django doesn't know
what to do here"; but it should be possible to do some issubclass checks.
It seems like you've got the sense of the "if" check backwards? Case 3 is
"A pre-Django 1.6 project with TEST_RUNNER set to something custom" -- if
`TEST_RUNNER` is set to something custom, they will never get the error.
The error only occurs if `TEST_RUNNER` is at its default value, the reason
being that the check is supposed to catch people who are just using the
default test runner, for whom the default will change when they upgrade to
1.6.
> Case 5 *should* never happen -- there's no reason to run check on a 1.6
project -- but if you do, you're going to get an error. Looking longer
term (and the reason why this came to my attention), the GSoC validation
project means this check will be run as part of the prestart checks for
syncdb and runserver, so it's important that we can correctly identify
that there's nothing wrong with the current project.
Again, in case 5 they will never see the error.
It's case 4 that is a problem, and the problem is simply that this check
was not written for a world where it would be run constantly.
> The crux of this problem is that we can't tell the difference between
"Letting the default fall through" and "Explicitly set". If we modify the
global_setting default to something identifiable, we have a way around
this problem. I should have a chance to look at this today; I'll report
back with a patch.
Although I think it's case 4, not 2/3/5, that need attention, I agree that
this is the crux of the immediate problem. It's a problem we've run into
before and will run into again in similar situations, so I'd prefer to see
a generic resolution than one that only works for the `TEST_RUNNER`
setting.
I also think shai has correctly identified a different angle on the
problem, which is that it would be nice to have a "this project currently
expects Django version X" setting, so that if we are running checks in
Django 1.6 on a project set to expect 1.5, we can warn about anything that
might be an issue for that upgrade without having to always be so clever
about trying to figure out their exact configuration. Once they've dealt
with those warnings (if necessary), they can simply update their settings
to say "expects Django 1.6" and never see any of those warnings again.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:4>
Comment (by russellm):
@carljm: I've gone back and read what I wrote -- I made a couple of
mistakes in my specific assertions, caused by me reporting results from a
version of the check command that I was in the middle of "fixing".
Apologies for the confusion.
However, I think my broad point is still valid. Let me try again.
Here's my test case. Broadly, I'm trying to enumerate every possible case
for upgrading to 1.6, or starting a new project under 1.6.
* Set up a virtualenv, Install Django 1.5.
* Create four test projects (testA, testB, testC and testD), make the
minimal settings changes necessary to have a sqlite database. Run the test
suite to make sure it works.
* Update virtualenv to Django 1.6
* Modify testB settings to define
TEST_RUNNER='django.test.runner.DiscoverRunner'
* Modify testC settings to define
TEST_RUNNER='django.test.simple.DjangoTestSuiteRunner'
* Modify testD settings to define
TEST_RUNNER='something.custom.TestRunner'
* Create two more test projects (testE and testF)
* Modify testF settings to define
TEST_RUNNER='something.custom.TestRunner'
Now, go into each project and run ./manage.py check.
* testA raises an error.
* testB raises an error, even though the project has been fixed by an
explicit setting.
* testC doesn't raise an error.
* testD doesn't raise an error; this may be masking an actual problem, as
we don't know if something.custom.TestRunner extends DiscoverRunner or
something else.
* testE raises an error, even though the project is fine (by definition)
* testF doesn't raise an error; this may be masking an actual problem, as
we don't know if something.custom.TestRunner extends DiscoverRunner or
something else.
It's testB and testE that are the concerning cases to me, because they
give an error even though no error exists - testB because the setting has
been explicitly set; testE because the project is, by definition, up to
date. testA is an interesting case, because the project might be valid; we
have no way to know if the user has fixed the underlying problem, but
there's no way for the user to silence the error, or validate that the
change they've made will actually fix anything.
testD and testF are problematic because we aren't reporting any potential
problems with the project -- even the problems that are trivially
identified by doing a subclass check.
The reason this has hit my radar is that I'm integrating the GSoC work.
This involves merging the validate and check management commands, with the
side effect that the TEST_RUNNER check is run on every syncdb and
runserver execution. It's also run a dozen or more times during the
execution of the test suite. And every once of these uses raises the error
message, even if you've got a Django 1.6 project, or you're running on the
Django 1.6 test suite which explicitly sets TEST_RUNNER. The GSoC work has
a way to silence specific errors/warnings, but the current coder requires
that we ship a project template that explicitly silences an upgrade
warning, which seems odd to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:5>
Comment (by carljm):
Hi Russ - this is all making much more sense now. I think really my only
concern is that I think an "upgrade warning" is in practice a very
different thing from a "project validation check", and I'm not sure it
will serve us well to conflate the two.
In my mind, an upgrade warning is where we want to draw to your attention
something that changed in Django that may require attention in your
project, but where we really can't determine in an automated way whether a
specific configuration is "broken" or "valid" - only you can determine
that. An upgrade warning is essentially a slightly-automated version of
the backwards-incompatible section of the release notes, to assist the
many who don't read them. An upgrade warning should err on the side of
being loud rather than quiet if we can't be sure whether you need to do
anything, and it should point you to release notes that more fully
describe what changed and where you may want to look for possible
brokenness. Because it errs on the side of being loud, an upgrade warning
shouldn't be run as a part of regular project validation checks, unless
there's an easy way to say "Ok, I'm satisfied my project upgraded well and
runs fine on Django 1.6, I don't want to see any Django 1.6 upgrade
warnings anymore" (something like Shai's "expects Django version"
setting). An upgrade warning is never an "error", it is only ever an
informational note.
A project validation check, on the other hand, should be quiet unless we
are pretty sure something is broken and needs to be fixed in your project.
I am concerned about trying to convert the TEST_RUNNER check, which was
written as an upgrade warning (i.e. it errs on the side of being loud, and
was expected to only be run manually at upgrade time), into a project
validation check. I don't think that a 1.6 project without an explicit
TEST_RUNNER setting is "broken" and needs to be "fixed" by adding an
explicit TEST_RUNNER setting, and I don't want to add a project validation
check that thinks the lack of an explicit TEST_RUNNER setting is an error.
I don't see how we can possibly decide in an automated way whether a
project with a custom TEST_RUNNER is "valid" or not, and I don't think
knowing whether said custom TEST_RUNNER subclasses `DiscoverRunner` helps
us to make such a determination at all.
So basically I am quite skeptical of the direction you seem to be heading,
which is to try to build a project validation check that attempts to
correctly determine in all cases whether a project's TEST_RUNNER setting
is "valid" or "in error". I don't think it is possible to do this with any
confidence. I think what we need to be doing instead is figuring out how
to handle "upgrade warnings" where we simply can't make reliable automated
determinations and just want to nudge possibly-affected upgrading users
towards checking out important info in the release notes. I'm not sure
this is a use case that was addressed by the GSoC work. At the moment,
something like Shai's "expected Django version" setting is the best of bad
options for this that I've seen suggested.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:6>
Comment (by russellm):
To be clear -- I'm not trying to say that the compatibility check should
be "smart"; I'm saying it shouldn't ever report an error that is wrong for
reasons we can predict. As it stands:
* The current error message says "You have not explicitly set
'TEST_RUNNER'", even if you have.
* There's no way to silence the error message; If you've made a change to
your project to correct the problem, the error is still reported.
* If you have a project created using Django 1.6, and you upgrade to
Django 1.7, you get a warning that TEST_RUNNER is a problem (which it
isn't)
I think the third point is the one that points out the problem best --
essentially, without version checks, ./manage.py check becomes "a
collection of everything in the last N release notes that you might need
to pay attention to". I'm not convinced *that* sort of command has value.
So - version tracking of some kind would seem to be necessary. I've taken
a swing at implementing the version check that Shai suggested.
https://github.com/freakboy3742/django/compare/version-check
This definitely fixes the problem. However, I'm not sure I like this as a
solution. Having an extra setting to track when a project was created
seems really messy to me. It's something we need to track, but a setting
doesn't feel like the right place.
There might be some crossover here with one of Jacob's complaints about
pluggable users -- that if an end user every modifies AUTH_USER_MODEL
after the first sync, hilarity will ensue. A setting isn't the right place
to track this, either; but you need to be able to store the "original"
AUTH_USER_MODEL value so you can warn if it's ever changed.. So - do we
introduce a new file that keeps these project configuration values? An
"internal" database table for tracking critical settings?
Looking forward to the GSoC: I'm not sure I fully pay your "upgrade
warning != project check" assertion, either. To my mind, an upgrade
warning is "a check that your project is compatible with the version of
Django it's running under".
The check command is a useful way to do this, but my concern is that this
command won't be executed by the people who most need to. The recent
django-dev thread about problems with migrating highlights this point for
me. I think there's a *huge* value in putting the upgrade checks somewhere
that you can't avoid running them.
This is one of the reasons the GSoC project merges the validate and two
commands. There are two key parts to the GSoC project that make this
approach viable IMHO.
Firstly, the GSoC project makes the distinction between INFO, WARNING,
ERROR and CRITICAL messages from the validation framework. All the old
validation errors are ERRORs - things like upgrade checks are WARNINGs.
(well... with one exception, but that's a different ticket). ERRORs and
CRITICALs will stop your project running. WARNINGs and INFOs will just be
noisy.
Secondly, the GSoC project adds the ability to silence any message you
get. This means that if you start getting a WARNING, and you are satisfied
that you've fixed the problem, you can add a value to settings to
explicitly say "I'm happy this isn't a problem", and you won't see it
again.
However, even with this, we need to have version checks -- otherwise,
we'll need to ship a Django 1.7 project template with "1.6 warnings
disabled" -- otherwise you'll be firing those warnings.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:7>
Comment (by russellm):
Also.. this is startingto get a bit detailed -- we should probably pull
this out into a django-dev thread...
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:8>
Comment (by carljm):
We can pull it out in a thread, but I don't have much more to say -
basically I think I agree with you that the upgrade checks are most useful
if they are run automatically, and that if we can add some sort of
version-checking to the new validate code, it can serve this purpose well.
I agree that settings are kind of ugly for the version check, but I am not
convinced there is another option that is better. Both "another file" and
"a database table" are, to my mind, much more intrusive additional project
cruft than another setting.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:9>
Comment (by aaugustin):
If I remember correctly, warning have "tags" or "categories". Could we
make "upgrade-to-1.5", "upgrade-to-1.6", etc. categories and make sure
these checks aren't performed by runserver & friends?
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:10>
Comment (by shai):
Here are some alternatives to a setting. I don't think any of them are
better -- I am bringing them up more as a devil's advocate -- but I hope
they can serve to push this discussion forward.
* The name of the settings module; that is, if it matches the pattern
`*_1_6`, assume the project is designed for Django 1.6.X, etc. Allows a
single code base to support separate Django versions easily. Very un-
pythonic, IMO.
* A special module that, when imported, disables upgrade checks to the
specific version. Thus, if the settings module (or any other module)
imports `django.core.checks.ready_for_1_6`, checks for upgrade to 1.6 are
disabled. As far as I can see, this is just an oddly-phrased setting.
* A special module whose presence in the PYTHONPATH disables specific
version upgrade checks. Thus, `touch django_1_6_ready.py` in a proper
folder, or even `pip install django-1-6-ready` in a virtualenv would
disable the relevant checks. Still implies ugly content in default project
templates.
* An environment variable. Makes very little sense -- the thing to record
is a property of the code, not a deployment parameter.
* (Russell's idea) A database table tracking upgrades. Fails completely
if no database is used.
* (Russell's idea) A separate file tracking upgrades. Even messier than a
setting.
I'm out of bad ideas for now.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:11>
Comment (by thepapermen):
Maybe this could be relevant to the discussion: Ruby on Rails has a
special "[http://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html
#upgrading-from-rails-3-2-to-rails-4-0 Guide for Upgrading Ruby on Rails]"
page, which has sections that are dedicated to guiding users on how they
should upgrade the framework from one specific version to another specific
version. Maybe a special page in the docs and a setting could be good
enough?
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:12>
Comment (by carljm):
We already have that page in the docs:
https://docs.djangoproject.com/en/1.5/howto/upgrade-version/
If people read that page in the docs and followed its advice (which
prominently includes reading the release notes for every major version
upgrading across), we wouldn't be having this discussion at all :-) What
we're talking about here is automated help for people who try to upgrade
without reading the relevant docs.
I'm not really sure this ticket should be a 1.6 blocker at all. The
current upgrade-help situation isn't ideal, but we've made backwards-
incompatible changes before using our current documentation, without
automated help, and the ecosystem has lived through it just fine. I think
the upcoming system of automated checks will be a great help, but I'm not
convinced we should be blocking 1.6 on further preparing for it. The
current check in the "check" command is not perfect, but I also don't
think it's broken; given the context of when and how it would be run
(probably not very much), I don't think it's a problem for 1.6 as-is.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:13>
* severity: Release blocker => Normal
Comment:
@carljm - On reflection, I think I agree that this isn't a release
blocker. There's definitely going to need to be some discussion for 1.7
(around landing the GSoC patch, and/or how the 1.6 warnings will manifest
on a 1.7 upgrade), but I don't think this needs to block 1.6.
That said, I think a minor change in the wording of the error message is
called for; as currently phrased, the message is misleading if you've
upgraded and *have* set TEST_RUNNER to point at the new test runner.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:14>
Comment (by Russell Keith-Magee <russell@…>):
In [changeset:"7f0fdffd07df71409fc44a3a039c25bf01ca1310"]:
{{{
#!CommitTicketReference repository=""
revision="7f0fdffd07df71409fc44a3a039c25bf01ca1310"
[1.6.x] Refs #21197 -- Clarified upgrade check message.
Thanks to Carl and Shai for the discussion.
Backport of 8ff4303 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:15>
Comment (by Russell Keith-Magee <russell@…>):
In [changeset:"8ff43039466f5e3c78d3587fdbe229ed44c4e1aa"]:
{{{
#!CommitTicketReference repository=""
revision="8ff43039466f5e3c78d3587fdbe229ed44c4e1aa"
Refs #21197 -- Clarified upgrade check message.
Thanks to Carl and Shai for the discussion.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:16>
* status: new => closed
* resolution: => wontfix
Comment:
I guess we are probably not going to revisit this at this point.
--
Ticket URL: <https://code.djangoproject.com/ticket/21197#comment:17>