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

introduce pytest.Marked as holder for marked parameter values #1921

Merged
merged 2 commits into from
Mar 20, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 62 additions & 15 deletions _pytest/mark.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,72 @@
from operator import attrgetter
from .compat import imap


def alias(name):
return property(attrgetter(name), doc='alias for ' + name)


class ParameterSet(namedtuple('ParameterSet', 'values, marks, id')):
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 should add unittests for ParameterSet to make sure we cover all corner cases and possible validation errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

i added a few, perhaps we should make an issue about more detailed tests

@classmethod
def param(cls, *values, **kw):
marks = kw.pop('marks', ())
if isinstance(marks, MarkDecorator):
marks = marks,
else:
assert isinstance(marks, (tuple, list, set))
Copy link
Member

Choose a reason for hiding this comment

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

Since this assert is triggered directly by user input, should we raise an exception which doesn't display the internal pytest traceback? Maybe UsageError?

Copy link
Member Author

Choose a reason for hiding this comment

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

we have no sane mechanism for that point in time, as such i'd just leave the exception until we do get a way to sensibly report such errors

Copy link
Member

Choose a reason for hiding this comment

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

UsageError is not a good fit?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, its an error that does warrant to just stop collection alltogether

Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm mistaken it seems that's what UsageError does, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

whops, i forgot a "not" there

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I disagree... if I type mark= instead of marks=, why would I not want a glaring and loud error at that? Do you have an example of where just emitting a warning makes sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

i want a collelction error, not a direct stop of the process

Copy link
Member

Choose a reason for hiding this comment

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

Oh OK, I agree.

Now I see, I got confused by your comment on the line below:

while taking a look again, it may be sensible to trigger a pytest warning there

and thought you meant a warning here as well. My bad.

Having collection error at this point is definitely the proper approach. 👍


def param_extract_id(id=None):
return id

id = param_extract_id(**kw)
return cls(values, marks, id)

@classmethod
def extract_from(cls, parameterset, legacy_force_tuple=False):
"""
:param parameterset:
a legacy style parameterset that may or may not be a tuple,
and may or may not be wrapped into a mess of mark objects

:param legacy_force_tuple:
enforce tuple wrapping so single argument tuple values
don't get decomposed and break tests

"""

if isinstance(parameterset, cls):
return parameterset
if not isinstance(parameterset, MarkDecorator) and legacy_force_tuple:
return cls.param(parameterset)

newmarks = []
argval = parameterset
while isinstance(argval, MarkDecorator):
newmarks.append(MarkDecorator(Mark(
argval.markname, argval.args[:-1], argval.kwargs)))
argval = argval.args[-1]
assert not isinstance(argval, ParameterSet)
if legacy_force_tuple:
argval = argval,

return cls(argval, marks=newmarks, id=None)

@property
def deprecated_arg_dict(self):
Copy link
Member

Choose a reason for hiding this comment

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

Please add a _ prefix

Copy link
Member Author

Choose a reason for hiding this comment

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

wont do that as its used in open code and just bad data

return dict((mark.name, mark) for mark in self.marks)


class MarkerError(Exception):

"""Error in use of a pytest marker/attribute."""



def pytest_namespace():
return {'mark': MarkGenerator()}
return {
'mark': MarkGenerator(),
'param': ParameterSet.param,
Copy link
Member

Choose a reason for hiding this comment

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

You might want to do this differently as we're getting rid of pytest_namespace in your other PR - or just deal with the conflicts and fix it up there, I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

well, i deal the conflicts i fold the conflicts ^^

}


def pytest_addoption(parser):
Expand Down Expand Up @@ -212,6 +267,7 @@ def istestfunc(func):
return hasattr(func, "__call__") and \
getattr(func, "__name__", "<lambda>") != "<lambda>"


class MarkDecorator(object):
""" A decorator for test functions and test classes. When applied
it will create :class:`MarkInfo` objects which may be
Expand Down Expand Up @@ -257,8 +313,11 @@ def __init__(self, mark):
def markname(self):
return self.name # for backward-compat (2.4.1 had this attr)

def __eq__(self, other):
return self.mark == other.mark

def __repr__(self):
return "<MarkDecorator %r>" % self.mark
return "<MarkDecorator %r>" % (self.mark,)

def __call__(self, *args, **kwargs):
""" if passed a single callable argument: decorate it with mark info.
Expand Down Expand Up @@ -291,19 +350,7 @@ def __call__(self, *args, **kwargs):
return self.__class__(self.mark.combined_with(mark))


