Add pytest.mark.skip shortcut (Issue #607)

[MichaelAquilina]

Add a non-conditional skip marker for convenience as requested in Issue #607.

I will also probably be adding another feature which automatically treats @pytest.skip as pytest.mark.skip if it detects itself being used as a decorator. I was thinking that this should probably be opened as a separate pull request though unless you don't mind it being included in here?

Ran python runtox.py -e py27,py34,flakes to test changes.

This work is a result of the pyconuk pytest sprint! :)

[MichaelAquilina]

I'll fix the tests as soon as I can.

I assume I'll need to update the docs for this?

[RonnyPfannschmidt]

for pytest.skip/xfail being used in such a way,
we should first intoduce a global concept of are we currently in a test call

the docs certainly need updates

the failures are related to backward compat around twisted trial, i suggest you install twisted in your venv while fixing up the interaction

good initial work :)

[flub]

For part 2, if pytest.skip is used as a decorator it should not behave as pytest.mark.skip, instead it should result in a collection error. Maybe simply raising an exception is sufficient for this.

[RonnyPfannschmidt]

instead of teaching skip to return a mark, we could have collection catch skip errors and give a warning

[MichaelAquilina]

Maybe I should add a comment explaining why this is needed? Or do you feel its unnecessary?

[nicoddemus]

Please do, seems "magical" 😉

[nicoddemus]

Thanks for the work! I noticed two problems while playing around with it however:

• reason is not handled when missing, so the user sees this when using a plain @pytest.mark.skip:

  SKIP [1] test_xx.py:2: <Skipped instance>
  

• the user could still use pytest.mark.skip('False') and the test would not skip.

This fixes it, but it is not pretty 😅:

@pytest.hookimpl(tryfirst=True)
def pytest_runtest_setup(item):
    evalskip = MarkEvaluator(item, 'skip')
    if evalskip.holder is not None:
        evalskip.reason = evalskip.holder.kwargs.get('reason', 'explicit skip')
        evalskip.result = True
        item._evalskip = evalskip
        pytest.skip(evalskip.getexplanation())

    evalskip = MarkEvaluator(item, 'skipif')
    if evalskip.istrue():
        item._evalskip = evalskip
        pytest.skip(evalskip.getexplanation())
    ...

The implementations lets the user don't pass any reason in which case it will use explicit skip... perhaps we want to enforce it?

[MichaelAquilina]

Ah yes, sorry for not catching this. Will give it a closer look and see if there's a nice way to fix this. I'll also make sure my tests are updated to prevent this from happening again.

[MichaelAquilina]

@RonnyPfannschmidt could you elaborate a bit more? Not sure that I understand your comment

[MichaelAquilina]

@fub I guess that provides a more consistent front in terms of the way pytest works. But #607 seems to conclude that @pytest.skip should act like @pytest.mark.skip if used as a decorator? I dont mind either way.

[RonnyPfannschmidt]

@MichaelAquilina when collection catches a pytest.skip.Exception, we should do a config.warn(...)

[MichaelAquilina]

@RonnyPfannschmidt should I open a separate pull request for that seeing as its slightly unrelated?

[RonnyPfannschmidt]

@MichaelAquilina yes please

[RonnyPfannschmidt]

you might want to use the versionadded rst directive there

[nicoddemus]

That would be:

 Marking a test function to be skipped
 -------------------------------------------

... versionadded:: 2.9

The simplest way to skip ...

[nicoddemus]

Btw good job on the docs! 👍

[hpk42]

FWIW i don't mind pytest.skip detecting if it's used as a decorator and erroring out -- if this can be done safely and doesn't complicate the implementation (pytest.mark is already a bit messy if you ask me). It should not silently behave like a marker, however.

As to if it can be safely done: usually pytest.skip takes a message -- how can you in its function body then safely know if it's used as a decorator?

[nicoddemus]

Just a reminder, this should go into the features (2.9) branch.

[nicoddemus]

@flub

For part 2, if pytest.skip is used as a decorator it should not behave as pytest.mark.skip, instead it should result in a collection error. Maybe simply raising an exception is sufficient for this.

👍

@RonnyPfannschmidt

instead of teaching skip to return a mark, we could have collection catch skip errors and give a warning

I'm not a big fan of this... warnings by default don't explicitly state what happened (you must pass -rw to see the message). Imagine a new user evaluating/playing around with pytest, writing a single test using @pytest.skip instead of @pytest.mark.skip: he would see that no tests were ran, and a misterious 1 pytest-warning, which is not obvious to find. I think an explicit error would convey better the misuse.

(I just realized that the output of pytest --help for -r chars still shows: (w) warnings instead of (w)pytest-warnings... 😳 will fix that right away).

@hpk42

It should not silently behave like a marker, however.

👍

[nicoddemus]

Also check the reason here please

[MichaelAquilina]

How would I check the reason here if it's not specified though? Is matching the string "skipped instance" good enough?

[nicoddemus]

I assume the reason in this case was "False", right? I mean, the only possible parameter for skip is a reason, differently from skipif which receives (condition, reason). So I meant that you should also use fnmatch_linesto ensure the expected message is being displayed.

[MichaelAquilina]

Thanks for all the feedback guys! Will probably get a chance to work on this sometime this evening or tomorrow. I'll look through all your comments and update as necessary :)

[MichaelAquilina]

@nicoddemus should this be marked for 2.8.1 or 2.9 in the docs seeing as this pull request is marked for the 2.8.1 milestone?

[nicoddemus]

As it is a new feature, 2.9 please. New features, even minor ones, should be released a new major version, otherwise people have to depend on pytest>=2.8.1 which is a little weird. I will update the milestone. 😄