Code

Opened 3 years ago

Closed 3 years ago

#16342 closed Bug (duplicate)

Django unittests run against installed django by default (rather than the checkout)

Reported by: soren Owned by: nobody
Component: Testing framework Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

By default, tests/runtests.py will import various things from "django". This means that if you're lucky, you don't have a Django install in your standard PYTHONPATH and you'll get an error, realise the error of your ways, and you can amend your PYTHONPATH. If you're less lucky, runtests.py will happily import the *installed* django, and you will get very, very confused why tests are passing even though you know you broke something.

Attachments (1)

set_the_pythonpath.patch (484 bytes) - added by soren 3 years ago.
Patch to always prepend the top level srcdir to sys.path.

Download all attachments as: .zip

Change History (7)

Changed 3 years ago by soren

Patch to always prepend the top level srcdir to sys.path.

comment:1 Changed 3 years ago by soren

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Attached a patch to fix it. I've no clue how to test this (or whether to do so at all, really). Feedback happily accepted.

comment:2 Changed 3 years ago by soren

  • Summary changed from Django unittests run against install django by default (rather than the checkout) to Django unittests run against installed django by default (rather than the checkout)

comment:3 Changed 3 years ago by soren

I was just told about ticket #9947, which refers to the same issue.

I'd like to attack this problem somehow. I'm not familiar with the bug management rules here. If it's more appropriate to reopen the old one and ditch this one, that's fine. Feel free to close this.

I can vaguely relate to the desire to run new tests against old code.

For those of you who actually make use of this, am I correct to assume that this is less common than running new tests against new code? I mean, it seems more natural to me that development of both "real" code and tests happens in the code checkout, but I've been surprised before :)

Would everyone's needs be met if the default was to prepend the toplevel dir to sys.path, but there was a simple way to disable this behaviour (like setting an environment variable or something)?

comment:4 Changed 3 years ago by aaugustin

  • Patch needs improvement set

An alternative is to implement a warning when the tests are run against a version of Django that is not the local checkout. That might be acceptable by the core developers.

Technically, your ticket is a duplicate of another ticket that was closed by a core developer as wontfix. The standard procedure would be to close it and tell you to start a discussion on django-developers.

However, I think it's something that can easily turn off new contributors, so we should try to help them a little bit without hindering the workflow of the core developers. I'll try to get some feedback, and I don't close the ticket right now.

comment:5 Changed 3 years ago by PaulM

I agree that this turns off new developers, but it's an absolute non-issue for the more experienced devs. To the best of my knowledge, everyone who does much work on the codebase uses virtualenv or directly modifies their path variable to isolate their environment. This means that there isn't a system install of Django at all, and there's absolutely no chance that there is more than one valid "django" to import.

We should really be teaching new users to use virtualenv at the end of the tutorial. If we can't get it there, we should at least include it in the documentation on writing tests or working on Django.

Mucking around with the path in the test suite is likely to be a nonstarter. It's much better to let Python do the imports as expected, and require users to modify the environment themselves where necessary.

Last edited 3 years ago by PaulM (previous) (diff)

comment:6 Changed 3 years ago by russellm

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

@Soren - the bug management rules here are that if you have found a duplicate, you close the new ticket as a duplicate. Jacob's comment on closing #9947 still apply, so I don't see any action required here.

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.