Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Look at os.environ before early_config #199

Closed
wants to merge 1 commit into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jan 22, 2015

This breaks a lot of internal tests, which could be adjusted.

I think it makes more sense to prefer the current environment before the
config from (e.g.) setup.cfg.

@blueyed blueyed changed the title Look at os.environ before early_config TBD: Look at os.environ before early_config Jan 23, 2015
blueyed added a commit that referenced this pull request Jan 28, 2015
It is not used currently though, see
#199.
@pelme
Copy link
Member

pelme commented Mar 29, 2015

Yes, it makes sense to change this order and I think it's fine.

I have a slight concern about backward incompatibility, this change might break things for someone, but I think that it is worth merging this.

@blueyed blueyed force-pushed the prefer-django-settings-from-env branch from 85393c6 to 038ca82 Compare March 29, 2015 20:00
@blueyed blueyed force-pushed the prefer-django-settings-from-env branch 4 times, most recently from aeeef1a to 62f92fb Compare April 23, 2015 00:43
This prefers the current current environment before the config from
(e.g.) setup.cfg.
@blueyed blueyed force-pushed the prefer-django-settings-from-env branch from 62f92fb to 976bb56 Compare April 23, 2015 00:51
@blueyed blueyed closed this in 2441b86 Apr 23, 2015
@blueyed blueyed deleted the prefer-django-settings-from-env branch April 23, 2015 01:11
pelme added a commit that referenced this pull request Oct 5, 2015
This was changed before the 2.9.0 release and was not documented. The
change is backward incompatible and may surprise users.

Then change was originally made in #199.
@pelme
Copy link
Member

pelme commented Oct 5, 2015

@blueyed This broke my own test suite when running soon-to-be-2.9.0-release against my own tests. I've added a note in the changelog in 726a63f.

I still think this is a good change, but it was difficult to track down at first when a bunch of unrelated tests random tests just failed in strange ways. Please have a look at the changelog and let me know/feel free to make other changes if you figure out a way to make this more visible/obvious. :)

@blueyed
Copy link
Contributor Author

blueyed commented Oct 5, 2015

it was difficult to track down

We should have information about it with --verbose then probably?

@blueyed blueyed changed the title TBD: Look at os.environ before early_config Look at os.environ before early_config Oct 5, 2015
@pelme
Copy link
Member

pelme commented Oct 5, 2015

A message that prints which Django settings is used? Something like

=================== test session starts ========================
platform darwin -- Python 2.7.10 -- py-1.4.30 -- pytest-2.7.2 -- /Users/andreas/.virtualenvs/pytest-django/bin/python
cachedir: /Users/andreas/code/pytest-django/.cache
rootdir: /Users/andreas/code/pytest-django, inifile: setup.cfg
plugins: django, cache, xdist, xdist, xdist
django settings module: foo.bar.settings_abc

??

@blueyed
Copy link
Contributor Author

blueyed commented Oct 5, 2015

Yes, and also the origin of it (e.g. option, environment or config).

@pelme
Copy link
Member

pelme commented Oct 5, 2015

Yes, that makes sense.

@pelme
Copy link
Member

pelme commented Oct 5, 2015

I'll fix it now!

pelme added a commit that referenced this pull request Oct 5, 2015
With the recent change to prefer DJANGO_SETTINGS_MODULE from the
environment rather than ini-file, it is nice to show which Django
settings is used and why it was choosen.

See discussion in #199
for more information.
@pelme
Copy link
Member

pelme commented Oct 5, 2015

Fixed in 4b0526e :)

@blueyed
Copy link
Contributor Author

blueyed commented Oct 5, 2015

Thanks!

@blopker
Copy link

blopker commented Oct 8, 2015

This broke our build.

We use the environment for running the app then use the ini file to override the environment. This way we can run py.test in any folder to run a subset of tests. Now we have to figure out another way to do this.

I guess I don't understand why the change was made. It seems like Django itself always uses the env as a last option.

@pelme
Copy link
Member

pelme commented Oct 8, 2015

The reasoning for this change is that typically the .ini file goes into the repository and sets a default settings module. The env var can be overridden (or set to a default in the local environment). Finally the --ds option can be used to override both ini and settings. Django does something similar: it uses the env var but it can be overridden by specifying --settings.

Would putting

addopts = --ds=yourtestsettings

in your ini file solve your problem? That would set the Django settings from the ini-file, but then you cannot override with --ds manually.

@blopker
Copy link

blopker commented Oct 8, 2015

Thank you for the workaround, I'll try it out.

