Mark for flaky tests

[The-Compiler]

I'd like to continue the discussion from #776 here. What I said there:

I agree there should be some difference between "is expected to fail" and "is flaky".

I personally use xfail for "I found a bug while writing tests, or found a bug and wrote a test for it - but I don't want to fix it immediately, and I don't want my CI failing because of it". (I don't have any flaky tests, fortunately)

IMHO, xpassed should be red, and a new pytest.mark.flaky marker should be added - then it's also immediately clear for someone reading the tests that this test is flaky, not expected to fail.

cc @esiegerman

[nicoddemus]

I agree with everything you wrote, but I fear the backward compatibility implications of making currently passing test suites with flaky tests (marked as xfail) to start to fail after upgrading pytest is a problem... 😟

[The-Compiler]

I'm not sure I understand - all I'm proposing is adding a new pytest.mark.flaky mark in addition to the existing xfail one, and not changing xfail 😉

[nicoddemus]

Sorry, I meant about this bit:

IMHO, xpassed should be red, and a new...

[The-Compiler]

Oh, sorry - now I understand.

That's only about the summary color though - IMHO it's acceptable that those are red on xpassed tests, as long as the exit status still is 0. It should be a good reminder for people to start using the flaky marker, but not break anything using a CI or similar.

[esiegerman]

[Thanks, @The-Compiler, for keeping this discussion going.]

... backward compatibility implications ... [for] flaky tests

At first I agreed about this, but I'm having serious second thoughts[1].

I've just done an exhaustive search of the docs for related strings: "xfail", "xpass", "flaky", "intermittent". What I've found is that all description of xfail's usage says that it's for tests that cannot succeed -- not a single mention of tests that might not succeed.[2]

So I have to wonder: how appropriate is it to preserve backward compatibility for the benefit of those who used the feature contrary to its documentation, at the expense of those who used it as the documentation says it's intended to be used?

I absolutely agree on the need to be able to mark flaky tests; but once such a marker is available, does xfail really have to remain so ambiguous that it's not really useful for its originally intended, and still documented, purpose?

[1] prompted largely by my failure to come up with nearly as good a name as "xfail" for a way to mark hard-failing tests

[2] The only references to xfail for flaky tests are pretty oblique:

• one example reason string in a changelog entry from v1.3.2, discussing something else entirely:

  Funcarg factories can now dynamically apply a marker to a test invocation. This is for example useful if a factory provides parameters to a test which are expected-to-fail:

  def pytest_funcarg__arg(request):
  request.applymarker(py.test.mark.xfail(reason="flaky config"))
  ...

  def test_function(arg):
  ...

  And even this is ambiguous -- the reason string says "flaky", but the text says "expected-to-fail"

• a couple of changelog entries refer to changes to xfail's behaviour that seem to make more sense in this context -- but that doesn't necessarily indicate a change of intention for xfail, just that even then, people were trying to shoehorn it into a use case for which it was never intended

Migration strategy

This is intended to minimize disruption to both camps.

1. Initially:
   • Add a flaky() marker and function, explicitly documented as being for intermittently failing tests; the outcome of a test so marked (whether it passes or fails) should not affect either the summary bar's color or pytest's exit status. flaky() could have report_if_passes and report_if_fails boolean arguments (or, equivalently, report_if={"passes" | "fails" | "both"}) that determine whether a line is printed for it in the list at the end of the run. (It could even have warn_if, similarly; but fail_if is of course pointless)
   • Add a boolean flaky parameter to xfail(); if True, treat the test as described above for flaky() (i.e. what xfail() does now); if False, treat a "passing" test as a hard failure -- red summary bar and non-zero exit status
   • Have that flaky= parameter default to True; this preserves existing behaviour for preexisting code
   • Issue: should the flaky= parameter be documented right from the start as a temporary, transitional feature?
2. After some appropriate amount of time has passed -- and with much warning to users:
   • Flip the default value for xfail()'s flaky= parameter's to False -- this makes xfail() with no parameter behave as one wants for a test that is always expected to fail
   • Deprecate flaky=True (but not flaky=False)
   • (optionally) Print a deprecation warning if True is passed
3. After some further amount of time has passed:
   • Remove support and documentation for xfail(flaky=True) ("remove support" means, treat it as an error)
   • Document flaky=False as historical and with no effect
4. (optionally) Deprecate xfail(flaky=False), and still later, remove the flaky= parameter entirely

This would leave us in (what I consider to be) a desirable end state:

• there's a way to mark tests that fail intermittently
• xfail isn't it :-)
• xfail is back to meaning a test is expected to fail every time; if the test passes, it's treated exactly as severely as the failure of a normal (i.e. unmarked) test

[nicoddemus]

Thanks for writing such a comprehensive proposal @esiegerman!

Perhaps you could send this proposal to the mailing list? Most users don't subscribe to the repository issues, and I think the mailing list might be a better media for the discussion.

[The-Compiler]

Also see #259 for some related issues - it seems the behaviour that xpass exited as an error already existed in earlier versions?

[pfctdayelise]

I can't say I'm a huge fan of pytest implicitly encouraging people to leave intermittently failing tests in their test suite. A test that you don't care if it passes or fails? May as well be a skip. Or just delete it, it's not telling you anything!

Intermittent fails are interesting and important because they tend to reveal where your tests have hidden dependencies. At least in the current state you are forced to confront them and hopefully find the cause of the flakiness. To me this is a pretty big change, to come from literally one comment from @hpk42 - I'd like to hear more about his use case exactly before this gets much more involved.

Sorry if this comes across as overly negative - but basically I think the rationale for inclusion needs a more rigorous examination. I'm open to being convinced. :)

[nicoddemus]

Hi @pfctdayelise,

Flaky tests are usually introduced in a code base due to complex code being tested (say multi threaded or asynchronous code) or due to platform differences (works on Windows but occasionally breaks on Linux, and vice-versa). Sometimes the flakiness is due to a problem in the test itself and the code it tests is fine and known to work. Flaky tests are of course a problem and should be dealt with as soon as possible in order to keep the test suite working and healthy.

The major problem with flaky tests is that they disrupt other developer's builds. It's frustrating for developers working on a piece of code in a feature branch, for their build to fail because of a completely unrelated test which was working fine in the previous build. Sometimes the developers which originally worked on the failing test are around and can take a look right away, but more often than not they might be busy with something else, like in a meeting or too entrenched in some other feature to help right away.

Our process when someone encounters a flaky test is to:

1. First, see if you can get in touch with the person who wrote the test to check if they can take a look at the problem and attempt to fix it right away;
2. If that person is not available, take a look at the test himself and perhaps fix it; sometimes the cause of the "flakyness" is obvious (like tests running in parallel which share resources like a file for example);
3. As a last resort, open a ticket for the flaky test addressed to a developer and mark the test as xfail with a reason which includes the ticket number and any other relevant information. This ticket is usually put at the top of the backlog so it can be worked on as soon as possible.

This process prevents flaky tests, which unfortunately do happen, from disrupting the workflow of the other developers and makes it clear that there are issues to be solved in the test suite.

Having said all that, I think the proposal of having a specific mark for flaky which means "don't break the test suite if this fails but note that it passes" is sound. It has a different purpose than a skip, which usually means "this test should be skipped because of reasons", with reasons being not able to run in a certain platform, python version, OS, etc. IOW, flaky tests should send the message "this should be looked into", while skipped tests are OK and not a problem. Having a mark for tests that are expected to fail but don't break the test suite, but break the suite if they start suddenly to pass also has its uses: in some situations, you create a test for a feature which is failing right now, but once it is fixed then the test should have the mark removed and promoted to a "normal" test. Currently with "xfail"'s behavior it is easy for developers to fail to notice this and let the mark in there forever, eternally xpassing.

Hope my points make some sense, and sorry for the long post! 😅

[pfctdayelise]

OK, what you have said makes sense, but it sounds like an argument for showing xpass results more prominently (which I think is what happened in #776?) (which I also agree with).

Actually #716 sounds like xfail behaviour has recently changed. If this is the case, I don't know what's going on anywhere any more!

My understanding of Jenkins is that it expects your test suite to "pass" (all tests act as expected) or "fail". There is no in between. I don't know if Travis or other CI systems have more nuance there. If all tests act as expected, IME nobody looks at the results.

OK so maybe what's being proposed is this:
CURRENTLY/PREVIOUSLY: xfail test that xpasses - happens silently (doesn't break a build)
#776: xfail test that xpasses - happens noisily (will break a build)
PROPOSAL HERE: xfail test that xpasses - happens silently - to be called 'flaky' (won't break a build)

Have I got it right?

[pfctdayelise]

Having thought about it some more, maybe another idea would be to have a strict=True/False parameter for the xfail mark. (If strict, an xpass result should break the build.) If a new mark is introduced, it would need to be mindful of how it interacts with things like the --runxfail option.

[nicoddemus]

Have I got it right?

Yep. 😄

If a new mark is introduced, it would need to be mindful of how it interacts with things like the --runxfail option.

That's a good point. Perhaps keeping a single mark with a flag is the way to go.

[dariuspbom]

xfail originally expected the test to fail, if it didn't and xpass instead then the test job was failed with non zero exit code and red.

This was changed, apparently to handle flaky tests. I'm sure I found this on the mailing list as to why it was changed. I'm sure I also found the version that it changed in. Sorry didn't keep the references to them.

There appears to be concern about breaking backward compatibility now that xfail is used for flaky tests and can either xfail or xpass with zero exit code and green.

However the flip side is that backward compatibility has already been broken in the first change. Test suites that were expecting xfail to expect the test to fail and if it xpass then failed the test job with non zero exit code and red are now broken. Even though it is a silent failure, those tests are no longer working as they were written.

So changing xfail back to the original strict behaviour then put up with tests failing loudly because they are flaky and need the mark updated.

Alternatively keeping xfail with current flaky behaviour then put up with tests silently xpass which hides problems and need the mark updated.

One is loud and one is silent but they are both important.

Given that both options have caused backward compatibility problems I'd prefer the semantic naming to make sense. In #716 I suggested xfail and xfailorxpass for flaky. Having xfail and flaky is fine (and possibly better than xfailorxpass).

If xfail is going to be overloaded with a flag and used for both then perhaps we can have a global setting in conftest rather than a default of flaky.

On our project I changed our conftest to detect xpass tests and set the exit code back to non zero. We have over 10,000 tests in pytest which is run under jenkins and we need xfails to match the expected outcome which is to fail and if for some reason it xpass then we need the non zero exit code so that we can be alerted to this as a failed build.

See also #716 #259 #810 #776