Conversation
Mesa DescriptionThis PR introduces a configurable timeout for A new helper function, Description generated by Mesa. Update settings |
There was a problem hiding this comment.
Performed full review of b37b87b...fae9e3b
Analysis
-
The implementation creates a dependency on the pytest-timeout plugin without properly handling environments where it's not available.
-
Hard-coded defaults in the implementation don't fully respect the timeout semantics of pytest-timeout (like float values, keyword arguments, and timeout=0 to disable).
-
The architectural linkage between the pytest plugin and RPyC bootstrap layer needs a more robust adapter layer before it's safe to ship.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 2 comments | Edit Agent Settings • Read Docs
| timeout_marker = pyfuncitem.get_closest_marker("timeout") | ||
| if timeout_marker and timeout_marker.args: | ||
| return int(timeout_marker.args[0]) | ||
| try: |
There was a problem hiding this comment.
pyfuncitem.config.getini("timeout") only works if the pytest-timeout plugin (or some other plugin/register) declared that ini option. In a plain pytest run without pytest-timeout installed, calling getini("timeout") raises ValueError during collection, so every @pytest.mark.in_container test now crashes even though the user never asked for a timeout. Please guard the lookup (e.g., wrap in try/except ValueError or check config.inicfg first) and fall back to your default only when the option actually exists.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/pytest-in-docker#4
File: pytest_in_docker/_plugin.py#L108
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
`pyfuncitem.config.getini("timeout")` only works if the pytest-timeout plugin (or some other plugin/register) declared that ini option. In a plain pytest run without pytest-timeout installed, calling `getini("timeout")` raises `ValueError` during collection, so every `@pytest.mark.in_container` test now crashes even though the user never asked for a timeout. Please guard the lookup (e.g., wrap in `try/except ValueError` or check `config.inicfg` first) and fall back to your default only when the option actually exists.
| """Read the pytest timeout marker, falling back to the ini default or 30s.""" | ||
| timeout_marker = pyfuncitem.get_closest_marker("timeout") | ||
| if timeout_marker and timeout_marker.args: | ||
| return int(timeout_marker.args[0]) |
There was a problem hiding this comment.
This helper only looks at positional marker args and coerces them to int, which diverges from pytest-timeout’s documented behaviour:
@pytest.mark.timeout(timeout=10)(keyword form) is ignored and you fall back to the ini/default.- Fractional values that pytest-timeout accepts (e.g.,
0.5) get truncated toward zero, so short floating timeouts ortimeout=0(the official way to disable) become a zero-second RPC limit. - Pytest-timeout’s default is
0meaning “no limit”, but when neither a marker nor ini entry is present you now force a 30‑second timeout.
If the goal is to align with pytest, this needs to honour keyword arguments, keep the float precision, and treat 0/None as “no timeout” instead of enforcing an immediate or 30‑second cut-off.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/pytest-in-docker#4
File: pytest_in_docker/_plugin.py#L107
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
This helper only looks at positional marker args and coerces them to `int`, which diverges from pytest-timeout’s documented behaviour:
* `@pytest.mark.timeout(timeout=10)` (keyword form) is ignored and you fall back to the ini/default.
* Fractional values that pytest-timeout accepts (e.g., `0.5`) get truncated toward zero, so short floating timeouts or `timeout=0` (the official way to disable) become a zero-second RPC limit.
* Pytest-timeout’s default is `0` meaning “no limit”, but when neither a marker nor ini entry is present you now force a 30‑second timeout.
If the goal is to align with pytest, this needs to honour keyword arguments, keep the float precision, and treat `0`/`None` as “no timeout” instead of enforcing an immediate or 30‑second cut-off.
No description provided.