Skip to content

Add conftest.py to increase timeouts for slow tests#894

Open
smburdick wants to merge 10 commits into
tqec:mainfrom
smburdick:add-conftest
Open

Add conftest.py to increase timeouts for slow tests#894
smburdick wants to merge 10 commits into
tqec:mainfrom
smburdick:add-conftest

Conversation

@smburdick
Copy link
Copy Markdown
Collaborator

By default tests have a 30-second timeout, which is more than enough time for the non-slow tests run in our pipeline.

Use conftest to set a higher timeout for the slow tests, which are run with pytest -m "slow".

@smburdick smburdick requested review from HaoTy, jbolns and nelimee April 2, 2026 17:09
@smburdick smburdick self-assigned this Apr 2, 2026
@smburdick smburdick added bug Something isn't working ci/cd Testing, review, release QOL Improves usability and functionality labels Apr 2, 2026
@smburdick
Copy link
Copy Markdown
Collaborator Author

Ran tests without a pre-existing detector database, and each one takes <= 67s on my machine.

@HaoTy
Copy link
Copy Markdown
Collaborator

HaoTy commented Apr 8, 2026

tqec/tqecd#45 is expected to be merged soon, so we may not even need to have the slow marker. Will take a look then.

@smburdick
Copy link
Copy Markdown
Collaborator Author

tqec/tqecd#45 is expected to be merged soon, so we may not even need to have the slow marker. Will take a look then.

I like it, but, the full test suite is actually broken atm due to the timeouts not accommodating the slow tests.

@Hadar01
Copy link
Copy Markdown

Hadar01 commented Apr 29, 2026

Hi all — sharing an AI-assisted code review of this PR. Generated by github-agent and then human-curated against pytest / pytest-timeout source before posting. The raw output, the prompt fixes that took it from "factually wrong" to "useful," and the curation workflow are all in examples/ on the agent's repo. Happy to delete this comment if it ends up being noise.


Verdict: NEEDS_DISCUSSION — not blocking. The diff is minimal and faithful to the issue. One subtle behavioural footgun seemed worth flagging before merge.

The footgun

When a slow test also carries an explicit @pytest.mark.timeout(N) decorator, the conftest's appended marker is silently ignored — the explicit one wins, regardless of whether N is shorter or longer than SLOW_TEST_TIMEOUT.

  • Fine for the case where someone writes @pytest.mark.timeout(900) on a heavy slow test (their 900 is preserved).
  • A footgun for the case where someone writes @pytest.mark.timeout(60) on a slow test (perhaps for debugging, perhaps by accident) — the conftest's 300s bump silently doesn't apply.

Why (with citations)

add_marker(..., append=True) (default — pytest source) appends to own_markers. pytest-timeout reads via item.get_closest_marker("timeout") (pytest-timeout source) which returns next(iter_markers(...)) — the first marker in own_markers. The explicit decorator was applied at collection time, before this hook runs, so it sits at index 0 and wins.

Runtime-verified on pytest 8.3.5 + pytest-timeout 2.4.0; reproducible 4-file project here.

Suggested fix

def pytest_collection_modifyitems(items):
    """Increase timeouts for tests marked as slow, unless the test has its
    own explicit timeout marker (which we leave alone)."""
    for item in items:
        if item.get_closest_marker("slow") and not item.get_closest_marker("timeout"):
            item.add_marker(pytest.mark.timeout(SLOW_TEST_TIMEOUT))

Mechanically equivalent to today's behaviour (the explicit decorator would win anyway), but makes the precedence visible at the call site and doesn't depend on pytest-timeout's marker-resolution rules.

Minor: coupling worth a comment

The diff increases timeouts for slow tests only. The 30 s baseline for non-slow tests comes from [tool.pytest.ini_options] timeout = 30 in pyproject.toml — outside this diff. If that line is ever removed, fast tests silently lose all timeouts. A short comment in conftest.py pointing at the pyproject setting would make the coupling discoverable. Not blocking.


Genuinely interested in whether the multi-marker case maps to anything that actually happens in tqec's test suite — if no slow tests carry their own timeout(...) decorator today, the conftest is fine as-is and the guard is just future-proofing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ci/cd Testing, review, release needs-review QOL Improves usability and functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants