Fix passenv CI in tox ini and make tests insensitive to the presence of the CI env variable

[ogrisel]

See: #13679 (comment)

[ogrisel]

@webknjaz CI is green again!

[webknjaz]

LGTM, but I'll let @RonnyPfannschmidt merge since he was active in the related PR.

[ogrisel]

@RonnyPfannschmidt if you have a few minutes to finalize this review, that would be very appreciated as it would unlock my work to finalize #13679.

[RonnyPfannschmidt]

This looks good

Tho only nitpick that comes to mind is that it could be more declarative

But thats a low hanging fruit followup

[nicoddemus]

I believe we want to remove this env var for all tests, correct?

If so, I suggest you add this autouse fixture to conftest.py:

@pytest.fixture(autouse=True)
def remove_ci_env_var(monkeypatch: MonkeyPatch) -> None:
    """
    Make the test suite insensitive if it is running in CI or not.
    
    If a test requires the variable, it should set it explicitly.
    """
    monkeypatch.delenv("CI", raising=False)

Then you can revert the changes done to the tests.

[ogrisel]

Actually, we might want to use this env var in #13679 to use a longer timeout value in some tests to decrease likelihood of failure on slow CI runners.

[ogrisel]

To be more specific: there is an existing test named test_timeout in test_faulhandler.py that relies on the presence of this variable to adjust the value of the timeout to make it less likely to fail in case the CI runner is too slow (see #7022). However, this strategy never worked as intended because the CI variable propagation was always blocked by the existing configuration in tox.ini.

[webknjaz]

Yeah, I wasn't sure what the impact would be in other places so I didn't suggest making it an autouse fixture.

[ogrisel]

I will update the PR to introduce the fixture but not make it auto-use for now.

[ogrisel]

@nicoddemus @webknjaz @RonnyPfannschmidt done in c34d9b5.

Once merged, I can follow up with a PR to update test_timeout to remove the skip marker and then trigger the tests on the CI a few times in a row to check that it can properly fix #7022 without having to skip the test entirely on the CI. Then I will update #13679 accordingly.

[nicoddemus]

To be more specific: there is an existing test named test_timeout in test_faulhandler.py that relies on the presence of this variable to adjust the value of the timeout to make it less likely to fail in case the CI runner is too slow

Ahh I see.

In that case we could then create a new mark that skips deleting the env var if present in a test.

@pytest.fixture(autouse=True)
def remove_ci_env_var(monkeypatch: MonkeyPatch, request: pytest.FixtureRequest) -> None:
    """
    Make the test insensitive if it is running in CI or not.

    Use `@pytest.mark.keep_ci_var` in a test to avoid applying this fixture, letting the test
    see the real `CI` variable (if present).
    """
    has_keep_ci_mark = request.node.get_closest_marker("keep_ci_var") is not None
    if not has_keep_ci_mark:
        monkeypatch.delenv("CI", raising=False)

We can apply this mark to test_timeout and any other test that requires seeing the real CI variable.

@ogrisel I understand this is PR is a side quest just to work on #13679, but would mind give the autouse fixture a go? I think the time to try this approach is now that we have someone on hands on the issue.

If there are too many breakages and you decide to not deal with them now, feel free to revert the autouse change and merge as is currently.

[nicoddemus]

I'm leaving pre-approved. Would appreciate you giving the autouse approach one more try before merging though.

[ogrisel]

Alright, let me give this a try.

[ogrisel]

I removed the skip mark on test_timeout to check whether the keep_ci_var works as expected.

Previously, this test would have failed with high probability on at least one of the CI runner. Now that we propagate the CI variable, it seems that this test pass on most of the CI runners but we got still got one failure: https://github.com/pytest-dev/pytest/actions/runs/17409387341/job/49422461654?pr=13684

Let me re-add the skip mark for now to decouple the different PRs.

[ogrisel]

So keep_ci_var seems to work as expected. Unfortunately, increasing the timeout conditionally on the presence of the CI variable does not seem to be enough to reliably fix #7022 as observed in #13684 (comment).

[nicoddemus]

Thanks @ogrisel.

Bummer that test_timeout still fails on occasion.

[patchback]

Backport to 8.4.x: 💚 backport PR created

✅ Backport PR branch: patchback/backports/8.4.x/bf92debb4fd874289f6aed29c6947b611b1de0ae/pr-13684

Backported as #13694

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.