-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
introduce pytest.Marked as holder for marked parameter values #1921
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I guess you want to deprecate pytest.mark.foo
on a parametrised value? This seems rather API heavy and I'm not sure I'd like to explain to people why we have two different ways of marking things. What's the main thing you're trying to address with this change?
@flub the current way is seriously broken - its impossible to pass a function for example the code that enables it is a painful hack i want to see gone pytest.mark.foo is something we apply to functions, not to values its a different type of behavior, and it breaks horrible |
Ah, I see what you mean. I must admit my first reaction on marking a parameterised value with skipped was hesitation as well but then practicality seemed to beat purity. But if we want to deprecate that why not experiment with other APIs? how about for example: |
well, you can have single and multiple marks per value, and not all values have marks |
Any other creative alternative? How about I realise I'm just throwing kind of random things at this but if changing the API anyway it's a good idea to brainstorm about how to create the best API with the requirements we know now. |
I don't really like the idea of having another list either - I think that'd be really useful, for example in this qutebrowser test where it's tricky to keep the @pytest.mark.parametrize('code', [
str,
lambda e: e[None],
lambda e: operator.setitem(e, None, None),
lambda e: operator.delitem(e, None),
lambda e: None in e,
list, # __iter__
len,
lambda e: e.has_frame(),
lambda e: e.geometry(),
lambda e: e.style_property('visibility', strategy='computed'),
lambda e: e.text(),
lambda e: e.set_text('foo'),
lambda e: e.insert_text('foo'),
lambda e: e.is_writable(),
lambda e: e.is_content_editable(),
lambda e: e.is_editable(),
lambda e: e.is_text_input(),
lambda e: e.remove_blank_target(),
lambda e: e.outer_xml(),
lambda e: e.tag_name(),
lambda e: e.rect_on_view(),
lambda e: e._is_visible(None),
], ids=['str', 'getitem', 'setitem', 'delitem', 'contains', 'iter', 'len',
'frame', 'geometry', 'style_property', 'text', 'set_text',
'insert_text', 'is_writable', 'is_content_editable', 'is_editable',
'is_text_input', 'remove_blank_target', 'outer_xml', 'tag_name',
'rect_on_view', 'is_visible'])
def test_vanished(self, elem, code):
"""Make sure methods check if the element is vanished."""
elem._elem.isNull.return_value = True
elem._elem.tagName.return_value = 'span'
with pytest.raises(webkitelem.IsNullError):
code(elem) |
its relatively easy to add a name to a marked value as well
|
we chould also extend the marked_value mechanism to a general annotated value then users can put test ids# names, marks and whatever they want in |
Another use-case I got reminded off via another issue is that you can have |
That sounds like a quite confusing API, which adds another overload for the word
Why? I'd guess the callable would just get the value (and not the |
so can we get back to making this something directly usable - namely - giving values a set of marks that part is solved, and the name should be discussed i rather want to introduce a simple straightforward api now than try to get a more complete concept right (which usually fails for a few iterations) |
I'm not at all convinced that the idea of a "marked value" is a very simple and straightforward concept. I would actually have no idea what it is in isolation let alone guess that it is a way of being able to apply a mark to one of the generated test items/functions. I think as the various parametrisation mechanisms have grown they have got weirder and weirder and are now really not as straight forward as might be desired for people new to it. |
basically a marked value is intended to bring markers and parametrization together perhaps it should be called marked_parameter instead of marked value, in any case, what we currently have is broken and cant be fixed without breaking things worse |
431bd2b
to
fece1fc
Compare
Sorry for joining so late in the discussion. I empathize with the fact that @RonnyPfannschmidt wants to solve the problem of "how to mark a value" quickly, but I also see the merits of the points raised by @flub and @The-Compiler that since we are introducing a new player into the mix (a "marked value"), we might as well discuss if we don't want to improve the overall API. We might arrive at something that not only fix how to mark values but also fixes some other problems/warts. I agree that the current parametrization API might be a little awkward specially when multiple parameters, Here an example from the docs, adding @pytest.mark.parametrize(("n", "expected"), [
(1, 2),
pytest.mark.xfail((1, 0)),
pytest.mark.xfail(reason="some bug")((1, 3)),
(2, 3),
(3, 4),
(4, 5),
pytest.mark.skipif("sys.version_info >= (3,0)")((10, 11)),
], ids=['1-2', '1-0', '1-3', '2-3', '3-4', '4-5', '10-11'])
def test_increment(n, expected):
assert n + 1 == expected Is something like the following what @flub and @The-Compiler are thinking about? @pytest.mark.parametrize(("n", "expected"), [
pytest.param(1, 2, id='1-2'),
pytest.param(1, 0, marks=pytest.mark.xfail, id='1-0'),
pytest.param(1, 3, marks=pytest.mark.xfail(reason="some bug"), id='1-3'),
pytest.param(2, 3, id='2-3'),
pytest.param(3, 4, id='3-4'),
pytest.param(4, 5, id='4-5'),
pytest.param(10, 11, id='10-11', marks=pytest.mark.skipif("sys.version_info >= (3,0)")),
])
def test_increment(n, expected):
assert n + 1 == expected Surely it is more verbose, but I'm sure it is more natural to read and understand what's going on IMHO, specially for pytest beginners. One thing that comes to mind is that the current fmt_date = lambda v: v.strftime('%Y/%M/%d')
@pytest.mark.parametrize('date', [
pytest.param(date(2016, 11, 10), id=fmt_date),
pytest.param(date(2016, 15, 10), id=fmt_date),
pytest.param(date(2016, 21, 10), id=fmt_date),
]) But that is a minor inconvenience IMHO. Another issue is that we probably don't want to let users mix styles... if one of the What are your thoughts? |
IMHO, combining @pytest.mark.parametrize('date', [
pytest.param(date(2016, 11, 10), marks=pytest.mark.xfail),
date(2016, 15, 10),
date(2016, 21, 10),
], ids=lambda v: v.strftime('%Y/%M/%d')) When Other than that, that sounds like a great proposal! |
this is a entirely different concept, but it can be done relatively quickly, i#ll try to finish before the weekend @nicoddemus did lay out things fabulously |
I thought about disallowing it at initially because:
@pytest.mark.parametrize('date', [
pytest.param(date(2016, 1, 1), id='new years'),
date(2016, 15, 10),
date(2016, 21, 10),
], ids=lambda v: v.strftime('%Y/%M/%d')) But after looking at the example itself I think it is pretty obvious that the param-specific Also, as I understand it the idea is to eventually deprecate the current way to mark parametrized values and eventually remove them in 4.0, so allowing users to mix the two styles is important to make it easy to adopt the new style because they only have to change the parameters that are marked to the new style, instead of all of them: @pytest.mark.parametrize('date', [
pytest.mark.xfail()(date(2016, 11, 10)),
date(2016, 15, 10),
date(2016, 21, 10),
]) To convert it when mixing the styles is allowed: @pytest.mark.parametrize('date', [
pytest.param(date(2016, 11, 10), marks=pytest.mark.xfail),
date(2016, 15, 10),
date(2016, 21, 10),
]) When it is not: @pytest.mark.parametrize('date', [
pytest.param(date(2016, 11, 10), marks=pytest.mark.xfail),
pytest.param(date(2016, 15, 10)),
pytest.param(date(2016, 21, 10)),
]) Depending on the values, a find/replace might be tricky as your example with lambdas suggest. So in summary, I agree with you that we should allow mixing |
Sounds good @RonnyPfannschmidt, but perhaps you should hold a bit to gather some more feedback? Particularly @flub which contributed a lot to the discussion already, but would love to hear from @hpk42 and @pfctdayelise as well. |
I guess the |
Hmm, I'm not so sure about that - if you use parametrization a lot (which I'd expect many testsuites to do), it's quite natural to want to xfail a single line there, no?
Are you talking about marking parameters here, or also about ids? For ids, I think we should still allow mixing |
well, we wouldn't remove the old way, just complain about it for a while |
@The-Compiler yes, mixing with |
a8570cb
to
5fe5acf
Compare
@nicoddemus @hpk42 i updated/rebased the pr and would appreciate a second pair of eyes - i'd like to speed up landing this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RonnyPfannschmidt really sorry about the delay in getting to this! I really appreciate your effort in cleaning up how marks stick to functions.
Overall I like how this is turning out @RonnyPfannschmidt, but I think besides my comments we also need a prominent CHANGELOG
entry and figure out how better add more documentation about this new and important feature.
_pytest/mark.py
Outdated
if isinstance(marks, MarkDecorator): | ||
marks = marks, | ||
else: | ||
assert isinstance(marks, (tuple, list, set)) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. 👍
_pytest/mark.py
Outdated
assert isinstance(marks, (tuple, list, set)) | ||
|
||
id = kw.pop('id', None) | ||
assert not kw, kw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, a better message would be "Unknown keyword arguments: {}".format(kw.keys())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while taking a look again, it may be sensible to trigger a pytest warning there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think it should break immediately. From my POV having **kwargs
here is just a way to emulate the keyword-only syntax of Python 3, so it should bail if the user passes a keyword parameter we don't know about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then i#ll just raise a Error by applying the kwargs to a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this is worthwhile doing?
@@ -94,21 +94,37 @@ 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, |
There was a problem hiding this comment.
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=)
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, FWIW.
]) | ||
def test_eval(test_input, expected): | ||
assert eval(test_input) == expected | ||
|
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
def alias(name): | ||
return property(attrgetter(name), doc='alias for ' + name) | ||
|
||
|
||
class ParameterSet(namedtuple('ParameterSet', 'values, marks, id')): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
_pytest/mark.py
Outdated
def alias(name): | ||
return property(attrgetter(name), doc='alias for ' + name) | ||
|
||
|
||
class ParameterSet(namedtuple('ParameterSet', 'values, marks, id')): | ||
@classmethod | ||
def param(cls, *value, **kw): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: should be *values
_pytest/mark.py
Outdated
return cls(value, marks, id) | ||
|
||
@classmethod | ||
def extract_from(cls, param, legacy_force_tuple=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite get the legacy_force_tuple
parameter here, even after looking at where it's used. Could add a quick-docstring explaining why it's needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do, it may need a better name, its for the special case of single name parametrizations
_pytest/mark.py
Outdated
return cls(value, marks, id) | ||
|
||
@classmethod | ||
def extract_from(cls, param, legacy_force_tuple=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: the param
parameter should be value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract_from operates on a legacy parameterset, so the name value is certainly wrong
_pytest/mark.py
Outdated
|
||
|
||
@property | ||
def deprecated_arg_dict(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a _
prefix
There was a problem hiding this comment.
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
reprec = testdir.inline_run() | ||
passed, failed = (0, 2) if strict else (2, 0) | ||
reprec.assertoutcome(passed=passed, failed=failed) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a test for the situation where the user uses passes id
to a param
and to parametrize
in the same call.
_pytest/mark.py
Outdated
assert isinstance(marks, (tuple, list, set)) | ||
|
||
id = kw.pop('id', None) | ||
assert not kw, kw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this is worthwhile doing?
e4d268c
to
9803119
Compare
Guys, anybody else wants to take a look at this PR before merging? Unless somebody says otherwise, I will merge this tomorrow. 👍 |
return {'mark': MarkGenerator()} | ||
return { | ||
'mark': MarkGenerator(), | ||
'param': ParameterSet.param, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ^^
_pytest/mark.py
Outdated
and may or may not be wrapped into a mess of mark objects | ||
|
||
:param legacy_force_tuple: | ||
enforce tuple wrapping so single arugment tuple values dont get decomposed and break tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks:
- arugment -> argument
- dont -> don't
- indent two spaces more for consistency
@@ -94,21 +94,37 @@ 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, FWIW.
doc/en/parametrize.rst
Outdated
|
||
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the backticks?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update your pytest! 😉
passed, failed = (0, 2) if strict else (2, 0) | ||
reprec.assertoutcome(passed=passed, failed=failed) | ||
|
||
|
||
def test_pytest_make_parametrize_id(self, testdir): | ||
testdir.makeconftest(""" | ||
def pytest_make_parametrize_id(config, val): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still have tests testing the legacy syntax/compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
passed, failed = (0, 2) if strict else (2, 0) | ||
reprec.assertoutcome(passed=passed, failed=failed) | ||
|
||
|
||
def test_pytest_make_parametrize_id(self, testdir): | ||
testdir.makeconftest(""" | ||
def pytest_make_parametrize_id(config, val): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still have tests testing the legacy syntax/compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
58d1529
to
a9a8697
Compare
i am thinking of folding a few commits, any opinion? |
Sure, go ahead! |
this allows a clear addition of parameterization parameters that carry along marks instead of nesting multiple mark objects and destroying the possibility of creating function valued parameters, it just folders everything together into one object carrfying parameters, and the marks.
a9a8697
to
e368fb4
Compare
parameters = [ | ||
ParameterSet.extract_from(x, legacy_force_tuple=force_tuple) | ||
for x in argvalues] | ||
del argvalues |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
LGTM now, but still needs a changelog entry. |
this pr introduces a actual real type to separate parameter values and marks
this enables having functions as parameters with marks
it currently mirrors the broken mark overriding of the nested mark extraction code in place
documentation and examples upcoming