Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

conftest.py presence changes sys.path and settings import #23

Closed
krya opened this Issue Dec 3, 2012 · 26 comments

Comments

Projects
None yet
5 participants

krya commented Dec 3, 2012

running py.test with DJANGO_SETTINGS_MODULE=myproject.settings py.test fails if there is no conftest.py file in root folder with ERROR: Could not import settings 'myproject.settings' (Is it on sys.path?): No module named myproject.settings

Owner

flub commented Dec 3, 2012

Can you describe the layout of your files and directories in a little more detail? And include how and in which directory you invoke py.test. My suspicion is that you indeed do not have the directory which contains the myproject package on sys.path.

The fact that adding a conftest.py works around this is because of a side-effect of how py.test locates and imports the conftest.py files. IIRC the end result is that each directory with conftest.py ends up being added to sys.path. I think there's even an issue filed about this as well, or at least has been discussed by the py.test devs. But IIRC no better solution for loading conftest.py files has been found yet.

krya commented Dec 3, 2012

layout is pretty much simple - its an empty project created with startproject :)
tested in django 1.4 and 1.3

Owner

flub commented Dec 4, 2012

Hi, sorry but startproject itself does not create any tests. So could you please provide the information I asked for above so we can try to reproduce this and verify that my speculation on the cause is correct.

Currently I still think that if you execute py.test from the root of the project the settings module will be found. It is simply a matter of how the python import system works and not a bug in pytest-django. The only added confusion is how py.test influences the python import system in the presence of conftest.py files which allows files to be importable under multiple names (which is considered a bad thing (tm)).

Owner

pelme commented Dec 4, 2012

I started a fresh Django project yesterday with django-admin.py startproject (Django 1.4.2), and have not yet created a conftest.py file, and pytest-django picks up the tests and everything works as expected.

I am not saying this is not a problem but, as @flub already pointed out, we need a detailed description on how to reproduce the error. Thanks!

krya commented Dec 5, 2012

here is a session from my terminal https://gist.github.com/4214583 starting django project and trying to use pytest.
the only thing I did out of terminal is edit settings file to use sqlite db.
@flub but why it should fail if there is no tests with import error?

Owner

flub commented Dec 5, 2012

@krya Fair point, it should just say 0 tests collected.

And thanks for the detailed demo/info, that was exactly what I was after and after some initial puzzlement I discovered I can reproduce it if I do "unset PYTHONPATH". I'll have to investigate some more

Owner

flub commented Dec 9, 2012

Hi, I've done some more experiments on all variations but am combing back to my very first conclusion: the package directory is simply not on sys.path and it's not pytest-django's responsibility to make it so.

The fact that the presence of a conftest.py file modifies sys.path is an issue of py.test itself. Equally so I think automatically adding the base directory of the project onto sys.path would be a generic py.test issue, not a pytest-django one. So unless I've missed something I'd like to close this as not a bug.

Thanks

Owner

pelme commented Dec 10, 2012

flub, you are right, it is not on sys.path, but, normally things in the current working directory are importable (current working directory is always added to sys.path).

When invoking py.test, without a conftest.py, that implicit current working directory seems to go away.

This finds myproject based on the current working directory:

[myproject] ~/myproject $ python -c 'import myproject;print myproject'           
<module 'myproject' from 'myproject/__init__.pyc'>

This works (should be identical to just calling py.test from the shell?)

[myproject] ~/myproject $ rm -f conftest.py; python -c 'import pytest;pytest.main()'
================================================= test session starts ==================================================
platform darwin -- Python 2.7.2 -- pytest-2.3.4
plugins: django
collected 0 items 

===================================================  in 0.01 seconds ===================================================

This also works; copying the py.test entry point executable to the current directory:

[myproject] ~/myproject $ rm -f conftest.py; cp `which py.test` . ;./py.test
================================================= test session starts ==================================================
platform darwin -- Python 2.7.2 -- pytest-2.3.4
plugins: django
collected 0 items 

===================================================  in 0.01 seconds ===================================================

As already noted above, this does not work:

[myproject] ~/myproject $ rm -f conftest.py; py.test
ERROR: Could not import settings 'myproject.settings' (Is it on sys.path?): No module named myproject.settings

I usually never rely on this implicit import and prefer to do add2virtualenv . (when cwd is ~/myproject as above) - that will solve all problems and make things very explicit and everything will always be found, no matter which directory that happends to be the current directory.

This is still an issue, and this behavior is indeed strange, I am not however sure where the actual problem is. pytest-django does not do any modifications of sys.path whatsoever, so it might be an issue in py.test.

Owner

flub commented Mar 18, 2013

@pelme sys.path is more subtle than that I think. When python is invoked interactively (i.e. in all your -c examples) the emptry string "" is added to the front of sys.path. But when you invoke a script (i.e. the py.test binary) then the directory of the script is added to the front of sys.path. This explains the behaviour you are seeing I think. Because of that oddity I have export PYTHONPATH=''" in my .bashrc so I always have the current directory on sys.path (which is probably a security vulnerability).

