#1642 add rootdir option #3127

feuillemorte · 2018-01-17T20:15:17Z

Added rootdir option.
How to test:

create folder with test environment, for example: /home/user/github/pytest
create folder with tests, for example: /home/user/rootest/tests/<tests_py>

enter test environment and run pytest tests --rootdir=$HOME/rootest

create pytest.in file in tests environment:
[pytest]
rootdir=$HOME/rootest

run tests

rootest
| - spoon.py
\ -tests
\ - test_one.py (import spoon)

testruns:

pytest tests --rootdir=$HOME/rootest
1 passed in 0.02 seconds

coveralls · 2018-01-17T20:42:42Z

Coverage decreased (-0.002%) to 92.619% when pulling 4a18d76 on feuillemorte:1642-add-rootdir-option into 1fd67c9 on pytest-dev:features.

coveralls · 2018-01-17T21:34:49Z

Coverage decreased (-0.002%) to 92.619% when pulling a7c39c8 on feuillemorte:1642-add-rootdir-option into 1fd67c9 on pytest-dev:features.

nicoddemus

Hi @feuillemorte, thanks a lot for the PR! Please take a look at my comments.

nicoddemus · 2018-01-18T19:58:37Z

_pytest/main.py

@@ -283,6 +291,17 @@ def __init__(self, config):
        self.trace = config.trace.root.get("collection")
        self._norecursepatterns = config.getini("norecursedirs")
        self.startdir = py.path.local()
+
+        rootdir_ini = config.getini('rootdir')


This doesn't seem the proper place to override the rootdir, we should instead check if the option was passed in the determine_setup function, which is responsible to automatically discover the rootdir:

pytest/_pytest/config.py

Lines 1326 to 1349 in 6213746

def determine_setup(inifile, args, warnfunc=None):

dirs = get_dirs_from_args(args)

if inifile:

iniconfig = py.iniconfig.IniConfig(inifile)

try:

inicfg = iniconfig["pytest"]

except KeyError:

inicfg = None

rootdir = get_common_ancestor(dirs)

else:

ancestor = get_common_ancestor(dirs)

rootdir, inifile, inicfg = getcfg([ancestor], warnfunc=warnfunc)

if rootdir is None:

for rootdir in ancestor.parts(reverse=True):

if rootdir.join("setup.py").exists():

break

else:

rootdir, inifile, inicfg = getcfg(dirs, warnfunc=warnfunc)

if rootdir is None:

rootdir = get_common_ancestor([py.path.local(), ancestor])

is_fs_root = os.path.splitdrive(str(rootdir))[1] == '/'

if is_fs_root:

rootdir = ancestor

return rootdir, inifile, inicfg or {}

We should obtain the rootdir from the command line arguments in Config.__init__ and pass it to this function, which should use the passed rootdir (if any) or go about discovering it like it does today if not passed on the command-line.

We should obtain the rootdir from the command line arguments in Config.init

How can I obtain it?
command options are empty at this moment

self.rootdir_cmd_arg = self.getoption('--rootdir')
returns
ValueError: no option named '--rootdir'
and there is a function pytest_cmdline_parse which executes after config.

determine_setup function executes in _initini and (as I think) ini files parsed before comand line. Am I right?

btw, how can I attach file from repo here like you?

I will have to think about how to obtain the command line option at that moment, but here's how you can post code snippets in comments on GitHub:

https://help.github.com/articles/creating-a-permanent-link-to-a-code-snippet/

pytest/_pytest/config.py

Line 991 in 6213746

def _initini(self, args):

ns.rootdir is working and when I pass to

pytest/_pytest/config.py

Line 1326 in 6213746

def determine_setup(inifile, args, warnfunc=None):

some code like this:

if rootdir_cmd_arg: rootdir_abs_path = py.path.local(rootdir_cmd_arg) if not os.path.isdir(str(rootdir_abs_path)): raise UsageError("Directory '{}' not found. Check your '--rootdir' option.".format(rootdir_abs_path)) rootdir = rootdir_abs_path return rootdir, inifile, inicfg or {}

