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

Deprecate the test command #1878

Merged
merged 1 commit into from Oct 22, 2019

Conversation

@jdufresne
Copy link
Contributor

jdufresne commented Oct 19, 2019

Refs #1684

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details
@jdufresne jdufresne force-pushed the jdufresne:deprecate-test-suite branch from c1b7869 to 3ae6624 Oct 19, 2019
Copy link
Member

pganssle left a comment

Would it be possible to add some tests for this?

Here is the relevant section of the PR deprecating the upload and register commands.

@@ -2142,6 +2148,12 @@ distutils configuration file the option will be added to (or removed from).
``test`` - Build package and run a unittest suite
=================================================

.. warning::
**test** is deprecated in favor of using `tox
<https://tox.readthedocs.org/>`_, `unittest

This comment has been minimized.

Copy link
@pganssle

pganssle Oct 21, 2019

Member

Should be https://tox.readthedocs.io

Also, I think we can reword this a bit - I'd prefer it to be clear that tox is the actual solution here, and the other two are test runners (plus pytest should probably be prioritized over unittest), I'd word it as:

.. warning::
    ``test`` is deprecated in favor of using `tox
    <https://tox.readthedocs.io>`_, or directly invoking your test runner (e.g.
    `pytest <https://docs.pytest.org>`_ or `unittest
    <https://docs.python.org/library/unittest.html>`_).

Another wording that might put our thumbs on the scale in favor of tox a bit more lightly would be:

.. warning::
  ``test`` is deprecated and will be removed in a future version. Users looking
  for a generic test entry point independent of test runner are encouraged to
  use `tox <https://tox.readthedocs.io>`_.
@@ -0,0 +1 @@
Deprecate ``test`` commands.

This comment has been minimized.

Copy link
@pganssle

pganssle Oct 21, 2019

Member
Suggested change
Deprecate ``test`` commands.
Formally deprecated the ``test`` command, with the recommendation that users migrate to ``tox``.
@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Oct 21, 2019

Thank you @jdufresne for your work! I really appreciate the way you've been quietly (at least in my corner of the universe) and methodically improving the overall health of the Python ecosystem.

Provide a warning to users. Suggest using tox as an alternative generic
entry point.

Refs #1684
@jdufresne jdufresne force-pushed the jdufresne:deprecate-test-suite branch from 3ae6624 to cd84510 Oct 22, 2019
@jdufresne

This comment has been minimized.

Copy link
Contributor Author

jdufresne commented Oct 22, 2019

Thanks for the review. I have implemented your suggestions and and added tests. I went with the stronger warning as I really like the "and will be removed in a future version" phrasing, but I'm happy to switch back to the softer wording if preferred. If you're looking for anything else out of the tests, let me know and I'll add them.

And thanks very much for the kind words! 🙂

@pganssle pganssle merged commit a768f8d into pypa:master Oct 22, 2019
6 checks passed
6 checks passed
Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 84.74%)
Details
codecov/project 84.79% (+0.05%) compared to 1362c8c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@pganssle pganssle mentioned this pull request Oct 22, 2019
@graingert

This comment has been minimized.

Copy link

graingert commented Oct 22, 2019

It may be worth locking this PR and the associated issue and directing people to https://discuss.python.org/

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Oct 22, 2019

@graingert Hm.. You're right that this will likely be a magnet for people upset by this news, but I think maybe we should leave it open. That will at least give people who don't want to follow possible objections a single thread (or maybe two threads) to ignore, and some people may indeed have constructive comments to make on the specifics of the implementation.

Let's revisit the issue of locking the thread later if the thread becomes toxic (anyone who feels that the thread is going that way, please feel free to send me an e-mail).

For anyone coming to this PR to complain or because someone linked it: please be respectful and try to understand the full history of the issue. I know it is voluminous, but please read all of #1684 before commenting.

@jdufresne jdufresne deleted the jdufresne:deprecate-test-suite branch Oct 22, 2019
@jdufresne jdufresne restored the jdufresne:deprecate-test-suite branch Oct 22, 2019
@jdufresne jdufresne deleted the jdufresne:deprecate-test-suite branch Oct 22, 2019
@FRidh

This comment has been minimized.

Copy link

FRidh commented Oct 24, 2019

It's a pity this PR wasn't mentioned in the original discussion about removing the test command.
#931 and neither at https://discuss.python.org/t/proposal-for-tests-entry-point-in-pyproject-toml/2077.

I don't think we should encourage users to use tox for this, as it's a too heavy solution for those simply wanting to test their installation, and would appreciate if the encouraged was replaced with may want. OTOh I do think encouraging tox is good because it keeps it uniform which was also the motivation of my discuss.python.org proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.