Skip to content

xfail doesn't actually run the test (but the docs promise that it does) #810

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

Closed
inducer opened this issue Jul 2, 2015 · 12 comments
Closed
Labels
status: needs information reporter needs to provide more information; can be closed after 2 or more weeks of inactivity type: enhancement new feature or API change, should be merged into features branch

Comments

@inducer
Copy link

inducer commented Jul 2, 2015

From http://pytest.org/latest/skipping.html#mark-a-test-function-as-expected-to-fail:

This test will be run but no traceback will be reported when it fails. Instead terminal reporting will list it in the “expected to fail” or “unexpectedly passing” sections.

Consider this snippet:

def test_xfail():
    import pytest
    pytest.xfail("doesn't work")

    import os
    os._exit(0)

This produces an "x" in the pytest output. If the test were actually run beyond the xfail as documented, then the interpreter should have just exited. What xfail seems to do is terminate the execution of the test. But that's inconvenient. Suppose you have a test that'll always raise an exception. You'd have to wrap the test in a try block and call xfail in a finally clause. Instead, pytest should mark the test as expecting to fail and continue on.

@nicoddemus
Copy link
Member

The documentation you mention actually refers to the xfail mark, not the explicit pytest.xfail usage.

Consider this example:

# contents of test_foo.py
import pytest

def test_xfail():
    print
    print '  xfail-before'
    pytest.xfail('xfail')
    print '  xfail-after'

@pytest.mark.xfail
def test_xfail_mark():
    print
    print '  mark-before'
    print '  mark-after'     
    assert 0
$ pytest test_foo.py -s -v
============================= test session starts =============================
platform win32 -- Python 2.7.7 -- py-1.4.26 -- pytest-2.7.0 -- D:\Shared\dist\12.0-win64\python-2.7.X\python.exe
qt-api: pyqt4
rootdir: X:\wellres, inifile:
plugins: timeout, localserver, xdist, cov, mock, qt
collected 2 items

test_foo.py::test_xfail
  xfail-before
xfail
test_foo.py::test_xfail_mark
  mark-before
  mark-after
xfail

As you can see, the test with the mark runs completely, but the one with the pytest.xfail runs only up to the pytest.xfail call. What happens is that pytest actually uses an exception to capture that a test called pytest.xfail:

def xfail(reason=""):
    """ xfail an executing test or setup functions with the given reason."""
    __tracebackhide__ = True
    raise XFailed(reason)

While this could be changed to just let the test continue execution and report it as xfail, I don't think that's an option as it would unfortunately change existing behavior and might break existing test suites.

Regardless if the behavior should change or not, I think that the documentation should explicitly mention this difference. A PR for the docs would be very welcome! 😄

@inducer
Copy link
Author

inducer commented Jul 2, 2015

I think the behavior should be changed. Many reasons:

  • Given your documentation, I don't think anyone expected the two xfails to be inconsistent with each other. I certainly didn't.
  • The behavior change is really quite low-risk. The worst that could happen is that a few test suites now report unexpected passes, which I imagine folks might like to know about anyway. In fact, the current behavior masks these unexpected passes. To me, the whole point of xfail is "I know this is broken right now, but let me know once it starts working." The current behavior defeats this purpose.
  • As it stands, imperative xfail and skip really do the same thing--so what's the point of having the imperative xfail at all? If you're going to stick with that interpretation, imperative xfail should IMO be deprecated to avoid API redundancy.

@nicoddemus
Copy link
Member

You raise some valid points!

Given your documentation, I don't think anyone expected the two xfails to be inconsistent with each other. I certainly didn't.