def extract_argvalue(maybe_marked_args):
# TODO: incorrect mark data, the old code wanst able to collect lists
# individual parametrized argument sets can be wrapped in a series
# of markers in which case we unwrap the values and apply the mark
# at Function init
newmarks = {}
argval = maybe_marked_args
while isinstance(argval, MarkDecorator):
newmark = MarkDecorator(Mark(
argval.markname, argval.args[:-1], argval.kwargs))
newmarks[newmark.name] = newmark
argval = argval.args[-1]
return argval, newmarks



class Mark(namedtuple('Mark', 'name, args, kwargs')):
Expand Down
71 changes: 36 additions & 35 deletions _pytest/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -788,36 +788,35 @@ def parametrize(self, argnames, argvalues, indirect=False, ids=None,
to set a dynamic scope using test context or configuration.
"""
from _pytest.fixtures import scope2index
from _pytest.mark import extract_argvalue
from _pytest.mark import ParameterSet
from py.io import saferepr

unwrapped_argvalues = []
newkeywords = []
for maybe_marked_args in argvalues:
argval, newmarks = extract_argvalue(maybe_marked_args)
unwrapped_argvalues.append(argval)
newkeywords.append(newmarks)
argvalues = unwrapped_argvalues

if not isinstance(argnames, (tuple, list)):
argnames = [x.strip() for x in argnames.split(",") if x.strip()]
if len(argnames) == 1:
argvalues = [(val,) for val in argvalues]
if not argvalues:
argvalues = [(NOTSET,) * len(argnames)]
# we passed a empty list to parameterize, skip that test
#
force_tuple = len(argnames) == 1
else:
force_tuple = False
parameters = [
ParameterSet.extract_from(x, legacy_force_tuple=force_tuple)
for x in argvalues]
del argvalues
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

i used this as refactoring aid, the argnames/argvalues variables where badly named and used
just killing it off the local namespace helped finding errors, and it also prevents using them after they shouldnt be used



if not parameters:
fs, lineno = getfslineno(self.function)
newmark = pytest.mark.skip(
reason="got empty parameter set %r, function %s at %s:%d" % (
argnames, self.function.__name__, fs, lineno))
newkeywords = [{newmark.markname: newmark}]
reason = "got empty parameter set %r, function %s at %s:%d" % (
argnames, self.function.__name__, fs, lineno)
mark = pytest.mark.skip(reason=reason)
parameters.append(ParameterSet(
values=(NOTSET,) * len(argnames),
marks=[mark],
id=None,
))

if scope is None:
scope = _find_parametrized_scope(argnames, self._arg2fixturedefs, indirect)

scopenum = scope2index(
scope, descr='call to {0}'.format(self.parametrize))
scopenum = scope2index(scope, descr='call to {0}'.format(self.parametrize))
valtypes = {}
for arg in argnames:
if arg not in self.fixturenames:
Expand Down Expand Up @@ -845,22 +844,22 @@ def parametrize(self, argnames, argvalues, indirect=False, ids=None,
idfn = ids
ids = None
if ids:
if len(ids) != len(argvalues):
raise ValueError('%d tests specified with %d ids' %(
len(argvalues), len(ids)))
if len(ids) != len(parameters):
raise ValueError('%d tests specified with %d ids' % (
len(parameters), len(ids)))
for id_value in ids:
if id_value is not None and not isinstance(id_value, py.builtin._basestring):
msg = 'ids must be list of strings, found: %s (type: %s)'
raise ValueError(msg % (saferepr(id_value), type(id_value).__name__))
ids = idmaker(argnames, argvalues, idfn, ids, self.config)
ids = idmaker(argnames, parameters, idfn, ids, self.config)
newcalls = []
for callspec in self._calls or [CallSpec2(self)]:
elements = zip(ids, argvalues, newkeywords, count())
for a_id, valset, keywords, param_index in elements:
assert len(valset) == len(argnames)
elements = zip(ids, parameters, count())
for a_id, param, param_index in elements:
assert len(param.values) == len(argnames)
newcallspec = callspec.copy(self)
newcallspec.setmulti(valtypes, argnames, valset, a_id,
keywords, scopenum, param_index)
newcallspec.setmulti(valtypes, argnames, param.values, a_id,
param.deprecated_arg_dict, scopenum, param_index)
newcalls.append(newcallspec)
self._calls = newcalls

Expand Down Expand Up @@ -959,17 +958,19 @@ def _idval(val, argname, idx, idfn, config=None):
return val.__name__
return str(argname)+str(idx)

def _idvalset(idx, valset, argnames, idfn, ids, config=None):
def _idvalset(idx, parameterset, argnames, idfn, ids, config=None):
if parameterset.id is not None:
return parameterset.id
if ids is None or (idx >= len(ids) or ids[idx] is None):
this_id = [_idval(val, argname, idx, idfn, config)
for val, argname in zip(valset, argnames)]
for val, argname in zip(parameterset.values, argnames)]
return "-".join(this_id)
else:
return _escape_strings(ids[idx])

def idmaker(argnames, argvalues, idfn=None, ids=None, config=None):
ids = [_idvalset(valindex, valset, argnames, idfn, ids, config)
for valindex, valset in enumerate(argvalues)]
def idmaker(argnames, parametersets, idfn=None, ids=None, config=None):
ids = [_idvalset(valindex, parameterset, argnames, idfn, ids, config)
for valindex, parameterset in enumerate(parametersets)]
if len(set(ids)) != len(ids):
# The ids are not unique
duplicates = [testid for testid in ids if ids.count(testid) > 1]
Expand Down
51 changes: 36 additions & 15 deletions doc/en/parametrize.rst
Original file line number Diff line number Diff line change
Expand Up @@ -55,27 +55,27 @@ them in turn::

$ pytest
======= test session starts ========
platform linux -- Python 3.5.2, pytest-3.0.7, py-1.4.32, pluggy-0.4.0
platform linux -- Python 3.5.2, pytest-3.0.3, py-1.4.31, pluggy-0.4.0
rootdir: $REGENDOC_TMPDIR, inifile:
collected 3 items

test_expectation.py ..F

======= FAILURES ========
_______ test_eval[6*9-42] ________

test_input = '6*9', expected = 42

@pytest.mark.parametrize("test_input,expected", [
("3+5", 8),
("2+4", 6),
("6*9", 42),
])
def test_eval(test_input, expected):
> assert eval(test_input) == expected
E AssertionError: assert 54 == 42
E assert 54 == 42
E + where 54 = eval('6*9')

test_expectation.py:8: AssertionError
======= 1 failed, 2 passed in 0.12 seconds ========

Expand All @@ -94,21 +94,42 @@ for example with the builtin ``mark.xfail``::
@pytest.mark.parametrize("test_input,expected", [
("3+5", 8),
("2+4", 6),
pytest.mark.xfail(("6*9", 42)),
pytest.param("6*9", 42,
Copy link
Member

Choose a reason for hiding this comment

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

We should add more examples of pytest.param, including usages of the id parameter in isolation and used together with parametrize(..., id=).

Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to tackle this @RonnyPfannschmidt?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, FWIW.

marks=pytest.mark.xfail),
])
def test_eval(test_input, expected):
assert eval(test_input) == expected

.. note::

prior to version 3.1 the supported mechanism for marking values
used the syntax::

import pytest
@pytest.mark.parametrize("test_input,expected", [
("3+5", 8),
("2+4", 6),
pytest.mark.xfail(("6*9", 42),),
])
def test_eval(test_input, expected):
assert eval(test_input) == expected

Copy link
Member

Choose a reason for hiding this comment

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

Please complement this stating why this mechanism was replaced in favor of pytest.param (not being able to mark functions, strange syntax to newcomers). Also please state that this mechanism will start to emit warnings in 3.1 and will be dropped in 4.0.

Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to tackle this @RonnyPfannschmidt?

Copy link
Member

Choose a reason for hiding this comment

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

Just noticed you did already, nevermind my previous comment


This was an initial hack to support the feature but soon was demonstrated to be incomplete,
broken for passing functions or applying multiple marks with the same name but different parameters.
The old syntax will be removed in pytest-4.0.


Let's run this::

$ pytest
======= test session starts ========
platform linux -- Python 3.5.2, pytest-3.0.7, py-1.4.32, pluggy-0.4.0
platform linux -- Python 3.5.2, pytest-3.0.3, py-1.4.31, pluggy-0.4.0
Copy link
Member

Choose a reason for hiding this comment

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

Update your pytest! 😉

rootdir: $REGENDOC_TMPDIR, inifile:
collected 3 items

test_expectation.py ..x

======= 2 passed, 1 xfailed in 0.12 seconds ========

The one parameter set which caused a failure previously now
Expand Down Expand Up @@ -181,15 +202,15 @@ Let's also run with a stringinput that will lead to a failing test::
F
======= FAILURES ========
_______ test_valid_string[!] ________

stringinput = '!'

def test_valid_string(stringinput):
> assert stringinput.isalpha()
E AssertionError: assert False
E assert False
E + where False = <built-in method isalpha of str object at 0xdeadbeef>()
E + where <built-in method isalpha of str object at 0xdeadbeef> = '!'.isalpha

test_strings.py:3: AssertionError
1 failed in 0.12 seconds

Expand Down
Loading