Fix determining rootdir from common_ancestor #1621

Merged
merged 2 commits into from Aug 6, 2016

Projects

None yet

8 participants

@blueyed
Contributor
blueyed commented Jun 20, 2016 edited

TODO:

@davehunt davehunt and 1 other commented on an outdated diff Jun 21, 2016
_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)
@davehunt
davehunt Jun 21, 2016 edited Contributor

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

@blueyed
blueyed Jun 21, 2016 Contributor

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

@davehunt
Contributor

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
Member

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

@blueyed
Contributor
blueyed commented Jun 21, 2016

@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
Member
nicoddemus commented Jun 22, 2016 edited

@blueyed I suggest we sit together to take look.

@taschini
Contributor

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 #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
Contributor
taschini commented Jun 22, 2016 edited

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

@nicoddemus
Member
nicoddemus commented Jun 22, 2016 edited

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
Contributor

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
Contributor
blueyed commented Jun 24, 2016

@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
coveralls commented Jun 24, 2016 edited

Coverage Status

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

@coveralls
coveralls commented Jun 25, 2016 edited

Coverage Status

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

@nicoddemus
Member

Could you guys take a look at the failures? ๐Ÿ˜

@davehunt @blueyed

@coveralls
coveralls commented Jun 25, 2016 edited

Coverage Status

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

@nicoddemus
Member

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

@blueyed
Contributor
blueyed commented Jun 26, 2016

@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 nicoddemus and 1 other commented on an outdated diff Jun 26, 2016
_pytest/config.py
rootdir, inifile, inicfg = getcfg(
[ancestor], ["pytest.ini", "tox.ini", "setup.cfg"])
if rootdir is None:
for rootdir in ancestor.parts(reverse=True):
if rootdir.join("setup.py").exists():
break
else:
- rootdir = ancestor
+ rootdir, inifile, inicfg = getcfg(
+ 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:
@nicoddemus
nicoddemus Jun 26, 2016 Member

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:
@blueyed
blueyed Jun 26, 2016 Contributor

Done.

@nicoddemus
Member

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
Member

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

@coveralls
coveralls commented Jun 26, 2016 edited

Coverage Status

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

@blueyed blueyed changed the title from WIP: fix determining rootdir from common_ancestor=='/' to Fix determining rootdir from common_ancestor Jun 26, 2016
@coveralls
coveralls commented Jun 26, 2016 edited

Coverage Status

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

@nicoddemus
Member
nicoddemus commented Jun 26, 2016 edited

Let's not forget to add a CHANGELOG entry before merging. ๐Ÿ˜

@davehunt
Contributor

I believe this will also resolve #1435 and #1471

@blueyed
Contributor
blueyed commented Jul 11, 2016

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

@blueyed blueyed self-assigned this Jul 11, 2016
@blueyed blueyed added this to the 3.0 milestone Jul 11, 2016
@nicoddemus
Member

Sounds good, thanks! ๐Ÿ‘

@RonnyPfannschmidt
Contributor

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
Contributor
blueyed commented Jul 23, 2016

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

@davehunt
Contributor
davehunt commented Jul 25, 2016 edited

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

@tomviner tomviner and 1 other commented on an outdated diff Jul 25, 2016
_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()]
@tomviner
tomviner Jul 25, 2016 Contributor

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()]
@blueyed
blueyed Aug 6, 2016 Contributor

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

@nicoddemus
Member
nicoddemus commented Aug 4, 2016 edited

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

blueyed and others added some commits Jun 20, 2016
@blueyed blueyed Fix determining rootdir from common_ancestor a289142
@davehunt @blueyed davehunt Update documentation to describe expected rootdir behaviour
eb08135
@blueyed
Contributor
blueyed commented Aug 6, 2016

@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.

@blueyed blueyed assigned nicoddemus and unassigned blueyed Aug 6, 2016
@coveralls
coveralls commented Aug 6, 2016 edited

Coverage Status

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

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicoddemus
Member

Thanks a ton @blueyed and @davehunt! ๐Ÿ˜„

@matthiasha
Contributor

Thanks guys, very much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment