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

@dirk-thomas dirk-thomas commented Oct 10, 2017

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

looks like a good idea

please add tests

@coveralls
Copy link

@coveralls coveralls commented Oct 10, 2017

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

@dirk-thomas dirk-thomas commented Oct 10, 2017

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

@coveralls
Copy link

@coveralls coveralls commented Oct 10, 2017

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

@RonnyPfannschmidt RonnyPfannschmidt Oct 10, 2017

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

Copy link
Contributor Author

@dirk-thomas dirk-thomas Oct 10, 2017

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

@coveralls
Copy link

@coveralls coveralls commented Oct 10, 2017

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

@nicoddemus nicoddemus commented Oct 10, 2017

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

@coveralls coveralls commented Oct 10, 2017

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

@RonnyPfannschmidt RonnyPfannschmidt commented Oct 11, 2017

good call, lets re-target this to features

@dirk-thomas dirk-thomas changed the base branch from master to features Oct 11, 2017
@dirk-thomas
Copy link
Contributor Author

@dirk-thomas dirk-thomas commented Oct 11, 2017

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

@RonnyPfannschmidt RonnyPfannschmidt commented Oct 11, 2017

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

@coveralls
Copy link

@coveralls coveralls commented Oct 11, 2017

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

@coveralls coveralls commented Oct 11, 2017

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

LGTM thanks.

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

@coveralls
Copy link

@coveralls coveralls commented Oct 11, 2017

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
2 checks passed
ceridwen added a commit to ceridwen/pytest that referenced this issue Oct 19, 2017
ceridwen added a commit to ceridwen/pytest that referenced this issue Oct 19, 2017
@dirk-thomas dirk-thomas deleted the pytest_addopts_before_initini branch Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants