Fix determining rootdir from common_ancestor #1621

blueyed · 2016-06-20T16:48:32Z

TODO:

reference/close issues:
changelog entry

davehunt · 2016-06-21T09:11:17Z

_pytest/config.py

@@ -1136,7 +1140,11 @@ def determine_setup(inifile, args):
                if rootdir.join("setup.py").exists():
                    break
            else:
-                rootdir = ancestor
+                dirs = get_dirs_from_args(args)


I don't think we need this actually. If we pass args directly into getcfg, they are validated as paths

It will be used in other places as well - at least that's what I have locally at the moment.

davehunt · 2016-06-21T09:17:35Z

Here's my documentation updates:

diff --git a/doc/en/customize.rst b/doc/en/customize.rst
index 34e319c..a8e6809 100644
--- a/doc/en/customize.rst
+++ b/doc/en/customize.rst
@@ -29,25 +29,29 @@ project/testrun-specific information.

 Here is the algorithm which finds the rootdir from ``args``:

-- determine the common ancestor directory for the specified ``args``.
+- determine the common ancestor directory for the specified ``args`` that are
+  recognised as paths that exist in the file system. If no such paths are
+  found, the common ancestor directory is set to the current working directory.

-- look for ``pytest.ini``, ``tox.ini`` and ``setup.cfg`` files in the
-  ancestor directory and upwards.  If one is matched, it becomes the
-  ini-file and its directory becomes the rootdir.  An existing
-  ``pytest.ini`` file will always be considered a match whereas
-  ``tox.ini`` and ``setup.cfg`` will only match if they contain
-  a ``[pytest]`` section.
+- look for ``pytest.ini``, ``tox.ini`` and ``setup.cfg`` files in the ancestor
+  directory and upwards.  If one is matched, it becomes the ini-file and its
+  directory becomes the rootdir.

-- if no ini-file was found, look for ``setup.py`` upwards from
-  the common ancestor directory to determine the ``rootdir``.
+- if no ini-file was found, look for ``setup.py`` upwards from the common
+  ancestor directory to determine the ``rootdir``.

-- if no ini-file and no ``setup.py`` was found, use the already
-  determined common ancestor as root directory.  This allows to
-  work with pytest in structures that are not part of a package
-  and don't have any particular ini-file configuration.
+- if no ``setup.py`` was found, look for ``pytest.ini``, ``tox.ini`` and
+  ``setup.cfg`` in each of the specified ``args`` and upwards. If one is
+  matched, it becomes the ini-file and its directory becomes the rootdir.

