Fix stdout/stderr redirects

[kenodegard]

Resolves #24

Changes include:

• Ruff version bump and enabled some additional lints (happy to remove these changes if undesired)
• Add a TDD test used to showcase problematic behavior
• Call item.ihook.pytest_runtest_call(item=item) instead of directly calling item.runtest() to benefit from pytest's automatic handling of sys.last_type, sys.last_value, sys.last_execption (apparently not handling this causes stdout/stderr to be redirected and inaccessible within tests, see https://github.com/pytest-dev/pytest/blob/3b798e54221f1895a983000c7e5bc8afdacd5011/src/_pytest/runner.py#L165-L182)
• Switch from pytest_runtest_protocol hook to pytest_pyfunc_call hook (deeper in the call stack, pytest_pyfunc_call is invoked from runtest which in turn is called from pytest_runtest_call)

[jezdez]

Nice, thanks @kenodegard!

[codspeed-hq]

CodSpeed Performance Report

Merging #25 will not alter performance

_{Comparing kenodegard:fix-outerr-redirects (26b5d09) with master (90e639b)}

Summary

✅ 5 untouched benchmarks

[art049]

Thank you, agree entirely, we need this :)
No problem with the ruff bump and config improvement; love it!

LMK what you think about the details

[art049]

In the current state this might introduce variance in the code we measure since we'll also measure the pytest hook.
I think it would be better to do it the other way around: have pytest_runtest_call call _run_with_instrumentation

A way to do it might be to and override runtest but don't hesitate if you have another idea!

[kenodegard]

After digging some more I found we can use pytest_pyfunc_call instead of pytest_runtest_protocol (runtest invokes this hook, see https://github.com/pytest-dev/pytest/blob/5bb1363435a8cb3e2010505dbeb1e015c36beed6/src/_pytest/python.py#L1762-L1764)

I got the tests to pass locally but I may be missing something so let me know if you don't think this is the right thing to do

[kenodegard]

@art049 thanks for running the tests

pushed 59ea7ad to resolve the typing failure in Python<3.10 and 2b026dd to resolve the perf trampoline test failure on Python 3.12

[kenodegard]

@art049 anything more to do here?

[art049]

It's a bit annoying that formatter changes and logic change are in the same PR in the end.
I'll maybe try to split things out.

[art049]

Since you're touching the core of the plugin, I have some additional feedback just to make sure the behavior stays the same.

Really appreciate you refactoring the test protocol thing! 🔥

Edit: thanks for splitting the formating things into #29 !

[art049]

When used with the fixture, the fixture payload shouldn't be executed wrapped again within pyfunc_call since it's already wrapped by the test itself.

for example, this would probably fail because of the warmup:

# This doesn't have any pytest marker but defines a bench since its using the fixture
def test_bench(benchmark):
    # ... some preprocessing
    called_once = False

    @benchmark
    def _():
        nonlocal called_once
        if not called_once:
            called_once = True
        else:
            raise Exception("bench codewas called twice but actual bench context only once")

[art049]

Don't hesitate to add that as an additional test

[kenodegard]

I started to get really confused with the intention here since the above example also fails with master, see #30

I suspect we need to do something similar to what pytest-rerunfailures does to clear the cached results between the cache priming run and the instrumented run: https://github.com/pytest-dev/pytest-rerunfailures/blob/a53b9344c0d7a491a3cc53d91c7319696651d21b/src/pytest_rerunfailures.py#L565-L567

[art049]

Love this refactor!
However, when a benchmark is defined from a fixture, we should still perform the warmup at this level and not in the benchmark fixture(see the other comment I left on it).

[art049]

I think the warmup shouldn't be included in the wrapper but handled at the protocol level

[kenodegard]

Ah, this also explains the issue we ran into on our own trying to use tmp_path in benchmark tests, e.g.:

@pytest.mark.benchmark
def test_tmp_path_benchmark(tmp_path: Path):
    tmp_path.mkdir()

[art049]

since you introduced this try block, can you add a test bench raising an exception?