Fix crash that happened under --runxfail -o empty_parameter_set_mark=xfail

[gnikonorov]

Closes #4497

Make sure runxfail plays nicely with empty_parameter_set_mark = xfail

[gnikonorov]

So now the file in the linked issue would be processed as so:
4497_new_behavior.txt
Versus:
4497_original_behavior.txt

[gnikonorov]

I think this is good to go @RonnyPfannschmidt, barring review

[RonnyPfannschmidt]

that change makes the fixture helper incorrect - if no parameter is existing this one has to raise a failure - the tests shouldnt even run however

[nicoddemus]

Shouldn't this raise an error then, or perhaps pytest.fail("no direct parameter")??

[RonnyPfannschmidt]

i propose introducing a runnable=True/False extra indication on xfail, so that those that are marked as absolutely not runable can trigger a warning when runxfail is given instead of running and failing

[gnikonorov]

Thanks for explaining @RonnyPfannschmidt

Just to clarify, you're suggesting that if runnable=True, we just catch the thrown exception and raise a warning instead?

As it stands right now, this change will just ignore the empty parameterset and run the test if xfail_strict=False. If it is true, then it fail the test with [XPASS(strict)] got empty parameter set ('a', 'b'), function test at /home/gnikonorov/pytest/gleb_test/test_runxfail_with_empty_parameter_set_mark_xfail.py:2. Why is this incorrect behavior?

Here's a run with xfail_strict true/false: runnable_true_false.txt

And, now the empty parameterset values are set to None if xfail_strict=False.( which I think seems reasonable ):

@pytest.mark.parametrize(
        ('a', 'b'),
        # no cases defined yet
        (
        ),
)
def test(a, b):
>       raise ValueError('{} {}'.format(a,b))
E       ValueError: None None

[RonnyPfannschmidt]

missing parameter literally means you have nothing to run any test with - running a test with a different value is wrong
the problem isn't that we see a exception/error, the problem is that this is a xfail that is under no circumstances "runnable"
perhaps we need to rethink the interpretation of xfail in general, but we certainly shouldn't make a test "fake-runnable"

[gnikonorov]

Thanks for explaining @RonnyPfannschmidt. Do we want to add a runnable flag to xfail though? To me it seems like the point of runxfail is to force a run so we’d be blocking runxfail.

I guess this wasn’t as straight forward as I originally thought

[RonnyPfannschmidt]

to be fair i would rather not have this type of flag at all, its just one possible solution

im not sure what a good option looks like

[gnikonorov]

agreed. In this case the user wouldn’t be able to add the flag since the test is being marked xfail by -o. Unless they added another command line flag for it which seems cumbersome

[nicoddemus]

i propose introducing a runnable=True/False extra indication on xfail

We already have the run parameter to xfail, or do you mean something else?

[bluetech]

We already have the run parameter to xfail, or do you mean something else?

The xfail that is added for the empty parameterset is indeed pytest.mark.xfail(run=False), however --runxfail overrides even that. Perhaps it shouldn't?

[RonnyPfannschmidt]

i just took a quick look at xfail and how it monkeypatches the pytest module,
and i believe it can be sorted by having the direct parameter function raise an actual fail

instead of using pytest.xfail() it would use raise pytest.xfail.Exception which works even with --runxfail

the direct fixture value function would then be something that raises a error indicating that the test was not given a parameter for the value

[gnikonorov]

that works for me, is everyone else on board? @nicoddemus @bluetech

[nicoddemus]

instead of using pytest.xfail() it would use raise pytest.xfail.Exception which works even with --runxfail

I'm probably misunderstanding, but pytest.xfail just raises XFailed:

@_with_exception(XFailed)
def xfail(reason: str = "") -> "NoReturn":
    ...
    raise XFailed(reason)

And pytest.xfail.Exception is XFailed.

So isn't it the same thing?

[bluetech]

I think @RonnyPfannschmidt refers to the nasty hack in skipping.py:

pytest/src/_pytest/skipping.py

Lines 49 to 61 in d69e9e6

	def pytest_configure(config: Config) -> None:
	if config.option.runxfail:
	# yay a hack
	import pytest
	old = pytest.xfail
	config._cleanup.append(lambda: setattr(pytest, "xfail", old))
	def nop(*args, **kwargs):
	pass
	nop.Exception = xfail.Exception # type: ignore[attr-defined] # noqa: F821
	setattr(pytest, "xfail", nop)

However this hack is not relevant for the xfail mark pytest.mark.xfail that the empty paramterset is using, because the mark code imports the xfail function directly from _pytest.outcomes and thus is not affected by the hack.

The skipping code itself checks if not item.config.option.runxfail directly.

[nicoddemus]

Ahh thanks @bluetech that explains it!

[gnikonorov]

can someone explain why we have this hack in the first place? It does seem really dirty

[RonnyPfannschmidt]

this hack is to prevent the imperative pytest.xfail("reason") from raising a exception when runxfail is set to true
as a followup we might want to consider a internal pytest context var which carries the config object
for those features that would need a contextual link to the current pytest configuration, but cant pass it without making the external api worse

[gnikonorov]

Thanks for explaining @RonnyPfannschmidt

[obestwalter]

@gnikonorov - would you still be interested in finishing this up? Otherwise it might be better to close this.

[RonnyPfannschmidt]

closing for now , happy to reopen when work is resumed