-Note that options from multiple ini-files candidates are never merged,
-the first one wins (``pytest.ini`` always wins even if it does not
+- if no ini-file was found, use the already determined common ancestor as root
+  directory. This allows to work with pytest in structures that are not part of
+  a package and don't have any particular ini-file configuration.
+
+Note that an existing ``pytest.ini`` file will always be considered a match,
+whereas ``tox.ini`` and ``setup.cfg`` will only match if they contain a
+``[pytest]`` section. Options from multiple ini-files candidates are never
+merged - the first one wins (``pytest.ini`` always wins, even if it does not
 contain a ``[pytest]`` section).

 The ``config`` object will subsequently carry these attributes:

I can push to your fork if you add me as a contributor, or I can push to my fork for you to cherry-pick this change. Or, you can use the patch from above.

nicoddemus · 2016-06-21T14:56:38Z

Guys, how is it going? Make sure to remove "WIP" from the PR's title once this is ready. 😉

blueyed · 2016-06-21T19:50:45Z

@nicoddemus
Not much progress on this today..
I've just seen that the test currently failing was added only recently (in e2e6e31), and probably the special behavior in that case is not compatible / correct anymore?! /cc @taschini

I've added the documentation patch and pushed some other (hopefully rather non-breaking fixup).

nicoddemus · 2016-06-22T07:05:25Z

@blueyed I suggest we sit together to take look.

taschini · 2016-06-22T07:14:34Z

Given the following directory structure:

/tmp/testdir
├── hello
│   └── ns_pkg
│       ├── __init__.py
│       └── hello
│           ├── __init__.py
│           └── test_hello.py
└── world
    └── ns_pkg
        ├── __init__.py
        └── world
            ├── __init__.py
            └── test_world.py

Before this pull request you get:

$ cd /tmp/testdir
$ PYTHONPATH=hello:world py.test -v --pyargs ns_pkg.hello world/ns_pkg
===================================== test session starts ======================================
platform darwin -- Python 2.7.11, pytest-2.9.3.dev0, py-1.4.31, pluggy-0.3.1
cachedir: .cache
rootdir: /tmp/testdir, inifile: 
collected 4 items 

hello/ns_pkg/hello/test_hello.py::test_hello PASSED
hello/ns_pkg/hello/test_hello.py::test_other PASSED
world/ns_pkg/world/test_world.py::test_world PASSED
world/ns_pkg/world/test_world.py::test_other PASSED

=================================== 4 passed in 0.12 seconds ===================================

With your PR you get:

$ cd /tmp/testdir
$ PYTHONPATH=hello:world py.test -v --pyargs ns_pkg.hello world/ns_pkg
============================= test session starts ==============================
platform darwin -- Python 2.7.11, pytest-2.9.3.dev0, py-1.4.31, pluggy-0.3.1
cachedir: world/ns_pkg/.cache
rootdir: /tmp/testdir/world/ns_pkg, inifile: 
collected 4 items

world/ns_pkg/::test_hello <- ../../hello/ns_pkg/hello/test_hello.py PASSED
world/ns_pkg/::test_other <- ../../hello/ns_pkg/hello/test_hello.py PASSED
world/ns_pkg/world/test_world.py::test_world PASSED
world/ns_pkg/world/test_world.py::test_other PASSED
=========================== 4 passed in 0.02 seconds ===========================

Remarks:

Note the difference in root dir: /tmp/testdir vs. /tmp/testdir/world/ns_pkg.

The following two output lines seem to be wrong:

world/ns_pkg/::test_hello <- ../../hello/ns_pkg/hello/test_hello.py PASSED
world/ns_pkg/::test_other <- ../../hello/ns_pkg/hello/test_hello.py PASSED

The directory structure at the top is what you get by running

py.test testing/acceptance_test.py::TestInvocationVariants::test_cmdline_python_namespace_package

The tree contains two packages with tests: ns_pkg.hello and ns_pkg.world; In this example the first one is located by specifying its package name, the second by specifying its directory.
The problem that test_cmdline_python_namespace_package highlights with this PR is independent from Ensure that a module within a namespace package can be found by --pyargs #1597. You would incur into it as soon as you have both module names and path names as arguments to py.test. This use case was not tested before.

taschini · 2016-06-22T07:17:57Z

For your convenience, here's the tar file of the test directory above.

nicoddemus · 2016-06-22T07:33:02Z

Thanks @taschini for the detailed analysis! 😁

We're at the Pytest sprint at the moment, I will sit together with @blueyed later, but I my first impression is that we will change the test slightly to create a pytest.ini on the testdir.tmpdir folder: this is usually the case and, as you mentioned, the rootdir change is not actually related to what this test is about.

taschini · 2016-06-22T07:51:47Z

I haven't looked at the source code of this PR, but from what I gather from the docs by @davehunt, there might be a little problem with the algorithm to determine the common ancestor. As far as I can see, in pseudo-code it works as follows:

paths = [arg for arg in args if os.path.exists(arg)]
common_ancestor = common_prefix(paths or ['.'])

It might be better to have something like

paths = [arg if os.path.exists(arg) else '.' for arg in args]
common_ancestor = common_prefix(paths or ['.'])

That is to say, instead of ignoring arguments that are not recognised as paths that exist in the file system, one might be better off to map them to the cwd.

blueyed · 2016-06-24T16:07:22Z

@taschini
Thanks for the detailed information again.

The "use-cwd-as-fallback" was a good idea, which I've incorporated into the latest fixup: the documentation needs to be revised probably now. I'll finish this tomorrow/tonight.

coveralls · 2016-06-24T16:42:20Z

Coverage increased (+0.009%) to 92.242% when pulling 9b023c9 on blueyed:fix-rootdir-common-ancestor into 0c63762 on pytest-dev:master.

coveralls · 2016-06-25T08:04:42Z

Coverage increased (+0.009%) to 92.242% when pulling 7443261 on blueyed:fix-rootdir-common-ancestor into 0c63762 on pytest-dev:master.

nicoddemus · 2016-06-25T12:33:56Z

Could you guys take a look at the failures? 😁

@davehunt @blueyed

coveralls · 2016-06-25T21:17:51Z

Coverage increased (+0.008%) to 92.294% when pulling f815b9c on blueyed:fix-rootdir-common-ancestor into e024214 on pytest-dev:master.

nicoddemus · 2016-06-26T19:01:41Z

@blueyed can this be merged, or are there still work to be done here?

blueyed · 2016-06-26T19:35:24Z

@nicoddemus
Well, it should get reviewed first.. :)
Especially regarding if the documentation matches the behavior - that's probably not the part for the most recent changes at least..

Then the commits need to be squashed also, of course - I've left it as-is for now to see where we've gone through.
I would like to keep the PR in two commits still, for the doc and the feature, so @davehunt gets credited as well?!

nicoddemus · 2016-06-26T20:17:47Z