All I know is I have a case where having the env last works really well. The addopts key feels like a hack to me. I'd like to see an actual app where letting the env override the ini is desirable.

I have a feeling this is going to bite a lot of people.

Anyway, this has shipped already so I guess I'll just find a way around it.

@pelme
Copy link
Member

pelme commented Oct 9, 2015

I think the --ds/env/ini-way is useful in libs where you don't typically define DJANGO_SETTINGS_MODULE in your local environment but expect the ini file to provide it. Then it becomes easy for a single contributor to run tests with a specific test setting most of the time, but then just override it with --ds.

For projects where you use the DJANGO_SETTINGS_MODULE env var in your local environment to run django-admin runserver and other stuff, having the ini-file override the env var makes more sense. You typically have a settings module for development and one for running tests, not multiple settings modules for tests.

I am split by this - this is more convenient for libs (like pytest-django itself) but not for my "real" project. :)

I am thinking about having another ini-style config that you could specify the order yourself (something like django_settings_module_source = option,ini,env) - but that seems kind of overkill too.

I agree that addopts = --ds=foo is a bit of a hack, but so is addopts itself but yet it is practically very useful. :)

@blopker Please let us know if addopts makes it work for you, if not
@blueyed Do you have any thoughts on this?

(If anyone else is reading this and got bitten by this, please drop a line here and let us know!)

@blopker
Copy link

blopker commented Oct 10, 2015

I can confirm addopts works! I still think it's not an ideal solution, but it's workable.

Aside from changing it back, I'd suggest adding addopts to the docs.

Cheers!

@blueyed
Copy link
Contributor Author

blueyed commented Oct 10, 2015

JFI: my use case was to easily use different settings from the environment, while there might be a default configured in the config file.

@blopker

We use the environment for running the app then use the ini file to override the environment. This way we can run py.test in any folder to run a subset of tests.

I can see this use case, and could imagine to use --ds myself for different environments.

I also cannot remember if the PR came from working on a project or when working on pytest-django itself.

I think a option like django_settings_module_source would be able to solve this for everybody, especially given that pytest-django tells you where settings are coming from now.

A workaround for @blopker's use case would also be to run py.test as env -u DJANGO_SETTINGS_MODULE py.test.

And after all I'd be OK with changing the default again, of course.

tomviner pushed a commit to tomviner/pytest-django that referenced this pull request Jun 22, 2016
This was changed before the 2.9.0 release and was not documented. The
change is backward incompatible and may surprise users.

Then change was originally made in pytest-dev#199.
tomviner pushed a commit to tomviner/pytest-django that referenced this pull request Jun 22, 2016
With the recent change to prefer DJANGO_SETTINGS_MODULE from the
environment rather than ini-file, it is nice to show which Django
settings is used and why it was choosen.

See discussion in pytest-dev#199
for more information.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 976bb56 on prefer-django-settings-from-env into * on master*.

mfa pushed a commit to aexeagmbh/pytest-django that referenced this pull request May 17, 2017
mfa pushed a commit to aexeagmbh/pytest-django that referenced this pull request May 17, 2017
This prefers the current current environment before the config from
(e.g.) setup.cfg.

Closes pytest-dev#199
mfa pushed a commit to aexeagmbh/pytest-django that referenced this pull request May 17, 2017
This was changed before the 2.9.0 release and was not documented. The
change is backward incompatible and may surprise users.

Then change was originally made in pytest-dev#199.
mfa pushed a commit to aexeagmbh/pytest-django that referenced this pull request May 17, 2017
With the recent change to prefer DJANGO_SETTINGS_MODULE from the
environment rather than ini-file, it is nice to show which Django
settings is used and why it was choosen.

See discussion in pytest-dev#199
for more information.
beyondgeeks added a commit to beyondgeeks/django-pytest that referenced this pull request Sep 6, 2022
beyondgeeks added a commit to beyondgeeks/django-pytest that referenced this pull request Sep 6, 2022
This prefers the current current environment before the config from
(e.g.) setup.cfg.

Closes pytest-dev/pytest-django#199
beyondgeeks added a commit to beyondgeeks/django-pytest that referenced this pull request Sep 6, 2022
This was changed before the 2.9.0 release and was not documented. The
change is backward incompatible and may surprise users.

Then change was originally made in pytest-dev/pytest-django#199.
beyondgeeks added a commit to beyondgeeks/django-pytest that referenced this pull request Sep 6, 2022
With the recent change to prefer DJANGO_SETTINGS_MODULE from the
environment rather than ini-file, it is nice to show which Django
settings is used and why it was choosen.

See discussion in pytest-dev/pytest-django#199
for more information.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants