Make spawn timeout into a pytest argument to solve false negative in tests

[RR5555]

Issue

When running the tests for the repo on gitpod.io, I kept on running into failed tests due to timeout of the spawn from pexpect:

=================================================================================== short test summary info ====================================================================================
SKIPPED [1] tests/test_prompt.py:609: TODO: fix this
SKIPPED [1] tests/test_settings.py:30: Windows-only test
XFAIL tests/test_updatediff.py::test_update_needs_more_context[True-1] - Not enough context lines to resolve the conflict.
XFAIL tests/test_updatediff.py::test_update_needs_more_context[True-2] - Not enough context lines to resolve the conflict.
XFAIL tests/test_updatediff.py::test_update_needs_more_context[False-1] - Not enough context lines to resolve the conflict.
XFAIL tests/test_updatediff.py::test_update_needs_more_context[False-2] - Not enough context lines to resolve the conflict.
FAILED tests/test_output.py::test_messages_with_inline_text[True] - pexpect.exceptions.TIMEOUT: <pexpect.popen_spawn.PopenSpawn object at 0x7efffa74bc40>
FAILED tests/test_output.py::test_messages_with_included_text[True] - pexpect.exceptions.TIMEOUT: <pexpect.popen_spawn.PopenSpawn object at 0x7efffa7542e0>
FAILED tests/test_conditional_file_name.py::test_answer_changes[True] - pexpect.exceptions.TIMEOUT: <pexpect.popen_spawn.PopenSpawn object at 0x7f0a2420a640>
FAILED tests/test_prompt.py::test_update_multiselect_choices[float] - pexpect.exceptions.TIMEOUT: <pexpect.popen_spawn.PopenSpawn object at 0x7ff88dd7db50>
FAILED tests/test_prompt.py::test_update_multiselect_choices[str] - pexpect.exceptions.TIMEOUT: <pexpect.popen_spawn.PopenSpawn object at 0x7fd3b0ab83d0>
========================================================= 5 failed, 961 passed, 2 skipped, 4 xfailed, 2 warnings in 288.16s (0:04:48) ==========================================================

The cause is the hardcoded timeout=10 and timeout=30 when calling the spawn fixture, resulting in the same timeouts when the underlying pexpect.popen_spawn is called. And it seems more time is needed in the gitpod.io dev environment than in the github CI tests.

---

This PR

Description

Thus, to modify all of the timeout values easily at pytest launch, this PR soft-code these timeout values through a fixture spawn_timeout set by a newly defined pytest argument --spawn-timeout (whose type is int| None using a small lambda function checker type). This PR also documents the usage of this argument when facing the previously described issue above when trying to contribute.

spawn_timeout can be None or -1 to follow the values of timeout in pexpect.popen_spawn .

The default values for the timeouts are the same as the hard-coded ones. A choice have been made regarding the timeouts of 30 as the spawn_timeout default value is 10 to fit the majority of the values: the value for 30 is soft-coded as spawn_timeout*3 (instead of an alternative spawn_timeout+20) when spawn_timeout is a positive int (to preserve the -1- and None-value use-cases).

Passing the --spawn-timeout pytest argument in the CLI or in the VSCode Workspace settings is described in the docs addition, along with the options it brings of either extending the timeout or removing it (when None).

Additional from __future__ import annotations have been added when needed to accommodate the type int | None of the spawn_timeout fixture, given that .python-version is 3.9.

Usage [As described in the doc]

If you get fails due to pexpect.exceptions.TIMEOUT: <pexpect.popen_spawn.PopenSpawn object at 0x............>, you can adjust the timeout to a longer one (default: 10), or remove the timeout (None).
Either add it as an argument in your command:

uv run poe test --spawn-timeout None

Or modify pytest arguments in VSCode Workspace settings:
.vscode/settings.json

{
  ...
  "python.testing.pytestArgs": [
    "--spawn-timeout=None"
  ]
}

[sisp]

Thanks for discovering this timeout issue with Gitpod and submitting a PR with a fix! 🙇

Making the timeout configurable via a pytest argument is a great idea! 👌 Just two comments/requests:

• Instead of using the new spawn_timeout fixture in every test that calls spawn, I'd modify the spawn fixture to use spawn_timeout value as a default value when spawn's timeout is None. Only for the few occurrences where the timeout value has been 30 would I use the spawn_timeout fixture in the test and call spawn with timeout=spawn_timeout * 3. This way, we can avoid the many extra spawn_timeout fixture uses in tests.
• Do we really need to be able to set the timeout to pexpect's default value? Being able to pass None feels a bit uncommon, and we might not even need it. I think the default timeout of 10s is fine, and it can always be overridden with a different value. TBH, I'd have to check pexpect's documentation to even know what their default value is. 😄

[RR5555]

Thank you very much for reviewing my PR ^^

Comment/Request 1: Implemented

Please let me know if further modifications regarding that request are needed.

Comment/Request 2: About pexpect timeout value types

In conftest.py:spawn

  # Disable subprocess timeout if debugging (except coverage), for commodity
  # See https://stackoverflow.com/a/67065084/1468388
  tracer = getattr(sys, "gettrace", lambda: None)()
  if not isinstance(tracer, (CTracer, type(None))):
    timeout = None

But that does actually not apply with coverage or when using: uv run poe test.

In pexpect.popen_spawn.PopenSpawn

class PopenSpawn(SpawnBase):
    def __init__(self, cmd, timeout=30, maxread=2000, searchwindowsize=None,
                 logfile=None, cwd=None, env=None, encoding=None,
                 codec_errors='strict', preexec_fn=None):
        super(PopenSpawn, self).__init__(timeout=timeout, maxread=maxread,
                searchwindowsize=searchwindowsize, logfile=logfile,
                encoding=encoding, codec_errors=codec_errors)

In pexpect.spawnbase.SpawnBase parent class for pexpect.popen_spawn.PopenSpawn

class SpawnBase(object):
...
	def __init__(self, timeout=30, maxread=2000, searchwindowsize=None,
                 logfile=None, encoding=None, codec_errors='strict'):

In pexpect.spawnbase.SpawnBase.expect method's docstring

When the keyword argument timeout is -1 (default), then TIMEOUT will raise after the default value specified by the class timeout attribute. When None, TIMEOUT will not be raised and may block indefinitely until match.

Summary [from my understanding]

Default for the class timeout is 30 in both the parent class SpawnBase and the class PopenSpawn.

timeout value	Effect
None	No timeout, wait indefinitely
-1	Default to the class instance timeout
x where x is a positive int	timeout=x

[sisp]

Looks great! 👌 Just a few remarks and an idea to use 0 as ∞.

[sisp]

Fantastic work, @RR5555! 🥇 Just two minor suggestions, then we're ready, I believe. 🎉

[RR5555]

Can I make the pytest argument type validator stricter?
Since we are discarding the case -1 anyway when writing timeout=spawn_timeout * 3,, I think it would be nice to catch negative values at the argument parsing level to return a nice error message, instead of losing test time, and hitting each TIMEOUT errors at pexpect function calls with a message not mentioning that the --spawn-timeout value is responsible.

	type=lambda x: int(x)
        if int(x) >= 0
        else sys.exit(
            f"\033[31mERROR: usage: pytest --spawn-timeout <int>\npytest: error: argument --spawn-timeout: should be positive or `0`: `{x}`\n\33[0m"
        ),

Resulting in:

[sisp]

Can I make the pytest argument type validator stricter?

Makes sense. AFAIK, pytest's options parser is based on argparse, so you can create a custom type and raise, e.g., a ValueError when the timeout value is less than -1.

The argument to type can be a callable that accepts a single string or the name of a registered type (see register()) If the function raises ArgumentTypeError, TypeError, or ValueError, the exception is caught and a nicely formatted error message is displayed.

— https://docs.python.org/3/library/argparse.html#type

For example:

def timeout_type(value: Any) -> int:
    value = int(value)
    if value >= -1:
        return value
    raise ValueError("Timeout must be a non-negative integer")


def pytest_addoption(parser: Parser) -> None:
    parser.addoption(
        "--spawn-timeout",
        action="store",
        default=10,
        help="timeout for spawn of pexpect",
        type=timeout_type,
    )

[RR5555]

I have considered that way before proposing the degraded alternative with a sys.exit.
The problem is that it does not pass down the ValueError message. Instead, it returns a not so informative message with the only hint being the name of validator function invoked:

So far, I only see 3 alternatives:

• Either we go outside the capturing error scope of pytest argparse and mute the traceback:

  def timeout_type(value: Any) -> int:
      value = int(value)
      if value >= -1:
          return value
      sys.tracebacklimit = 0
      raise TimeoutError("Timeout must be a non-negative integer")

  In which case, we lose the formatting of the pytest argparse error message.
• Or we mimic the output of the pytest error capture by exiting with a specifically formatted message (cf. initial proposal).
• Or we modify the pytest argparser, or we try to input "" to option_id in the pytest ArgumentError (haven't cracked these ones yet, so not sure how viable it might be).

Please let me know if you see any better way to handle this ^^

[sisp]

It seems that raising ValueError swallows the message as you described. But raising argparse.ArgumentTypeError works:

def timeout_type(value: str) -> int:
    ivalue = int(value)
    if ivalue >= -1:
        return ivalue
    raise ArgumentTypeError("Timeout must be a non-negative integer or -1")


def pytest_addoption(parser: pytest.Parser) -> None:
    parser.addoption(
        "--spawn-timeout",
        action="store",
        default=10,
        help="timeout for spawn of pexpect",
        type=timeout_type,
    )

$ pytest --spawn-timeout -3
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: argument --spawn-timeout: Timeout must be a non-negative integer or -1

By the way, I've updated the custom type above a little to satisfy mypy and include -1 in the error message.

[RR5555]

Oh, nice!

I've applied modifications, but 2 points require your arbitration:

• I've put the type validator function definition inside of the pytest_addoption function using it. I thought it made more sense scope-wise. But I can put it outside if it is preferable.
• I've excluded -1 from the valid values because of the simplification we made in test_prompt.py: We removed ternaries in favor of a simpler timeout=spawn_timeout * 3. If we accept -1 back, then I'll change the type validator and add back the ternaries for the 2 lines in test_prompt.py.

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.00%. Comparing base (4831f35) to head (7172b8a).
⚠️ Report is 97 commits behind head on master.

Files with missing lines	Patch %	Lines
tests/conftest.py	73.33%	4 Missing ⚠️
tests/test_prompt.py	95.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2250      +/-   ##
==========================================
+ Coverage   97.78%   98.00%   +0.21%     
==========================================
  Files          55       55              
  Lines        6088     6106      +18     
==========================================
+ Hits         5953     5984      +31     
+ Misses        135      122      -13     

Flag	Coverage Δ
unittests	98.00% <91.66%> (+0.21%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sisp]

I've applied modifications, but 2 points require your arbitration:

• I've put the type validator function definition inside of the pytest_addoption function using it. I thought it made more sense scope-wise. But I can put it outside if it is preferable.

Looks good, no need to change it. 👍

• I've excluded -1 from the valid values because of the simplification we made in test_prompt.py: We removed ternaries in favor of a simpler timeout=spawn_timeout * 3. If we accept -1 back, then I'll change the type validator and add back the ternaries for the 2 lines in test_prompt.py.

Sorry, I got confused about the -1 value. The current solution without -1 is great, no need to change it.

---

Only a minor formatting request, other than that it looks great. 👍 Thank you! 🙇

[sisp]

Thanks for this great contribution, @RR5555! 🙏 LGTM! 🎉

[RR5555]

Thank you very much for your guidance and your patience ^^