fix: accept string timeout values in pyproject.toml - #205
Conversation
PR pytest-dev#200 registered the timeout ini option as type='float', which rejects string values like timeout = '20.0' in pyproject.toml. Users must now choose a config format compatible with only one pytest-timeout version: 2.4.0 requires strings, 2.5.0 requires floats, and no value works for both. Remove the explicit type so the ini parsing accepts both string and numeric representations. _validate_timeout already calls float(timeout) on the raw value and handles both forms correctly. Fixes pytest-dev#203 Co-authored-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com>
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
Thanks for addressing the compatibility regression. The current change accepts quoted values, but it removes numeric parsing and the tests mask that configuration error. With pytest 9.1.1 on head 73e74c29ab5f6cb696ffea29e7e9ad8303a10c67, the unquoted case exits 3 during configuration while the quoted case reaches the expected timeout. Until both representations have a valid supported parsing path and the tests distinguish configuration failures from timeouts, #203 remains unresolved.
| help=SESSION_TIMEOUT_DESC, | ||
| ) | ||
| parser.addini("timeout", TIMEOUT_DESC, type="float") | ||
| parser.addini("timeout", TIMEOUT_DESC) |
There was a problem hiding this comment.
This restores string parsing but drops the numeric TOML behavior that #200 added. On this head with pytest 9.1.1, unquoted timeout = 1 and session_timeout = 60 exit 3 during configuration with TypeError: config option 'timeout' expects a string, got int: 1. The quoted form reaches the timeout path. This still leaves users choosing one representation by version and does not satisfy #203.
There was a problem hiding this comment.
Addressed in 4c9d604. Instead of dropping type entirely, I kept the default string type and added _get_ini_value() which catches TypeError from config.getini() (raised on pytest 9+ for non-string TOML values) and falls back to the raw inicfg value. So both timeout = "1" and timeout = 1 work.
| """ | ||
| ) | ||
| result = pytester.runpytest_subprocess() | ||
| result.stdout.no_fnmatch_line("INTERNALERROR*") |
There was a problem hiding this comment.
This assertion can pass on the configuration error above: pytest writes INTERNALERROR> to stderr, while this only inspects stdout, and both the expected timeout and a configuration failure have nonzero return codes. I ran both regression cases on this head; both test functions passed even though the unquoted case exited 3 before collection. Please assert that the subprocess reached a real test outcome, or explicitly validate stderr and the timeout output.
There was a problem hiding this comment.
Addressed in 4c9d604. The test now asserts MATCH_FAILURE_MESSAGE ("Timeout (>1.0s) from pytest-timeout.") in stdout and assert_outcomes(failed=1) instead of the weak stdout-only INTERNALERROR check. Also parametrized over both quoted-string and unquoted-int forms.
On pytest 9+, registering an ini option without an explicit type (defaulting to "string") rejects non-string TOML values, while type="float" rejects quoted strings. Both representations need to work so users don't have to choose a config format by version. Introduce _get_ini_value() which reads via config.getini() but catches TypeError and falls back to the raw config dict, then use it for both and ini reads. The regression test now covers both (quoted string) and (unquoted int), and asserts a real timeout outcome via the failure message and assert_outcomes instead of a stdout-only INTERNALERROR check that masked the configuration error. Closes pytest-dev#203
|
Thanks for the thorough review. I've addressed all three concerns in 4c9d604. Code change (review discussion): Instead of dropping
All paths reach Test fix (r3861232832): The test now uses:
This replaces the old Coverage: The test is now parametrized over both |
|
Hey — just a heads up that 4c9d604 addresses both points from your review:
Would appreciate another look when you get a chance. |
Type of Changes
Description
PR #200 registered the
timeoutandsession_timeoutini options astype="float", which rejects string values liketimeout = "20.0"in pyproject.toml. Users must now choose a config format compatible with only onepytest-timeoutversion: 2.4.0 requires strings, 2.5.0 requires floats, and no value works for both.Remove the explicit type so the ini parsing accepts both string and numeric representations.
_validate_timeoutalready callsfloat(timeout)on the raw value and handles both forms correctly.Closes #203