my rootdir was changed, but it's not working =) It try to find modules not in rootdir. Searching solution...

nicoddemus · 2018-01-18T20:00:44Z

_pytest/main.py

@@ -33,6 +33,9 @@ def pytest_addoption(parser):
    parser.addini("testpaths", "directories to search for tests when no files or directories are given in the "
                               "command line.",
                  type="args", default=[])
+    parser.addini("rootdir", "define root directory for tests. If this parameter defined command argument "


In general we should avoid adding both an ini option and command-line option for the same thing, because it is easy to change the value of an ini option in the command line (using -o) and one can change command-line options by setting addopts in an ini file, so it is a bit redundant.

In this case I think we will have to go with only a command-line option, because we determine the rootdir very early, even before we are setup to parse the ini file.

feuillemorte · 2018-01-20T19:30:48Z

pytest tests --rootdir=invalid_folder
not run tests and returns
ERROR: Directory '/home/feuillemorte/github/pytest_fork/invalid_folder' not found. Check your '--rootdir' option.

and I can't to write test on it because

result = testdir.runpytest("--rootdir=wrong_dir")
result.stdout.str()

returns empty string

Please, review this fix and write comments

coveralls · 2018-01-20T19:57:29Z

Coverage increased (+0.005%) to 92.632% when pulling 37d836d on feuillemorte:1642-add-rootdir-option into f2fb841 on pytest-dev:features.

nicoddemus

and I can't to write test on it because ... returns empty string

Sorry I didn't quite get that... the test seems correct and is passing on the platforms, what is the problem that you are encountering exactly?

nicoddemus · 2018-01-22T19:47:50Z

_pytest/config.py

        self.rootdir, self.inifile, self.inicfg = r
        self._parser.extra_info['rootdir'] = self.rootdir
        self._parser.extra_info['inifile'] = self.inifile
        self.invocation_dir = py.path.local()
+        if ns.rootdir:
+            self.invocation_dir = self.rootdir
+            sys.path.append(str(self.rootdir))


This seems strange to me, why is this change required?

what is the problem that you are encountering exactly

I want to write negative test and I can't match result. It can't catch

This seems strange to me, why is this change required?

for path in sys.path: print(path)

/*/env/bin /*/env/lib/python36.zip /*/env/lib/python3.6 /*/env/lib/python3.6/lib-dynload /usr/lib64/python3.6 /usr/lib/python3.6 /*/env/lib/python3.6/site-packages /*/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg /*/env/lib/python3.6/site-packages/attrs-17.4.0-py3.6.egg /*/env/lib/python3.6/site-packages/six-1.11.0-py3.6.egg /*/env/lib/python3.6/site-packages/py-1.5.2-py3.6.egg /*/env/lib/python3.6/site-packages/pytest-3.3.3.dev50+g83034bbd.d20180122-py3.6.egg /*/env/lib/python3.6/site-packages

Python paths are not contain rootdir, so, I get an error:
ModuleNotFoundError: No module named 'spoon'

if it's not a good solution, please, give me an advice
Thank you!

and I can't to write test on it because

If that's because result.stdout.str() is returning an empty string it might be that you have to look for the error in result.stderr.str() instead.

Perhaps you can post the exact test you're trying and what you expected? That way I might be able to point the exact problem.

Python paths are not contain rootdir

Oh you mean in the test? If that's the case you can change the test instead to include the path by calling testdir.syspathinsert() at the start of the test.

Oh you mean in the test

It's not only in test. It repeates when I test manualy a rootdir option

But the rootdir is not added currently, so I don't think it should be added if --rootdir is passed in the command-line either.

I can check rootdir path and add to sys.path if it's not exist. Is it ok?

I mean if you have tests in another folder, for example, /home/user/some_dir/tests you don't have this folder in paths. So, you can't import some files by this path if you pass arg --rootdir=/home/user/some_dir/tests - you'll get an import error

@feuillemorte I see what you mean, but "automatically adding rootdir to sys.path" is a different issue than "set the rootdir on the command-line"; I don't think we should do both changes in the same PR and without some serious consideration.

Adding the rootdir to sys.path is not something to be done lightly because this can cause a lot of unexpected consequences, believe me. 😬

I see, thank you. I'll delete this line then :)

But really, I don't know in which cases rootdir will be used because we can't use imports there (except system imports) Only if import will be like 'from root.spoon...'
And in this case we can run like 'pytest root/tests' without rootdir option. Am I wrong?

The rootdir is used so pytest can have a reproducible node ids between runs in different computers. Here's an excerpt from the docs:

Construct nodeids during collection; each test is assigned a unique nodeid which is rooted at the rootdir and takes in account full path, class name, function name and parametrization (if any).

Is used by plugins as a stable location to store project/test run specific information; for example, the internal cache plugin creates a .cache subdirectory in rootdir to store its cross-test run state.

So it is not really related to sys.path at all... but I understand why it might be confusing given its name.

nicoddemus · 2018-01-23T19:32:45Z

@feuillemorte could you please add a paragraph to custommize mentioning this new option? Thanks!

…est into 1642-add-rootdir-option

feuillemorte · 2018-01-25T16:48:04Z

Fixed. Please, review

nicoddemus

Thanks for the follow up @feuillemorte. Please see comments.

nicoddemus · 2018-01-27T11:49:07Z

testing/test_session.py

@@ -253,3 +253,28 @@ def pytest_sessionfinish():
    """)
    res = testdir.runpytest("--collect-only")
    assert res.ret == EXIT_NOTESTSCOLLECTED
+
+
+def test_rootdir_option_arg(testdir):


Please also add tests for absolute and paths containing environment variables.

tests on absolute path added (not commites yet)

paths containing environment variables not working in tests, but working when I test locally :(

tests:

testdir.runpytest("--rootdir=$HOME") ERROR: Directory '/tmp/pytest-of-feuillemorte/pytest-31/test_rootdir_option_arg2/$HOME/root' not found. Check your '--rootdir' option.

locally directory changed:

pytest tests --rootdir=$HOME/root ERROR: Directory '/home/username/root' not found. Check your '--rootdir' option.

Please, help))

You need to explicitly call os.path.expandvars to deal with variable expansion.

When you say "locally" I suppose you mean the command line? In that case is your shell doing the expansion, not pytest. 😉

nicoddemus · 2018-01-27T11:52:03Z

testing/test_session.py

+def test_rootdir_wrong_option_arg(testdir):
+    rootdir = testdir.mkdir("root")
+    testsdir = rootdir.mkdir("tests")
+    testsdir.join("test_one.py").write("def test_one():\n    assert 1")


This can be written as:

testdir.makepyfile(""" def test_one(): assert 1 """)

Which is more readable IMO. This call will create a file test_rootdir_wrong_option_arg.py in the testdir directory (same name as the test), which is not really relevant for his this test.

Actually I don't even think the file or the directories is relevant for the test at all, so we can remove this preparation from this test.

It seems that I can't create file in root/tests folder by this code. It will created in the testdir folder. But tests run fine, so, fixed =)

nicoddemus · 2018-01-27T11:53:57Z

_pytest/config.py

        self.rootdir, self.inifile, self.inicfg = r
        self._parser.extra_info['rootdir'] = self.rootdir
        self._parser.extra_info['inifile'] = self.inifile
        self.invocation_dir = py.path.local()
+        if ns.rootdir:
+            self.invocation_dir = self.rootdir


What's the reason for this change?

I'm not sure we should be changing the invocation_dir, it is supposed to contain the directory where pytest was invoked from, regardless of the rootdir.

nicoddemus · 2018-01-31T21:42:27Z

doc/en/customize.rst

@@ -371,3 +371,9 @@ passed multiple times. The expected format is ``name=value``. For example::


 .. _`#3155`: https://github.com/pytest-dev/pytest/issues/3155
+
+.. confval:: rootdir


I just realized that this is not really a configuration variable (e.g goes into a pytest.ini file) but a command line option.

I think it makes more sense to add a paragraph to the Initialization: determining rootdir and inifile section in this same file instead.

nicoddemus · 2018-01-31T21:43:20Z

_pytest/config.py

@@ -991,7 +991,8 @@ def pytest_load_initial_conftests(self, early_config):

    def _initini(self, args):
        ns, unknown_args = self._parser.parse_known_and_unknown_args(args, namespace=self.option.copy())
-        r = determine_setup(ns.inifilename, ns.file_or_dir + unknown_args, warnfunc=self.warn)
+        rootdir = ns.rootdir if ns.rootdir else None
+        r = determine_setup(ns.inifilename, ns.file_or_dir + unknown_args, warnfunc=self.warn, rootdir_cmd_arg=rootdir)


This could be written as:

r = determine_setup(ns.inifilename, ns.file_or_dir + unknown_args, warnfunc=self.warn, rootdir_cmd_arg=ns.rootdir or None)

Besides shorter, this doesn't left a rootdir local variable to be used by the unaware. 😁

nicoddemus · 2018-01-31T21:45:58Z

_pytest/config.py

@@ -1346,6 +1347,11 @@ def determine_setup(inifile, args, warnfunc=None):
                    is_fs_root = os.path.splitdrive(str(rootdir))[1] == '/'
                    if is_fs_root:
                        rootdir = ancestor
+    if rootdir_cmd_arg:
+        rootdir_abs_path = py.path.local(rootdir_cmd_arg)


You should call os.path.expandvars(rootdir_cmd_arg) here, not in the test; we want to make sure the functionality is available to users after all. 😉

nicoddemus · 2018-01-31T21:46:25Z

testing/test_session.py

+            assert 1
+    """)
+
+    result = testdir.runpytest("--rootdir={}".format(os.path.expandvars(path)))


As I commented earlier we need to remove this os.path.expandvars call from here.

nicoddemus · 2018-01-31T21:46:53Z

testing/test_session.py

+    if 'relative' in path:
+        path = path.format(relative=os.getcwd())
+    if 'environment' in path:
+        os.environ['PY_ROOTDIR_PATH'] = os.getcwd()


Better to use monkeypatch.setenv instead so this variable is removed at the end of the test.

nicoddemus · 2018-01-31T21:50:50Z

testing/test_session.py

+
+@pytest.mark.parametrize("path", ["root", "{relative}/root", "{environment}/root"])
+def test_rootdir_option_arg(testdir, path):
+    if 'relative' in path:


You can simplify the setup because applying a format() call to a string without replacements will yield the original string:

def test_rootdir_option_arg(testdir, path): path = path.format(relative=str(testdir.tmpdir), environment='$PY_ROOTDIR_PATH')

nicoddemus · 2018-01-31T21:51:34Z

testing/test_session.py

+@pytest.mark.parametrize("path", ["root", "{relative}/root", "{environment}/root"])
+def test_rootdir_option_arg(testdir, path):
+    if 'relative' in path:
+        path = path.format(relative=os.getcwd())


You should not be using os.getcwd() here because this might leave some files after the run (the .pytest_cache directory for example). Use str(testdir.tmpdir) instead.

nicoddemus · 2018-01-31T21:51:57Z

testing/test_session.py

+    """)
+
+    result = testdir.runpytest("--rootdir={}".format(os.path.expandvars(path)))
+    result.stdout.fnmatch_lines(['*rootdir: {}/root, inifile:*'.format(os.getcwd()), "*1 passed*"])


Ditto here about os.getcwd()

nicoddemus

Excellent work @feuillemorte, thanks!

feuillemorte added 3 commits January 17, 2018 23:02

#1642 Add rootdir option

d784155

#1642 Added changelog entry

86f0196

#1642 remove print

4a18d76

#1642 fix flake8

a7c39c8

feuillemorte changed the title ~~1642 add rootdir option~~ #1642 add rootdir option Jan 17, 2018

feuillemorte mentioned this pull request Jan 18, 2018

Allow users to explicitly specify the path of the root directory #1642

Closed

nicoddemus requested changes Jan 18, 2018

View reviewed changes

#1642 Fix rootdir option

83034bb

nicoddemus reviewed Jan 22, 2018

View reviewed changes

nicoddemus mentioned this pull request Jan 23, 2018

Customize test id on the terminal report #3140

Closed

Update formatting in the CHANGELOG

a7066ba

feuillemorte added 4 commits January 25, 2018 15:57

#1642 Fix tests

0cfa975

#1642 Remove adding rootdir to sys path

503e00f

added rootdir description to customize.rst

3a004a4

Merge branch '1642-add-rootdir-option' of github.com:feuillemorte/pyt…

ace772c

…est into 1642-add-rootdir-option

nicoddemus requested changes Jan 27, 2018

View reviewed changes

feuillemorte added 3 commits February 1, 2018 00:03

#1642 fix tests and config.py

741b571

#1642 Resolve conflicts

9f1772e

#1642 Fix tests

9366517

nicoddemus requested changes Jan 31, 2018

View reviewed changes

feuillemorte and others added 2 commits February 1, 2018 11:20

#1642 Fix comments

3eb6cad

Reword docs slightly

37d836d

nicoddemus approved these changes Feb 1, 2018

View reviewed changes

nicoddemus merged commit ce0a9aa into pytest-dev:features Feb 7, 2018

	def determine_setup(inifile, args, warnfunc=None):
	dirs = get_dirs_from_args(args)
	if inifile:
	iniconfig = py.iniconfig.IniConfig(inifile)
	try:
	inicfg = iniconfig["pytest"]
	except KeyError:
	inicfg = None
	rootdir = get_common_ancestor(dirs)
	else:
	ancestor = get_common_ancestor(dirs)
	rootdir, inifile, inicfg = getcfg([ancestor], warnfunc=warnfunc)
	if rootdir is None:
	for rootdir in ancestor.parts(reverse=True):
	if rootdir.join("setup.py").exists():
	break
	else:
	rootdir, inifile, inicfg = getcfg(dirs, warnfunc=warnfunc)
	if rootdir is None:
	rootdir = get_common_ancestor([py.path.local(), ancestor])
	is_fs_root = os.path.splitdrive(str(rootdir))[1] == '/'
	if is_fs_root:
	rootdir = ancestor
	return rootdir, inifile, inicfg or {}

		@@ -371,3 +371,9 @@ passed multiple times. The expected format is ``name=value``. For example::


		.. _`#3155`: https://github.com/pytest-dev/pytest/issues/3155

		.. confval:: rootdir

#1642 add rootdir option #3127

#1642 add rootdir option #3127

Conversation

feuillemorte commented Jan 17, 2018 • edited Loading

coveralls commented Jan 17, 2018

coveralls commented Jan 17, 2018

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuillemorte commented Jan 20, 2018

coveralls commented Jan 20, 2018 • edited Loading

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuillemorte Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuillemorte Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuillemorte Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Jan 23, 2018

feuillemorte commented Jan 25, 2018

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuillemorte Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuillemorte Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

nicoddemus Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

feuillemorte commented Jan 17, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

feuillemorte Jan 22, 2018 •

edited

Loading

feuillemorte Jan 22, 2018 •

edited

Loading

feuillemorte Jan 23, 2018 •

edited

Loading

feuillemorte Jan 31, 2018 •

edited

Loading

feuillemorte Jan 31, 2018 •

edited

Loading

nicoddemus Jan 27, 2018 •

edited

Loading