Raise exception if parametrize collects an empty parameter set#3970
Raise exception if parametrize collects an empty parameter set#3970nicoddemus merged 3 commits intopytest-dev:featuresfrom
Conversation
|
Not exactly sure of what the standard workflow is here. Should I squash the commits? If so, when? Or is that done by the one who decides to merge? |
|
if you like please squash until the sequence is to your liking, personally i prefer to merge full expressive history lines after doing a bit manual cleanup |
sambarluc
force-pushed
the
raise_on_empty_parameterset
branch
from
d7a6a5a to
f13938e
Compare
|
Ok, done. Unless other issues pop up, it's in your hands! |
Codecov Report
@@ Coverage Diff @@
## features #3970 +/- ##
============================================
- Coverage 94.48% 94.26% -0.22%
============================================
Files 107 107
Lines 23651 23661 +10
Branches 2349 2347 -2
============================================
- Hits 22347 22305 -42
- Misses 993 1048 +55
+ Partials 311 308 -3
Continue to review full report at Codecov.
|
|
great job, thanks for the patience and for nicely implementing the proposal |
nicoddemus
left a comment
nicoddemus
left a comment
There was a problem hiding this comment.
Thanks @sambarluc for the great implementation.
I left some comments which I think we should discuss a bit. 👍
doc/en/reference.rst
Outdated
|
|
||
| * ``skip`` skips tests with an empty parameterset (default) | ||
| * ``xfail`` marks tests with an empty parameterset as xfail(run=False) | ||
| * ``fail_at_parametrize`` raises an exception if parametrize collects an empty parameter set |
There was a problem hiding this comment.
I'm wondering, should we change the default for 4.0 to this option?
There was a problem hiding this comment.
If you ask me, I would definitely pick fail_at_parametrize as the default, that is by far the most obvious behaviour we expect in all our tests. However, I don't have a broader overview of how pytest is used.
There was a problem hiding this comment.
Yes, I agree, it should error out by default.
Hmmm one way to implement this transition with minimal disturbance to our users: if we find an empty parameter, we issue a warning saying that the default will change in a few versions, and users that like the current default can set it into their pytest.ini files. What do you think @RonnyPfannschmidt?
But sorry, just realized this is not related to this PR and I'm hijacking this thread. 😁
There was a problem hiding this comment.
i created a follow-up issue
|
Not sure of how badly github deals with changes in history, so I leave the |
|
Thanks @sambarluc for the contribution, much appreciated! 👍 |
|
Feel free to rebase/squash your commits now. 👍 |
|
Just an idea, before I squash. What about having the option as
'fail_at_collect', instead of 'fail_at_parametrize'? I was thinking that it
may offer less opportunities of confusion.
Le ven. 14 sept. 2018 à 00:17, Bruno Oliveira <notifications@github.com> a
écrit :
… Feel free to rebase/squash your commits now. 👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3970 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF0xphdDxQZuCjw-cX_-luxkPLU0p147ks5uatmKgaJpZM4WlTkn>
.
|
|
nice idea - in that case i would like to propose the name |
|
My 5 cents: I would not go for |
|
hmm, that brings me to the point that that particular option wasn't intended to manage it to begin with - lets go with the value you proposed and open a follow-up ticket to think it trough a bit later |
|
I'll wait for @nicoddemus before squashing, I guess it's not particularly urgent. |
sambarluc
force-pushed
the
raise_on_empty_parameterset
branch
from
a2bed03 to
b84e5c8
Compare
|
Hi folks, I think
You are right, this will go into |
Optionally raise an exception when parametrize collects no arguments. Provide the name of the test causing the failure in the exception message. See: pytest-dev#3849
sambarluc
force-pushed
the
raise_on_empty_parameterset
branch
from
b84e5c8 to
913c07e
Compare
|
Not sure what's wrong with appveyor. How can I retrigger? |
|
its a codecov failure |
|
Doesn't seem to be related to the code itself. |
|
exactly, as far as im concerned, the tests passed, the codecov failed, and that shouldnt reflect in the build status @nicoddemus feel free to merge |
|
Thank you all for the great tool you provide with pytest! |
5 participants
Currently,
parametrizeskips or xfails a test if no arguments are collected. In both cases, the test is considered successful in most (all?) testing systems. This can potentially hide bugs in the argument collection of parametrize, and thus lead to tests not being performed by mistake. The PR adds an ini option that causes a specific exception to be raised when parametrize collects no arguments.Implements feature request #3849