_pytest/config.py

+                    dirs, ["pytest.ini", "tox.ini", "setup.cfg"])
+                if rootdir is None:
+                    rootdir = get_common_ancestor([py.path.local(), ancestor])
+                    if os.path.splitdrive(str(rootdir))[1] == os.sep:


I would suggest this to be moved to a local variable to improve legibility:

is_fs_root = os.path.splitdrive(str(rootdir))[1] == os.sep if is_fs_root:

nicoddemus · 2016-06-26T20:21:14Z

LGTM, but I would like a second set of yes to take a look as well.

I would like to keep the PR in two commits still, for the doc and the feature, so @davehunt gets credited as well?!

Sounds good. I think you could that already when you got the time. 😁

nicoddemus · 2016-06-26T20:21:39Z

Also, you could remove WIP: from the PR title once you squash the commits.

coveralls · 2016-06-26T20:29:39Z

Coverage increased (+0.008%) to 92.298% when pulling 53cd1ec on blueyed:fix-rootdir-common-ancestor into 7e78965 on pytest-dev:master.

coveralls · 2016-06-26T20:35:10Z

Coverage increased (+0.009%) to 92.299% when pulling 8261801 on blueyed:fix-rootdir-common-ancestor into 7e78965 on pytest-dev:master.

nicoddemus · 2016-06-26T21:57:20Z

Let's not forget to add a CHANGELOG entry before merging. 😁

davehunt · 2016-06-28T08:53:59Z

I believe this will also resolve #1435 and #1471

blueyed · 2016-07-11T21:00:26Z

@nicoddemus
It's in the TODO of the desc.
I will amend the commit message and add a changelog entry tomorrow.

nicoddemus · 2016-07-11T21:02:14Z

Sounds good, thanks! 👍

RonnyPfannschmidt · 2016-07-23T11:53:27Z

i took a look as well, and feel its ready to merge

@davehunt @blueyed - sorry to be so noisy but any eta on doing the finishing touches you wanted to do

blueyed · 2016-07-23T16:05:51Z

@RonnyPfannschmidt
Thanks for the review.
I will finish in during the EP sprints tomorrow, and we'll merge it then.

davehunt · 2016-07-25T10:28:07Z

I'm happy if @blueyed is happy. Looking forward to having this in 3.0!

tomviner · 2016-07-25T18:34:02Z

_pytest/config.py

        common_ancestor = common_ancestor.dirpath()
    return common_ancestor

+def get_dirs_from_args(args):
+    return [d for d in (py.path.local(x) for x in args if not
+                        str(x).startswith("-")) if d.exists()]


The formatting of this makes it pretty hard to read. Could d for d in be aligned with if d.exists() more clearly?

Would splitting into two statements, or something like this be better:?

return [d for d in (py.path.local(x) for x in args if not str(x).startswith("-")) if d.exists()]

@tomviner
Thanks, applied - also wrapped the first if.

nicoddemus · 2016-08-04T18:35:44Z

@blueyed the only thing missing here is the CHANGELOG? If so I can write the CHANGELOG myself if you are short on time. 😁

blueyed · 2016-08-06T17:37:42Z

@nicoddemus
That would be awesome. My plan was to look closely at existing issues again, and I would have expected that the documentation is not matching the behavior that changed over time - but I'm happy if it does.

coveralls · 2016-08-06T17:39:59Z

Coverage increased (+0.009%) to 92.324% when pulling eb08135 on blueyed:fix-rootdir-common-ancestor into ffb583a on pytest-dev:master.

nicoddemus · 2016-08-06T20:39:02Z

Thanks a ton @blueyed and @davehunt! 😄

matthiasha · 2016-08-08T12:53:38Z

Thanks guys, very much appreciated!

davehunt reviewed Jun 21, 2016
View reviewed changes

This was referenced Jun 21, 2016

Does not get path to database file correctly with "/tmp" as additional dir/arg tarpas/pytest-testmon#45

Closed

2.8.2 - specific test not running if custom parameter is used #1183

Closed

blueyed force-pushed the fix-rootdir-common-ancestor branch from 2ceea07 to 9b023c9 Compare June 24, 2016 15:59

blueyed force-pushed the fix-rootdir-common-ancestor branch from c1ea4b3 to 01b53cd Compare June 25, 2016 12:56

nicoddemus reviewed Jun 26, 2016
View reviewed changes

blueyed force-pushed the fix-rootdir-common-ancestor branch from f815b9c to 53cd1ec Compare June 26, 2016 20:26

blueyed changed the title ~~WIP: fix determining rootdir from common_ancestor=='/'~~ Fix determining rootdir from common_ancestor Jun 26, 2016

