pytest-dev / pytest

The pytest framework makes it easy to write small tests, yet scales to support complex functional testing
https://pytest.org
MIT License
11.93k stars 2.65k forks source link

--fixtures and --markers are broken when there's a required option added in pytest_addoption() #3042

Open segevfiner opened 6 years ago

segevfiner commented 6 years ago

Similar to https://github.com/pytest-dev/pytest/issues/1999

When you add a required option in pytest_addoption the --fixtures and --markers arguments cannot be used without specifying the required options. This was fixed for --help in https://github.com/pytest-dev/pytest/pull/2458.

Example

conftest.py

import pytest

def pytest_addoption(parser):
    parser.addoption("--please-work", action="store_true", required=True)

@pytest.fixture
def spam():
    return "spam"

Output

fixtures_required> pytest
usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: argument --please-work is required

>pytest --help 
usage: pytest [options] [file_or_dir] [file_or_dir] [...]

positional arguments:
  file_or_dir

general:
  -k EXPRESSION         only run tests which match the given substring
                        expression. An expression is a python evaluatable
                        expression where all names are substring-matched
                        against test names and their parent classes. Example:
                        -k 'test_method or test_other' matches all test
                        functions and classes whose name contains
                        'test_method' or 'test_other', while -k 'not
                        test_method' matches those that don't contain
                        'test_method' in their names. Additionally keywords
                        are matched to classes and functions containing extra
                        names in their 'extra_keyword_matches' set, as well as
                        functions which have names assigned directly to them.

<snip>

to see available markers type: pytest --markers
to see available fixtures type: pytest --fixtures
(shown according to specified file_or_dir or current dir if not specified)

fixtures_required>pytest --fixtures 
usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: argument --please-work is required

fixtures_required>pytest --markers 
usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: argument --please-work is required

Versions

Windows 10.0.16299.125 x64

Package    Version               Location
---------- --------------------- -------------------------
appdirs    1.4.3
attrs      17.3.0
colorama   0.3.9
funcsigs   1.0.2
packaging  16.8
pip        9.0.1
pluggy     0.6.0
py         1.5.2
pyparsing  2.2.0
pytest     3.3.2.dev45+g370daf04
setuptools 38.2.4
six        1.11.0
tox        2.9.1
virtualenv 15.1.0
wheel      0.30.0
nicoddemus commented 6 years ago

Thanks @segevfiner for the report!

cryvate commented 6 years ago

Would a fix similar to #2458 work? Or is that likely to screw up the marker/fixture discovery?

RonnyPfannschmidt commented 6 years ago

we should probably deprecate/disallow required=True and/or handle initial cli parsing differently

segevfiner commented 6 years ago

@cryvate I haven't tried/checked

Deprecating this will still require an alternative that we should recommend instead (Code likely uses this, so it likely requires a proper deprecation period).

Implementing this by our self requires access to the dict of arguments that were parsed that is internal to argparse. (To the best of my memory of

2458). Just checking that the value is different from the default value is

not what argparse does, you can pass the default value as an argument and it still satifies required.

If you can't use required. You might, as a test developer, try to check for a non default value from pytest_configure, but that too breaks --help and friends, since they run pytest_configure before doing their thing. So figuring out the best hook for doing something like this isn't immediatly obvious either.

It isn't that uncommon to have tests that require arguments to run. Mostly for integration style tests that might require servers and credentials and such to work.

That's what I remember of this, at least.

בתאריך 19 בדצמ' 2017 18:08,‏ "Ronny Pfannschmidt" notifications@github.com כתב:

we should probably deprecate/disallow required=True and/or handle initial cli parsing differently

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pytest-dev/pytest/issues/3042#issuecomment-352804709, or mute the thread https://github.com/notifications/unsubscribe-auth/AXlg_5lxmTYjeihLuJLax2Y4XmeVoqv0ks5tB99wgaJpZM4REWBo .

blueyed commented 5 years ago

Yes, deprecating required=True is not good.

I've worked on making --version / --help work if there are any ArgumentErrors (e.g. a invalid choice) in https://github.com/pytest-dev/pytest/pull/4651.

The problem here is that pytest_cmdline_parse is used to build the config, and then pytest_cmdline_main handles them, but usage errors abort before this hook is called already.

In f3babf13 I reworked the code for argument validation from addopts already a bit, and thought about continuing in case of ArgumentErrors for #4651 (marking the config as having a usage error, and then quitting before the main test run, but allowing to e.g. handle --markers), but then stopped there for now.