I agree that the documentation could be improved (that's why I mention a PR would be welcomed to fix that), and I agree that it would be better for the two xfail forms to be consistent with each other.

The behavior change is really quite low-risk. The worst that could happen is that a few test suites now report unexpected passes, which I imagine folks might like to know about anyway.

I agree that it is low-risk, and in my experience pytest.mark.xfail is much more used than pytest.xfail, but keep in mind that a test suite containing a test just like your example (with a os._exit() call) or in which a pytest.xfail() call is preventing crashing code from being reached will now start to fail. pytest tries very hard to be backward compatible in order to avoid new releases from breaking existing (working) test suites.

As it stands, imperative xfail and skip really do the same thing.

They do the same thing, but the semantics are different: skip means a test should not be executed in some expected circumstances (a Windows-only test being skipped on Linux for example). xfail on the other hand means that a test is failing always/sometimes but it shouldn't, like a test exercising a reported bug or a flaky test. So, after the test suite finishes, skips are OK, but xfails should to be worked-on eventually or signal red-flags that should be checked out eventually.

All in all, I don't lean too strongly on either side of the issue and of course I welcome the discussion. 😄

Perhaps others can chip in? Any thoughts on this @hpk42, @flub, @bubenkoff, @The-Compiler...?

(Btw, please keep in mind that I only closed the issue because usually people forget to close their own issues, at no point I wanted to put an end to the discussion, hope you understand.)

@The-Compiler
Copy link
Member

I'm not sure really - I agree they should be consistent, but I also see the danger of breaking existing code.

So I searched for code using pytest.xfail, and found this for example:

def test_record_to_file(camera, previewing, mode, filenames_format_options):
    ...
    if resolution[1] > 480 and format == 'mjpeg':
        pytest.xfail('Locks up camera')
    camera.start_recording(filename1, **options)

So it seems people really rely on that behavior, and it was easy to find an example to prove it - so I agree it should be clarified in the documentation, but not changed.

@inducer
Copy link
Author

inducer commented Jul 3, 2015

If it is indeed not changed, I would like to request an xfail-like imperative thing that works like the marker.

@The-Compiler
Copy link
Member

Maybe adding a skip parameter which is True by default to pytest.xfail? That means you could do pytest.xfail(skip=False) if you want that behavior.

If others agree with the idea, would you like to submit a PR? I'm sure it'd be much appreciated, and we're happy to help if you get stuck.

@inducer
Copy link
Author

inducer commented Jul 3, 2015

I took a look at the code, and I must admit that I wasn't able to make much sense of it. (In particular how the marking happens and where an xfail mark takes effect.)

@nicoddemus
Copy link
Member

Maybe adding a skip parameter which is True by default to pytest.xfail?

I like this suggestion, but I'm having trouble figuring out how one would implement it...

Currently when pytest.xfail is called, it raises an exception which is caught by pytest and it marks the test as xfailed. All good.

But how to implement this in a way to "mark" the test it xfailed, but still continue execution after the call? Mind that pytest.xfail can be called anytime including from other fixtures or functions, and the fact that pytest.xfail is a global function severely limits our options on how to obtain the "test request" at the point of the call.

@inducer
Copy link
Author

inducer commented Jul 3, 2015

A thread-local variable maybe that tracks what test request is active? (This would still permit recursion with some care, but no other form of concurrency.)

@The-Compiler
Copy link
Member

I have to admit I didn't think about the implementation yet 😆.

What @inducer proposed or ugly hacks using sys._getframe are about the only things coming to mind...

@nicoddemus nicoddemus reopened this Jul 4, 2015
@nicoddemus nicoddemus added the type: enhancement new feature or API change, should be merged into features branch label Jul 9, 2015
@RonnyPfannschmidt
Copy link
Member

marks make, the functions raise exceptions, i think that is consistent enough
got experimenting with something more dynamic about outcome control i suggest to write a plugin and using a fixture

@RonnyPfannschmidt
Copy link
Member

this issue is has died down after getting a bit out of hand, we should open to the point issues for the documentation fix and close off the rest after a wait period

@RonnyPfannschmidt RonnyPfannschmidt added the status: needs information reporter needs to provide more information; can be closed after 2 or more weeks of inactivity label May 14, 2017
nicoddemus added a commit to nicoddemus/pytest that referenced this issue May 31, 2017
Also did a general review of the document to improve the flow

Fix pytest-dev#810
nicoddemus added a commit to nicoddemus/pytest that referenced this issue May 31, 2017
Also did a general review of the document to improve the flow

Fix pytest-dev#810
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs information reporter needs to provide more information; can be closed after 2 or more weeks of inactivity type: enhancement new feature or API change, should be merged into features branch
Projects
None yet
Development

No branches or pull requests

4 participants