blueyed force-pushed the fix-rootdir-common-ancestor branch from 00d90da to 8261801 Compare June 26, 2016 20:31

blueyed self-assigned this Jul 11, 2016

blueyed added this to the 3.0 milestone Jul 11, 2016

davehunt mentioned this pull request Jul 12, 2016

getcfg's parsing of command line arguments #574

Closed

diegorusso mentioned this pull request Jul 23, 2016

Path in pytest options distracts pytest.ini discovery #1435

Closed

tomviner reviewed Jul 25, 2016
View reviewed changes

blueyed and others added 2 commits August 6, 2016 19:35

Fix determining rootdir from common_ancestor

a289142

Update documentation to describe expected rootdir behaviour

eb08135

blueyed force-pushed the fix-rootdir-common-ancestor branch from 8261801 to eb08135 Compare August 6, 2016 17:35

blueyed assigned nicoddemus and unassigned blueyed Aug 6, 2016

nicoddemus mentioned this pull request Aug 6, 2016

Fix rootdir common ancestor #1796

Merged

nicoddemus merged commit eb08135 into pytest-dev:master Aug 6, 2016

This was referenced Aug 6, 2016

Using positional arg/dir outside of PWD does not load options from explicit plugin #1594

Closed

Py.test improperly handles command line options with spaces #1471

Closed

matthiasha mentioned this pull request Aug 9, 2016

#1435 rootdir discovery: re-open #1801

Closed

matthiasha mentioned this pull request Aug 19, 2016

Documentation update for rootdir discovery #1819

Merged

davehunt mentioned this pull request Mar 21, 2017

Problem reading --variables from pytest.ini if --driver-path specified pytest-dev/pytest-selenium#105

Closed

Fix determining rootdir from common_ancestor #1621

Fix determining rootdir from common_ancestor #1621

Conversation

blueyed commented Jun 20, 2016 • edited Loading

davehunt Jun 21, 2016 • edited Loading

Choose a reason for hiding this comment

blueyed Jun 21, 2016

Choose a reason for hiding this comment

davehunt commented Jun 21, 2016

nicoddemus commented Jun 21, 2016

blueyed commented Jun 21, 2016

nicoddemus commented Jun 22, 2016 • edited Loading

taschini commented Jun 22, 2016

taschini commented Jun 22, 2016 • edited Loading

nicoddemus commented Jun 22, 2016 • edited Loading

taschini commented Jun 22, 2016

blueyed commented Jun 24, 2016

coveralls commented Jun 24, 2016 • edited Loading

coveralls commented Jun 25, 2016 • edited Loading

nicoddemus commented Jun 25, 2016

coveralls commented Jun 25, 2016 • edited Loading

nicoddemus commented Jun 26, 2016

blueyed commented Jun 26, 2016

nicoddemus Jun 26, 2016

Choose a reason for hiding this comment

blueyed Jun 26, 2016

Choose a reason for hiding this comment

nicoddemus commented Jun 26, 2016

nicoddemus commented Jun 26, 2016

coveralls commented Jun 26, 2016 • edited Loading

coveralls commented Jun 26, 2016 • edited Loading

nicoddemus commented Jun 26, 2016 • edited Loading

davehunt commented Jun 28, 2016

blueyed commented Jul 11, 2016

nicoddemus commented Jul 11, 2016

RonnyPfannschmidt commented Jul 23, 2016

blueyed commented Jul 23, 2016

davehunt commented Jul 25, 2016 • edited Loading

tomviner Jul 25, 2016

Choose a reason for hiding this comment

blueyed Aug 6, 2016

Choose a reason for hiding this comment

nicoddemus commented Aug 4, 2016 • edited Loading

blueyed commented Aug 6, 2016

coveralls commented Aug 6, 2016 • edited Loading

nicoddemus commented Aug 6, 2016

matthiasha commented Aug 8, 2016

blueyed commented Jun 20, 2016 •

edited

Loading

davehunt Jun 21, 2016 •

edited

Loading

nicoddemus commented Jun 22, 2016 •

edited

Loading

taschini commented Jun 22, 2016 •

edited

Loading

nicoddemus commented Jun 22, 2016 •

edited

Loading

coveralls commented Jun 24, 2016 •

edited

Loading

coveralls commented Jun 25, 2016 •

edited

Loading

coveralls commented Jun 25, 2016 •

edited

Loading

coveralls commented Jun 26, 2016 •

edited

Loading

coveralls commented Jun 26, 2016 •

edited

Loading

nicoddemus commented Jun 26, 2016 •

edited

Loading

davehunt commented Jul 25, 2016 •

edited

Loading

nicoddemus commented Aug 4, 2016 •

edited

Loading

coveralls commented Aug 6, 2016 •

edited

Loading