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

get PYTEST_ADDOPTS before calling _initini #2824

Merged
merged 4 commits into from
Oct 18, 2017
Merged

get PYTEST_ADDOPTS before calling _initini #2824

merged 4 commits into from
Oct 18, 2017

Conversation

dirk-thomas
Copy link
Contributor

While it works to pass -o cache_dir=/somewhere as command line arguments or set the option in the setup.cfg file I tried to achieve the same by setting the environment variable PYTEST_ADDOPTS="-o cache_dir=/somewhere". This doesn't work atm since the variable is only being considered after Config._initini has been called which e.g. decides on the override_ini.

To enable this use case this patch moves the call to Config._initini behind getting the environment variable PYTEST_ADDOPTS. I didn't see any negative side effects of this change but I am new to pytest so might be missing something.

I haven't added a test for this since I thought getting feedback in the patch first makes sense. If a test is desired please let me know and I am happy to do so.

I targeted master since the change is trivial. Please let me know if I should target features instead.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a good idea

please add tests

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 92.236% when pulling 63f91e9 on dirk-thomas:pytest_addopts_before_initini into 46e3043 on pytest-dev:master.

@dirk-thomas
Copy link
Contributor Author

I added a test in 5bcdb52 - without the patch it fails - with the patch it passes.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 92.236% when pulling 5bcdb52 on dirk-thomas:pytest_addopts_before_initini into 46e3043 on pytest-dev:master.

@@ -843,3 +843,10 @@ def test_with_existing_file_in_subdir(self, tmpdir):
rootdir, inifile, inicfg = determine_setup(None, ['a/exist'])
assert rootdir == tmpdir
assert inifile is None

def test_addopts_before_initini(self, testdir, tmpdir, monkeypatch):
monkeypatch.setenv('PYTEST_ADDOPTS', '-o cache_dir=/somewhere')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good thinking, for more pedantic separation i propose to replace the arbitrarily chosen /somewhere by using testdir.join('somewhere')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 48c31f9. I had to use testdir.tmpdir.join(...) though.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 92.236% when pulling 48c31f9 on dirk-thomas:pytest_addopts_before_initini into 46e3043 on pytest-dev:master.

@nicoddemus
Copy link
Member

Shouldn't we target features? While it might be argued that this is a bug-fix, it seems strange to me that PYTEST_ADDOPTS now considers additional options after a bug-fix release.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 92.236% when pulling d801d31 on dirk-thomas:pytest_addopts_before_initini into 46e3043 on pytest-dev:master.

@RonnyPfannschmidt
Copy link
Member

good call, lets re-target this to features

@dirk-thomas dirk-thomas changed the base branch from master to features October 11, 2017 16:34
@dirk-thomas
Copy link
Contributor Author

I updated the PR to target the features branch instead.

Just out of curiosity: what is roughly the timeline the changes on the features branch make it into a new release? Do they go into the next minor or major version?

@RonnyPfannschmidt
Copy link
Member

the features branch is for minor versions primarily, we dont have a clear schedule as of now

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 92.566% when pulling cfd928c on dirk-thomas:pytest_addopts_before_initini into 1480aed on pytest-dev:features.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 92.563% when pulling ce8c829 on dirk-thomas:pytest_addopts_before_initini into 1480aed on pytest-dev:features.

…on Windows

We use shlex to parse command-line arguments and PYTEST_ADDOPTS, so passing
a full path with '\' arguments produces incorrect results on Windows

Anyway users are advised to use relative paths for portability
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks.

Used a relative cache_dir because a full path on Windows contains \ characters which are parsed incorrectly by shlex.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 92.563% when pulling 10a3b91 on dirk-thomas:pytest_addopts_before_initini into 1480aed on pytest-dev:features.

@nicoddemus nicoddemus merged commit 537fc3c into pytest-dev:features Oct 18, 2017
ceridwen pushed a commit to ceridwen/pytest that referenced this pull request Oct 19, 2017
ceridwen pushed a commit to ceridwen/pytest that referenced this pull request Oct 19, 2017
@dirk-thomas dirk-thomas deleted the pytest_addopts_before_initini branch October 20, 2017 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants