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

use of 'pytest' in setup.cfg collides with 'pytest' distutils command #567

Closed
pytestbot opened this Issue Aug 22, 2014 · 20 comments

Comments

Projects
None yet
8 participants
@pytestbot

pytestbot commented Aug 22, 2014

Originally reported by: Jason R. Coombs (BitBucket: jaraco, GitHub: jaraco)


In pytest-runner 2, I adapted pytest-runner to support the 'pytest' distutils command. However, because the pytest project itself uses the [pytest] section of setup.cfg, that conflicts with the same section for the 'pytest' distutils command.

Using the 'pytest' distutils command in a project which specifies, for example, 'norecursedirs', the runner will fail because the pytest-runner distutils command doesn't recognize that parameter (or any other ini options). An example error message is:

error: error in setup.cfg: command 'PyTest' has no such option 'norecursedirs'

I see a few options here:

  1. Just use the 'ptr' command name.
  2. Have pytest runner distutils command disregard all unrecognized options.
  3. Have pytest runner distutils command implement and ignore all of the options that might be presented in setup.cfg. It would need to do this before pytest is present (because args are processed before dependencies such as pytest are downloaded/imported).
  4. Have pytest drop support for setup.cfg.
  5. Require that users not use setup.cfg for pytest ini options (only support pytest.ini and tox.ini for pytest options when running under distutils).

I suspect there are other options, too. I'm not particularly happy with any of those options, but I'm leaning toward (2).

I've filed the ticket here with pytest for two reasons:

  • I want the implementation to be as acceptable as possible for the pytest project to endorse it as a viable integration mechanism.
  • I believe the use of the [pytest] section in the setup.cfg by the pytest library is a violation of the explicit expectation that those sections are meant for distutils commands.

@hpk42 What is your reaction? What would you suggest?


@pytestbot

This comment has been minimized.

pytestbot commented Sep 2, 2014

Original comment by holger krekel (BitBucket: hpk42, GitHub: hpk42):


The reason why pytest looks in setup.cfg (as does tox) is that i'd eventually like to have all project specific metadata in a setup.cfg. I don't know how much people use a "[pytest]" section in setup.cfg these days, however. And i don't know how much of a worthwhile goal it is. Dropping support for [pytest] could only happen for the second next major release (it would be deprecated in 2.7). So i guess option "2" is the best one for you for now, indeed.

@pytestbot

This comment has been minimized.

pytestbot commented Sep 2, 2014

Original comment by Jason R. Coombs (BitBucket: jaraco, GitHub: jaraco):


I happen to use the "[pytest]" section in setup.cfg extensively, but that's just one use case. It seemed like the most canonical place to add recommended options for a project's pytest runs. Indeed, it sounds as if that would have been your recommendation as well. If the project chooses to deprecate that, it could continue to support options in the setup.cfg, but under a different section (perhaps something with spaces that would be unlikely to collide with a command). Or pytest could prefer use of one of the other files instead.

I'll explore the use of suppressing unknown options to the command (option 2). I don't know how straightforward that will be, because distutils uses its own port of getopt with which I'm not familiar. It's something I need to learn, though, to solve other setuptools issues.

@pytestbot

This comment has been minimized.

pytestbot commented Feb 3, 2015

Original comment by Jason R. Coombs (BitBucket: jaraco, GitHub: jaraco):


I looked into the code for pytest-runner and pytest, but it looks like it'll be impossible to adequately bypass this issue in pytest-runner.

First, when pytest loads settings from the .ini file, it loads the settings into a variable and references the keys throughout the code, so there's no authoritative list of options that might be expected to exist. That is, there's no way pytest-runner could introspect pytest to determine which options to whitelist, so any white list would need to be updated manually and periodically, and the list of options could be found anywhere in pytest.

Second, I looked into whitelisting any unrecognized option, but that's not possible either because pytest-runner is a distutils command, and commands are parsed in distutils.dist.Distribution, part of the stdlib. I considered adding a __getattr__ wrapper such that any option was present, like so:

diff --git a/ptr.py b/ptr.py
--- a/ptr.py
+++ b/ptr.py
@@ -6,6 +6,7 @@
 import shlex as _shlex
 import contextlib as _contextlib
 import sys as _sys
+import warnings as _warnings

 import setuptools.command.test as orig

@@ -103,3 +104,19 @@
        """
        with _save_argv(_sys.argv[:1] + self.addopts):
            self.result_code = __import__('pytest').main()
+
+   def __getattr__(self, key):
+       """
+       Because pytest (the project) was recommending or at least supporting
+       the use of setup.cfg for options to the py.test native test runner,
+       those options are now incorrectly applied to the setuptools pytest
+       command. See Pytest #567 for details.
+
+       This method works around the problem by making _any_ option appear
+       to be defined as an attribute on the command. See
+       `distutils.dist.Distribution._set_command_options` to see where
+       this workaround takes effect.
+       """
+       tmpl = "Suppressed error with option {key} to pytest command"
+       _warnings.warn(tmpl.format(**locals()))
+       return None

That doesn't work, though, because command options are not the only aspect that calls and depends on hasattr.

Third, I considered just trapping the error message, but as far as I can tell, there's no way to do that from the API exposed by distutils.

So I'm left feeling that options (2) and (3) aren't really viable without a refactoring of distutils, which is its own quagmire. And indeed, those options would only serve as a workaround and wouldn't resolve the essential conflict.

The reason why pytest looks in setup.cfg (as does tox) is that i'd eventually like to have all project specific metadata in a setup.cfg.

I see this goal in direct conflict with a 'pytest' command. Could pytest (the project) consider using a different section in setup.cfg to avoid the collision? If pytest wants to own the [pytest] section in setup.cfg, then pytest-runner should use another name for its distutils command. Assuming the current state of the art for distutils and setuptools, I don't see any other options.

@pytestbot

This comment has been minimized.

pytestbot commented Mar 1, 2015

Original comment by Ronny Pfannschmidt (BitBucket: RonnyPfannschmidt, GitHub: RonnyPfannschmidt):


i wonder if renaming the section to [tool:pytest] might help, in the long run

@pytestbot

This comment has been minimized.

pytestbot commented Mar 2, 2015

Original comment by Jason R. Coombs (BitBucket: jaraco, GitHub: jaraco):


Yes, something like that would work.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Jan 1, 2016

We should deprecate and drop usage in setup.cfg

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jan 6, 2016

Related to #1311

kynan added a commit to kynan/nbstripout that referenced this issue Feb 4, 2016

jbarlow83 added a commit to jbarlow83/OCRmyPDF that referenced this issue Feb 16, 2016

Kaniabi added a commit to Kaniabi/travispy that referenced this issue Mar 21, 2016

sathishscripts pushed a commit to sathishscripts/kallitheaGit that referenced this issue Apr 1, 2016

pytest migration: switch to pytest; remove nose support
Make pytest the default test runner and remove support for nose.
Tests can be run using:
   - py.test
   - python setup.py test

The pytest configuration needs to move from setup.cfg to pytest.ini to support
this - see pytest-dev/pytest#567 and
https://bitbucket.org/pytest-dev/pytest-runner/issues/7/support-all-pytest-commands .
@ches

This comment has been minimized.

ches commented Jul 11, 2016

All said up to this point in this thread, I probably would have voted for option 1: don't use pytest as pytest-runner's distutils command name. A project previously using [pytest] in setup.cfg without issue has a broken experience when trying to add pytest-runner to the project and running setup.py test—from that perspective, the user considers pytest-runner as the broken piece, and the responsibility lies with pytest-runner to fix that at the peril of losing those users. It's not ideal, but it's a straightforward solution, and I wager nearly everyone immediately aliases to test anyway.

If there are other problems, though (#1311), then I still like @RonnyPfannschmidt's suggestion of changing to a prefix like [tool:pytest] if that is workable. My preference for this probably aligns with @hpk42's original motive mentioned above—proliferation of too many config and metadata files for tools. Others support setup.cfg as well (flake8, nose I believe), so it was nice to consolidate some of the root directory cruft in new projects which not only feels messy but IMO creates confusion for newcomers less familiar with the Python ecosystem. This is of course a subjective issue beyond the scope of this matter, but using setup.cfg seemed an innocent solution.

If this is deemed an abuse of distutils' turf, I can see the argument to remove setup.cfg support altogether, but right or wrong it has become pretty widespread at this point. Having originally opened this issue and being a PyPA contributor, maybe @jaraco has more opinion about this now.

@foxx's problem report in #1311 is a little unclear to me: it appears he was using pytest-runner, which possibly conflates things. Not sure if the addopts issue was when invoking through the runner exclusively or not.

@RonnyPfannschmidt RonnyPfannschmidt added this to the 3.0 milestone Jul 11, 2016

@ches

This comment has been minimized.

ches commented Jul 11, 2016

FWIW, Coverage is another that supports setup.cfg, using a prefix pattern.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Jul 12, 2016

@ches thanks for the reference, but as you can see, the "sane" tools do use a prefix while pytest does not

@jaraco

This comment has been minimized.

Contributor

jaraco commented Jul 14, 2016

My opinion remains basically the same, though it's crystallized even more now. The namespace for non-prefixed names in setup.cfg is practically if not explicitly reserved for Distutils command names, so pytest should drop support for it or use a prefixed name.

I would be happy to switch pytest-runner back to using the 'ptr' name (it's currently still supported), but only if there's a consensus to do so, and I still feel like that's a less suitable solution than the alternative.

@ssbarnea

This comment has been minimized.

ssbarnea commented Jul 24, 2016

Hi guys! I ended up to this page shortly after trying to implement the good practices from http://docs.pytest.org/en/latest/goodpractices.html

What should I do to pass over this issue? ...and be able to run tests using the standard python setup.py test. Do we have any proof-of-concept examples?

I am using pytest and partially tox (tox when doing matrix builds locally and just pytest when on travis as it has its own matrix setup). Thanks.

@ches

This comment has been minimized.

ches commented Jul 24, 2016

@ssbarnea You should be fine following the advice of the documentation to use pytest-runner, just note that you can't use a [pytest] section in setup.cfg unless the options you're setting there are explicitly supported by pytest-runner. You will instead want to use pytest.ini or tox.ini for your general pytest configuration.

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Aug 5, 2016

This one seems like an easy, backwards-incompatible change for 3.0 - change [pytest] to [tool:pytest] in setup.cfg and warn/deprecate about [pytest]?

@jaraco

This comment has been minimized.

Contributor

jaraco commented Aug 5, 2016

@The-Compiler And introduce forward-compatibilty for [tool:pytest] ASAP.

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Aug 5, 2016

@jaraco Yeah - we want to release 3.0 ASAP, I was looking at which issues really should go into 3.0 (because otherwise they'd need to be postponed to 4.0).

@hackebrot

This comment has been minimized.

Member

hackebrot commented Aug 16, 2016

Anyone capacities to pick this one up?

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 16, 2016

@hackebrot I have to remove invocation fixtures first and was going to get this if nobody volunteers until then, but if you have the time and want to tackle this it would be great.

Just to make sure I understand, the plan is to add support for [tool:pytest] in setup.cfg and make that the official and preferred way to specify pytest options in a setup.cfg file. At the same time, emit a deprecation warning if it encounters a plain [pytest] section in a setup.cfg file. Nothing should be done regarding the current way to specify options in pytest.ini or tox.ini files.

Is my understanding correct?

@hackebrot

This comment has been minimized.

Member

hackebrot commented Aug 16, 2016

I am busy with #1795 atm, but I'd like to see this being worked on. 😄

Can't wait to use pytest 3.0. 🚀

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Aug 17, 2016

Support [tool:pytest] in setup.cfg files
Also deprecate [pytest] usage in setup.cfg files

Fix pytest-dev#567

This was referenced Nov 12, 2017

@justanr justanr referenced this issue Mar 20, 2018

Merged

Auth services #421

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