Tests marked xfail are reported as failures

[rodrigogiraoserrao]

Hey there, thanks for your work on syrupy!

I'm wondering if the fact that XFAIL tests are reported as failures is an intended design decision, a bug, or something you haven't contemplated yet.

The full context is from Textualize/textual#2282 but, in short, I have a snapshot test that is marked with xfail:

However, at the end, I get a report saying that one snapshot test failed:

I expected to see a yellow warning saying that one snapshot test gave an expected failure instead of the red warning saying that the test failed, especially taking into account the confusing contrast with pytest, which happily reports that the tests passed.

[rotu]

The "snapshot failed" message is confusing, not just confined to xfail.

E.g. the below test passes but it also displays "1 snapshot failed."

import random

def test_nomatch(snapshot):
    assert (random.random() != snapshot())

[noahnu]

@rotu We don't support negation. I'm not sure how negation would work with updating snapshots. If you're interested in creating a feature request proposal for how negation would work, or contributing an error message when negation is detected, it'd be welcome. I know you still have that PR contribution open, I've been quite busy but will see what can be done with it.

[noahnu]

Support for "failed" snapshots manifests in two ways:

• Using the pytest.xfail annotation
• Capturing the Assertion Error exception (which we do in a syrupy test itself)

In both instances, the test itself passes but Syrupy prints the failure. This is because Syrupy is determining the pass/fail immediately upon assertion (== snapshot). It doesn't look at the test case itself.

Here is where we collect the assertion result: https://github.com/tophat/syrupy/blob/5eee3d845cd3d3c4a47ea981911e5d4f8ebe83e0/src/syrupy/assertion.py#L305

Rather than defer the assertion result collection, I think we should be able to do a pass over the assertion results once the session completes and update any assertion results based on the state of the test case.

I'm definitely interested in fixing this (and sorry for the long delay in my response). I'll take a stab at this this weekend.

[rotu]

it makes sense to not support inequality and my example is quite contrived! The issue is that there are really 3 parts of a snapshot assertion:

• declaring a snapshot
• checking equality
• asserting on that equality

and I’m not clear what the semantics should be.

• if we call snapshot() once but never check equality, is that a problem?
• If we set x = snapshot(*opts); assert y == x; assert z == x does that mean the test should only pass if y==z?
• If we check equality but don’t assert on the result what should happen?

[tophat-opensource-bot]

🎉 This issue has been resolved in version 4.0.6 🎉

The release is available on:

• v4.0.6
• GitHub release

Your semantic-release bot 📦🚀

[rodrigogiraoserrao]

@noahnu Thanks for fixing this! All the best :)