I just wanted to start using py.test and hit the same problem. In my opinion, if pytest-django wants to be a replacement ./manage.py test, then this should be addressed. I see two ways to do that:

  • automatically add current path to PYTHONPATH
  • provide Django application replacing test command.

My argument here is it's really hard for beginners to understand this.

Owner

flub commented Feb 20, 2014

Maybe the 2dn option should be considered again. It means people can just run ./manage.py test and it will "just work". I know it wasn't deemed desirable since you're much more limited to what you can do since you have to conform to Django's test API but it might be suitable as a basic compat layer which will solve this ethernal PYTHONPATH issue.

Suor commented Aug 1, 2014

This issue is ridiculously frustrating. I wonder how much people were diverted from pytest-django because of that. You try to use it and it "just don't work". Nothing in an installation or get started states that you should do some black magic with PYTHON_PATH. FAQ entry clearly lies that touch conftest.py will fix it, it doesn't work for pytest-django>2.3, setting aside a question what the heck conftest.py does with this.

This was brought up and up again several times in various github issues. So this is obviously problem.

P.S. Anyone still having this problem could try pytest-pythonpath.

pelme added a commit that referenced this issue Aug 1, 2014

Owner

pelme commented Aug 1, 2014

@Suor

I totally agree that this is a problem, but we haven't found a straightforward solution that works reliably in all cases.

We had a lot of discussions about this on EuroPython and have a potential solution: Scanning for manage.py files and adding those files directories to PYTHONPATH. That would match what most people are used to from Django.

I removed the conftest.py recommendation in the FAQ and also added a mention of pytest-pythonpath, sorry for the confusion.

The current solution is to install the package with pip install -e . or similar according to the FAQ:
http://pytest-django.readthedocs.org/en/latest/faq.html#i-see-an-error-saying-could-not-import-myproject-settings

I think this is an important issue and any feedback and/or PR:s with improvements to the docs or how to solve this is very welcome.

Suor commented Aug 1, 2014

Thanks for quick response and updating FAQ. However, a problem persists.

  1. Fixing a FAQ is good, but that someway assumes people will run into this. Also your FAQ is not that googlable. I ran into this problem twice and I only found FAQ with a help of a link from this issue. I suggest adding some instruction regarding this into "Getting Started".
  2. Looking for manage.py is hugely error prone. I, for example, lay it into tests package. What we need is to add current directory not the one with manage.py.
  3. Installing current package with pip install -e . is a weird hack. As well as adding PYTHON_PATH=. to virtualenv somehow. It should be possible to clone a repo and then just pip install -r test_requirements.txt && py.test. Anything other than that is some custom setup which adds complexity.
Owner

flub commented Aug 1, 2014

  1. Guess I'd agree with this, many people run into this so mentioning it in getting started is probably a good idea.
  2. Interesting, so that means using manage.py is not a viable option either. Note that the cwd is not a solution either, people can invoke py.test from within their test (sub)directory which would make this not work either.
  3. pip install -e . is not a weird hack, it is simply the standard way of doing this. The main problem is that is appears to be impossible to find out what the project root is without the user telling where it is. Telling where it is is done using pip install -e . or setting PYTHONPATH in the virtualenv.

Would it make sense to change the getting started guide to tell you to use a virtualenv and then describe how to set PYTHONPATH in there just as it describes it for DJANGO_SETTINGS_MODULE now? As well as the pip install -e . option.

If you have any tips or ideas on how pytest-django could find the project root in a reliable way that would be greatly appreciated.

Suor commented Aug 1, 2014

  1. Then how does it work with pytest without pytest-django. I tried now and it does somehow. It even works if I start py.test from a subdir. And printing sys.path in a test shows that project dir is included there.

Suor commented Aug 1, 2014

Idea - look for pytest.ini and tox.ini.

Owner

pelme commented Aug 1, 2014

  1. Yes, that sounds like a very good idea. I've opened an issue to track this: #155
  2. Yes, it is error prone, but most people have a manage.py file that starts runserver - and it implicitly adds its path to Python path - which is what everyone expects in a Django project. We could provide an option to not scan for manage.py. What is your use case for having manage.py in your tests package?
  3. This problem is not specific to pytest. You basically needs to have your code available on the path to be able to import it, that is the basic issue here. People that are only used to interact with their code from manage.py however does not need to care for this, since it implicitly "fixes" the python path.

There are a bunch of different way to achieve the end goal of "being able to import my stuff". Providing a setup.py is the standard way of packaging a Python application (regardless of if you want to distribute it on PyPI or not), and installing in editable mode manages the Python path to have it behave like an actual installed package.

