Skip to content

Conversation

@skylarjhdownes
Copy link
Contributor

Fixes #1937

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.079% when pulling f1faaea on skylarjhdownes:master into 863b7d0 on pytest-dev:master.


If you place a conftest.py file in the root directory of your project
(as determined by pytest, see above.) pytest will run tests against
the code below that directory by adding it to your sys.path instead of
Copy link
Member

Choose a reason for hiding this comment

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

After a quick look I couldn't find any evidence in the sources of that... perhaps @RonnyPfannschmidt can chime in?

Copy link
Member

Choose a reason for hiding this comment

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

This is kind of a hack, which admittedly I've used and even advertised in the past. But it's a side-effect on the not very ideal way how we import conftest.py files currently. I'm rather hesitant to document it officially as then it becomes something we can not break in the future, while this is a domain where it would be very nice to clean up these side-effects. We shouldn't be doing such unexpected things with sys.path!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just now getting back to this... Should I remove these paragraphs from the pull request then? They would have saved me a heck of a lot of grief. Perhaps the conftest.py thing should be mentioned as an unsupported side effect somehow, instead of a supported feature?

Either way, the thing I really think should be more clearly stated is that the user is expected to install the python code to be tested in develop mode. As a newb coming from using nose's sys.path hackery, that pattern was not at all obvious to me. I'm happy to shelve the conftest.py piece of this and focus on documenting the develop mode pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Either way, the thing I really think should be more clearly stated is that the user is expected to install the python code to be tested in develop mode.

I think this is the heart of the matter; this is good practice and will be useful with any Python project (at least OS that is). So I think removing the sys.path hackery altogether is best in the long term.

Copy link
Member

Choose a reason for hiding this comment

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

As a newb coming from using nose's sys.path hackery

What if we mention this detail in nose.rst? This way users coming from nose will have an obvious place to understand the difference regarding how pytest finds the source code. We should instruct the user to create a virtual environment though, not mention about the sys.path implementation detail.

@nicoddemus
Copy link
Member

Btw, Travis failed with:

/home/travis/build/pytest-dev/pytest/doc/en/getting-started.rst:189: WARNING: Title underline too short.

pip install -e . # install package using setup.py in editable mode
# similar to running python setup.py develop
This installs your package with a symlink to your development code
Copy link
Member

Choose a reason for hiding this comment

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

Nice!


pytest --fixtures # shows builtin and custom fixtures

Contributing tests to an existing project
Copy link
Member

Choose a reason for hiding this comment

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

To be honest I would rather remove this entire session... it seems distracting and very narrow for a specific user case. I understand that was your use case, but nowadays IMHO people are used to virtualenvs in general (that includes venv and conda), so this in the end reads confusing to me, specially on the getting-started session.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree on the 'people being used to virtualenvs' point, many users coming to a getting started section will not be used to any of the basic concepts of Python or unit testing. This being too specific/narrow makes sense to me though. It's kinda just tacked on the end at the moment.

Seems like running the tests on a new project will be enough people's first experience with pytest that there should be a walkthrough for it somewhere, and the getting started section was the first place I looked for it. Maybe there should be a page on it linked at the bottom?

Copy link
Member

Choose a reason for hiding this comment

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

I disagree on the 'people being used to virtualenvs' point, many users coming to a getting started section will not be used to any of the basic concepts of Python or unit testing.

You may be right, I'm probably biased on this. But perhaps this should be moved somewhere in goodpractices then? virtualenv, tox, etc are all mentioned there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pip install -e/setup.py develop concept is mentioned there already, and I don't think the whole "contributing to an existing repo" concept fits there. Hmm.

The only other place the develop concept is mentioned is the nose page.

@nicoddemus
Copy link
Member

Hey @skylarjhdownes thanks for following up!

flavor of version control and (optionally) setting up a virtualenv
you will want to run::

pip install -e # Environment dependent alternatives include
Copy link
Member

Choose a reason for hiding this comment

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

I think we can add the full instructions here:

cd <repository>
pip install -e .

Copy link
Member

Choose a reason for hiding this comment

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

(Note the . at the end)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.079% when pulling 330a2f6 on skylarjhdownes:master into 863b7d0 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.079% when pulling 34117be on skylarjhdownes:master into 863b7d0 on pytest-dev:master.

Moved the "Contributing tests to an existing project" section to it's own page.
CHANGELOG.rst Outdated
*

*
* Added documentation related to issue #1937
Copy link
Member

Choose a reason for hiding this comment

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

Please make that reference a link, see #1934 above


If you place a conftest.py file in the root directory of your project
(as determined by pytest, see above.) pytest will run tests against
the code below that directory by adding it to your sys.path instead of
Copy link
Member

Choose a reason for hiding this comment

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

Either way, the thing I really think should be more clearly stated is that the user is expected to install the python code to be tested in develop mode.

I think this is the heart of the matter; this is good practice and will be useful with any Python project (at least OS that is). So I think removing the sys.path hackery altogether is best in the long term.


If you place a conftest.py file in the root directory of your project
(as determined by pytest, see above.) pytest will run tests against
the code below that directory by adding it to your sys.path instead of
Copy link
Member

Choose a reason for hiding this comment

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

As a newb coming from using nose's sys.path hackery

What if we mention this detail in nose.rst? This way users coming from nose will have an obvious place to understand the difference regarding how pytest finds the source code. We should instruct the user to create a virtual environment though, not mention about the sys.path implementation detail.

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.

Please see my comments above, I "accepted" the review by accident. 😁

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 92.949% when pulling 1ab1962 on skylarjhdownes:master into 863b7d0 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 92.949% when pulling 3d211da on skylarjhdownes:master into 863b7d0 on pytest-dev:master.

CHANGELOG.rst Outdated
*

*
* Added documentation related to issue (`#1937`_)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like so?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but you still need to add a link down below:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. I see.

rjprins added a commit to rjprins/pytest that referenced this pull request Jan 13, 2017
rjprins added a commit to rjprins/pytest that referenced this pull request Jan 13, 2017
Using pytest with an existing test suite
===========================================

Pytest can be used with most existing test suites, but it's
Copy link
Contributor

Choose a reason for hiding this comment

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

its*

@RonnyPfannschmidt
Copy link
Member

@skylarjhdownes @nicoddemus it seems we let this one bit rot, can we try to revive it ?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.69% when pulling 3aa4fb6 on skylarjhdownes:master into c734a2d on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.69% when pulling f7caa56 on skylarjhdownes:master into c734a2d on pytest-dev:master.

@nicoddemus
Copy link
Member

Thanks a lot @skylarjhdownes for following up!

Fixed the linting error and some small formatting issues, so I think this is now good to go! 👍

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.69% when pulling 4585238 on skylarjhdownes:master into c734a2d on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.69% when pulling 4585238 on skylarjhdownes:master into c734a2d on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.69% when pulling f2300fb on skylarjhdownes:master into c734a2d on pytest-dev:master.

@nicoddemus nicoddemus merged commit 8c69d5c into pytest-dev:master Apr 7, 2017
@RonnyPfannschmidt
Copy link
Member

@skylarjhdownes @nicoddemus great job in getting this finished up so quickly - thanks

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.

6 participants