1.6 compatibility checks don't correctly validate TEST_RUNNER

[Russell Keith-Magee]

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.

[Shai Berger]

Note similar discussion on mailing list: ​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.

[Carl Meyer]

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.

[Russell Keith-Magee]

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

[Carl Meyer]

Hi Russ - I think I must be missing something key, because I'm having trouble making sense of your response :-)

Replying to 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.

[Russell Keith-Magee]

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

[Carl Meyer]

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.

[Russell Keith-Magee]

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.

[Russell Keith-Magee]

Also.. this is starting to get a bit detailed -- we should probably pull this out into a django-dev thread...

[Carl Meyer]

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.

[Aymeric Augustin]

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?

[Shai Berger]

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.

[thepapermen]

Maybe this could be relevant to the discussion: Ruby on Rails has a special "​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?

[Carl Meyer]

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.

[Russell Keith-Magee]

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

[Russell Keith-Magee <russell@…>]

In 7f0fdffd07df71409fc44a3a039c25bf01ca1310:

[1.6.x] Refs #21197 -- Clarified upgrade check message.

Thanks to Carl and Shai for the discussion.

Backport of 8ff4303 from master.

[Russell Keith-Magee <russell@…>]

In 8ff43039466f5e3c78d3587fdbe229ed44c4e1aa:

Refs #21197 -- Clarified upgrade check message.

Thanks to Carl and Shai for the discussion.

[Tim Graham]

I guess we are probably not going to revisit this at this point.