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

Expand "Testing" section in contributing guide #633

Merged
merged 8 commits into from
May 27, 2020

Conversation

bhrutledge
Copy link
Contributor

@bhrutledge bhrutledge commented May 23, 2020

Following a similar pattern to "Code style" in #631. I started this while working on that PR, then realized it would be better on its own. I also attempted to address #543 (at least partially) by adding explicit support for PYTEST_ADDOPTS.

Does this accurately describe the expected testing workflow for new contributors? My own workflow is to run pytest directly, using a virtual environment created with tox --devenv venv; I'm tempted to open an PR that documents that approach for discussion.

@deveshks, as a new contributor, what do you think? Any suggestions?

@bhrutledge bhrutledge requested a review from jaraco May 23, 2020 11:24
tox.ini Outdated Show resolved Hide resolved
@bhrutledge bhrutledge changed the title Expand "Testing" docs in contributing guide Expand "Testing" section in contributing guide May 23, 2020
docs/contributing.rst Outdated Show resolved Hide resolved
tox.ini Outdated
pytest --cov=twine --cov-context=test \
--cov-report term-missing --cov-report html \
{posargs:tests}
pytest {posargs:--cov=twine --cov-context=test --cov-report term-missing --cov-report html tests}
Copy link
Member

Choose a reason for hiding this comment

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

I've never liked this form, where lots of default arguments are passed if no arguments are passed... because then passing any other argument, related or unrelated, will disable all of the defaults. Ideally, supplying a parameter such as -k integration would not disable coverage (or potentially other options that get added here later). That's why I put default arguments for coverage in .coverage or pytest.ini. That said, this usage is acceptable.

Copy link
Contributor Author

@bhrutledge bhrutledge May 24, 2020

Choose a reason for hiding this comment

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

@jaraco I was trying to strike a balance between running the whole test suite with coverage, and running a subset of tests. In the latter case, I generally care less about coverage, and find the text report distracting.

I hadn't thought about putting them in pytest.ini; I'll take a look at that. Off the top of my head, it seems like --cov and --cov-context could go there, while --cov-report could stay here.

As I noted earlier, another option might be to have a separate coverage testenv, e.g.:

https://github.com/sphinx-doc/sphinx/blob/master/tox.ini
https://github.com/jazzband/pip-tools/blob/master/tox.ini

Are there any cons to that approach?

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 moved some options to pytest.ini in 22ece54.

@@ -86,12 +86,42 @@ modules only, rather than individual classes or functions.
Testing
^^^^^^^

Twine is tested against Python versions 3.6, 3.7, and 3.8. To run these tests
locally, you will need these versions of Python installed on your machine.
We use `pytest`_ for writing and running tests.
Copy link
Member

Choose a reason for hiding this comment

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

I feel like these docs, while they might help a new user, are largely general-purpose guidance. If it were me, to avoid having to keep these docs up-to-date, I'd direct users to a common place to learn about tox and pytest and maybe link to the places in the twine code where non-default behaviors are defined. More docs means more maintenance, so I aim to provide basic directions ("run tox") and leave it an an exercise for the user to follow the link to tox to learn how to install and use it. That said, there's nothing objectionable here.

Copy link
Contributor Author

@bhrutledge bhrutledge May 24, 2020

Choose a reason for hiding this comment

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

@jaraco I definitely didn't want to duplicate information from the official docs from the tools, but rather call out the places where twine has non-default behaviors that streamline testing workflow.

A middle-ground that might be more maintainable would be move to move some these examples to comments in tox.ini, and link to it as you suggest, but I feel like I don't see that done very often.

@@ -35,7 +35,7 @@ Now, in your virtual environment, ``twine`` is pointing at your local copy, so
when you make changes, you can easily see their effect.

We use `tox`_ to run tests, check code style, and build the documentation.
To install ``tox`` in your active your virtual environment, run:
To install ``tox`` in your active virtual environment, run:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following up on https://github.com/pypa/twine/pull/633/files#r429546104 and https://github.com/pypa/twine/pull/633/files#r429551908:

Except for the typo, I'm inclined to leave this instruction as-is for now, since we resolved the issue @deveshks was having. This section seems sufficient a new contributor who only has Python 3 installed to set up a complete development environment for Twine.

@bhrutledge bhrutledge merged commit 20b735f into pypa:master May 27, 2020
@bhrutledge bhrutledge deleted the testing-docs branch May 27, 2020 09:57
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.

4 participants