A common way to solve this is to add a line which reads "-e ." to the requirements file (see pytest-django's own requirements file: https://github.com/pelme/pytest_django/blob/master/requirements.txt) - it is possible to clone pytest-django, install requirements and then just run py.test directly.

Again, I very mich agree that this is a very confusing problem. The problem is still that there are no 100% bullet proof solution that works in all cases.

Owner

pelme commented Aug 1, 2014

  1. Can you provide an example where this happens without pytest-django with only pytest?

It does not work for me with this very quick test:
https://gist.github.com/pelme/6815b40ba294df6d9679

This issue is also even more subtle because of how Django works. Starting with Django 1.7, pytest must not import any models or Django related code before django.setup() has been called (which does all imports and loading of apps). This makes it even trickier to have the paths set up at the correct point. pytest normally implicitly adds stuff to the path when collecting conftest files and tests, and we can simply not rely on this when running Django tests. Paths and all app import must happen before this point. This works fine for any other Python code, but since Django puts restrictions on how things can be imported, this is not possible in this case.

Looking for pytest.ini and tox.ini will only help if those files already exists. You likely don't have pytest.ini if you are just switching to pytest, and it would be unexpected to change the paths based on those files in pytest-django for existing pytest users. Documenting that you have to create a file called pytest.ini is not really much easier than creating setup.py + pip install -e ..

To have a meaningful tox.ini you also probably need a setup.py file already, and then "-e ." would be very easy.

Therefore, I stil think that the manage.py scan (with the option to disable it) currently makes most sense for pytest-django.

Thank you very much for the feedback, keep it coming. :)

Suor commented Aug 1, 2014

Other angle, you are probably overlooking as a library author, is that there are more people developing projects (mere websites) in Django then reusable apps or libraries. And it usually doesn't make any sense to install a project. So recommending to write setup.py (bogus obviously) and then pip install -e . is far from reasonable and understandable in this case.

By the way, writing setup.py first time is a non-trivial exercise.

Owner

pelme commented Aug 2, 2014

My primary use case and original need for pytest-django is not a library, but a website (a Django project), this is how I interact with pytest-django most of the time.

And it usually doesn't make any sense to install a project.

I disagree with this - the usefulness of setup.py is not limited to pure libraries. Installing your project in editable mode in a virtualenv is the easiest way to manage Python paths that I have found. I interact with my project from multiple programs: celery, gunicorn and other utilities - having that just work consistently from any working directory is just not possible without setting up the paths. The setup.py file is literally two lines long (exact copy/paste from the pytest-django FAQ), and then there is the "-e ." line in the requirements file. That is really all there is to it. This setup.py file is not sufficient to make a proper package, but that can be evolved over time when that is actually needed.


With that said: People that are used interact with their code via manage.py will have an harder time getting pytest+Django working than should be needed. I fully agree that the current situation is problematic, badly documented and causes pain for people, and I intend to fix it. The idea of the manage.py-scanning solution is just one week old and I have not had the time to implement it yet, but that is currently the best solution in my opinion.

Can you please answer these questions?

  1. Do you see any issues with auto finding manage.py, when there is an option to disable it?
  2. Do you have any other suggestions on how to improve the situation apart from the manage.py scan (or follow up comments on my comments on the tox.ini / pytest.ini suggestion)?
  3. Can you provide additional details where "it works" with pytest and not with pytest-django? I can then try to track down the differences.

Again, thanks for your feedback, I appreciate that you care for this. I hope we can find a solution to this to accommodate both for those who likes to manage their python path explicitly and those who just want this out of their way to interact with their Django project as usual.

Suor commented Aug 4, 2014

  1. Could produce weird errors when placed not in project root. Introduces "magical behaviour" - non obvious connection between otherwise distinct parts of project. I have manage.py in tests cause tests is a directory for test project - makes sense for libs.
  2. py.test could watch for ImportErrors and add a message explaining why it could be caused and how one should set this up.
  3. Replied to your gist https://gist.github.com/pelme/6815b40ba294df6d9679
Owner

pelme commented Aug 4, 2014

  1. Do you think that is common enough to have multiple manage.py files to avoid the idea altogether (even with the option to disable it)?
  2. That's a good idea. I've opened #156 to track it.
  3. I also replied again.

Suor commented Aug 4, 2014

  1. I don't think it's common, but unlucky few will have a hard time debugging :) It could be combined with ImportError watch - saying that root of their project was detected by manage.py (and which exactly) and then usual adding usual text from point 2.
  2. Thanks.
  3. I understand why pytest magic doesn't work, but it could be simulated if it's not that hard (I don't really know). Then it could replace manage.py heuristic.
Owner

pelme commented Aug 4, 2014

  1. Good point, I guess the key here is to provide good error messages and insights into the "magic" that is happening.
  2. In order to do that, we basically would need to do two test collection scans and implicitly add paths for the test folders to sys.path during the first run, then load Django, and then do the real test collection. I a day at the EuroPython sprints figuring this would be possible, and I don't think it is feasible. There would probably be a lot of other strange side effects too.
Owner

pelme commented Dec 23, 2014

Closing this since the behavior is now documented and the automatic Django project finder is implemented.

@pelme pelme closed this Dec 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment