cli: add --default-timeout option for tests with no explicit timeout#596
Conversation
MrAnno
left a comment
There was a problem hiding this comment.
Thank you for the PR and the meticulous work :)
Timeout resolution order
It sounds perfect, it's also backward-compatible.
Code style notes
Thanks again for your precision, it looks perfect to me.
Yeah, the format Uncrustify wants to produce is far from what we can see on bleeding.
Location of the new tests
Timing-based tests aren't the most warmly welcomed creatures in test suites.
sample / timeout.c is already in the test suite, but it doesn't validate much, it's more like a smoke test.
I think the location of default-timeout.c is right. What's worth considering is how many times we want to run these tests and for what purpose.
default-timeout currently runs three times: once as part of the "full" test suite, and twice again as cram-validated tests.
I think it would be worth not registering default-timeout as a "full" test and letting cram validate it instead. Or if you prefer keeping it in both suites, we can remove the timeout sample from the "full" test suite instead (running 2 --always-succeed smoke tests around timeouts is not really useful).
Help message clarity for --timeout vs --default-timeout
I like your suggestion, it clarifies everything discussed in #279. Would you like to add it to this PR?
|
Thank you for the review and the kind words!
I'll go with this option, removing the
Yes, happy to include the updated help strings in this PR. I'll update both |
|
I just made the change you suggested, and switched the order in which the |
This PR introduces a new
--default-timeoutCLI option (and its correspondingcriterion_options.default_timeoutfield) that applies a fallback timeout to any test that has no timeout set, neither directly on the test nor inherited from its suite.The existing
--timeoutoption acts as a hard cap: it overrides every test's timeout downward if the test's own timeout exceeds the cap.--default-timeoutfills the complementary gap: it only kicks in when no timeout would otherwise apply, leaving explicitly-timed tests completely untouched.This would also somewhat resolve #279
Timeout resolution order (after this PR)
.timeout(overrides suite)--default-timeoutapplied only when the result of steps 1–2 is<= 0--timeoutcap applied only when the result so far exceeds the capCode style notes
Uncrustify was run over the touched files. Two issues came up:
Pre-existing errors in untouched files. Uncrustify reported style violations in files not modified by this PR. Those were left alone to keep the diff focused and avoid muddying the review with unrelated formatting churn.
Backslash alignment in the
USAGEmacro (src/entry/params.c). Uncrustify suggested de-aligning the continuation backslashes added for the new--default-timeoutline. The rest of the macro uses aligned backslashes as an intentional style choice, so the new lines were written to match the surrounding code rather than follow the tool's suggestion.Open questions / uncertainties
Location of the new tests
The cram test (
test/cram/timeout.t) referencesdefault-timeout.c.binby name, which is only built when the full test suite is configured. It is unclear whether this file belongs undertest/cram/(which typically contains self-contained script tests) or whether it would be better placed alongside the full-test fixtures it depends on, or driven fromtest/full/meson.builddirectly.Help message clarity for
--timeoutvs--default-timeoutThe current help strings are:
These may not make the semantic difference obvious enough to end users, since "for all tests" could be read as "applies universally" without conveying that it acts as a cap. A clearer phrasing might be something like: