-
-
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
empty parameterset - enable opt to xfail #3044
empty parameterset - enable opt to xfail #3044
Conversation
_pytest/mark.py
Outdated
from pytest import UsageError | ||
raise UsageError( | ||
"empty_parameterset must be one of skip and xfail," | ||
" insteat it is {!r}".format(empty_parameterset)) |
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.
insteat -> instead (but "but" would be a better match probably)
0d88125
to
156abd0
Compare
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.
Looks like a great start, thanks!
Besides the few comments I made, I think we still need to add the documentation for this feature in customize.rst
.
fs, lineno = getfslineno(function) | ||
reason = "got empty parameter set %r, function %s at %s:%d" % ( | ||
argnames, function.__name__, fs, lineno) | ||
return mark(reason=reason) |
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.
For requested_mark == 'xfail'
this will be equivalent to MARK_GEN.xfail(run=False)(reason=reason)
... is this right?
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.
correct
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.
Weird, so pytest.mark.xfail(run=False)(reason=reason)
produces the correct result?
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.
@nicoddemus pytest marks support compounding addition of args/kwargs
mark.foo(1)(2)(2)(b=2) == mark.foo(1,2,3,b=3)
testing/test_mark.py
Outdated
if mark is None: | ||
# normalize to the requested name | ||
mark = 'skip' | ||
assert result_mark.name == mark |
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 also check the reason
attribute
04a2778
to
8979b2a
Compare
@nicoddemus changes are implemented, will create a followup issue wrt the deprecation/option change timeline |
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.
Thanks @RonnyPfannschmidt it will be great to get this into 3.4
. Please take a look at my comments.
_pytest/mark.py
Outdated
@@ -97,6 +94,20 @@ def _for_parameterize(cls, argnames, argvalues, function): | |||
return argnames, parameters | |||
|
|||
|
|||
def get_empty_parameterset_mark(config, argnames, function): | |||
requested_mark = config.getini('empty_parameterset') |
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.
Now that I take a look another look at it, I think this option should be named empty_parameter_set_mark
instead, what do you think?
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.
yup, a moment please for the fix
changelog/2527.feature
Outdated
@@ -0,0 +1 @@ | |||
introduce a pytest ini option to pick the mark for empty parametersets and allow to use xfail(run=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 think this can be improved to be more user-oriented:
Introduce ``empty_parameter_set_mark`` ini option to select which mark to apply when ``@pytest.mark.parametrize`` is given an empty set of parameters. Valid options are ``skip`` (default) and ``xfail``. Note that it is planned to change the default to ``xfail`` in future releases as this is considered less error prone.
doc/en/customize.rst
Outdated
|
||
.. versionadded:: 3.4 | ||
|
||
allows to pick the action for empty parametersets in parameterization |
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.
This should start with upper case: Allows ...
doc/en/customize.rst
Outdated
* ``skip`` skips tests with a empty parameterset | ||
* ``xfail`` marks tests with a empty parameterset as xfail(run=False) | ||
|
||
The default is ``skip``, it will be shifted to xfail in future. |
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 mentioning that skip is default in its own description is slightly better:
* ``skip`` skips tests with a empty parameter set (default)
And use this paragraph to mention that we plan to change the default in a future release with a link to #3155
testing/test_mark.py
Outdated
@@ -891,3 +891,26 @@ class TestMarkDecorator(object): | |||
]) | |||
def test__eq__(self, lhs, rhs, expected): | |||
assert (lhs == rhs) == expected | |||
|
|||
|
|||
@pytest.mark.parametrize('mark', [None, 'skip', 'xfail']) |
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 check for the empty string in the code, should you add an empty string here too I suppose?
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.
Thanks, LGTM!
now it should actially pass, there was a small oversight on my part, i think i'd like to create a constant |
@RonnyPfannschmidt took the liberty of making a slight tweak to the docs in 169635e, hope you don't mind. As soon as CI passes we should merge this. 👍 |
closes #2527