Allow @testitem to set its own timeout
#129
Conversation
src/ReTestItems.jl
Outdated
| @@ -428,7 +431,7 @@ end | |||
|
|
|||
| any_non_pass(ts::DefaultTestSet) = ts.anynonpass | |||
|
|
|||
| function record_timeout!(testitem, run_number::Int, timeout_limit::Real) | |||
| function record_timeout!(testitem, run_number::Int, timeout_limit::Float64) | |||
| timeout_s = round(Int, timeout_limit) | |||
There was a problem hiding this comment.
why the conversion to a float then to an Int? can we just stay in Int?
There was a problem hiding this comment.
yah, this is a hassle. it's just because we were lax before and so were allowing users to pass any Real, so in this PR i converted them to Float64 asap, so we at least know the type we have... but i think going the other way and rounding them to Int is the better idea (we still need to accept Floats to avoid a breaking change, as you say)
src/ReTestItems.jl
Outdated
| # TODO: should the testitem's own timeout take precedence or should we just take the `max`? | ||
| # timeout = max(something(testitem.timeout, 0.0), default_timeout) | ||
| timeout = something(testitem.timeout, default_timeout) |
There was a problem hiding this comment.
user should definitely take precedence. It can be useful to set a lower bound here so that for tests that should go fast but are at risk of blowup, you can set a lower timeout than the default so those timeouts can catch regressions more often
|
|
||
| If a `@testitem` should be aborted after a certain period of time, e.g. the test is known | ||
| to occassionally hang, then you can set a timeout (in seconds) by passing the `timeout` keyword. | ||
| Note that `timeout` currently only works when tests are run with multiple workers. |
There was a problem hiding this comment.
why is that? (out of curiosity)
There was a problem hiding this comment.
Just not implemented yet #87
We effectively largely have two separate code paths, depending on whether or not there are workers.
If there are workers, we spawn a task to run the test and a task to run a timer, and they race.
But we haven't yet implemented the equivalent for the single-process path, and that path currently doesn't spawn any tasks, it just evals code sequentially (but it could spawn tasks, maybe even using something like try_with_timeout)
I think just because initial development was for the CI use-case, for which we expect workers to be used, and the single-process case has got less love (for similar reasons, the "eager" logs case also hasn't recieved as much love #74)
There was a problem hiding this comment.
I think to robustly cancel a task on the same process, we'd need something like JuliaLang/julia#6283
src/ReTestItems.jl
Outdated
| If a `@testitem` sets its own `timeout` keyword, then that takes precedence. | ||
| Note timeouts are currently only applied when `nworkers > 0`. |
There was a problem hiding this comment.
I'd vote to limit this to int64 positive seconds to avoid conversions and general need to handle various number types. I know that's technically breaking, so take it with a bit of a grain of salt, but if we're storing up breaking changes to a release someday, something to consider
@testitems to set timeout limit #13@testitemcan set its own timeout (in seconds) via thetimeoutkeyword, like@testitem "foo" timeout=600testitem_timeoutpassed toruntests(...)(i.e. this PR makestestitem_timeoutbe a "default" timeout value that can be overridden per testitem.)maxof the two values, which is what we do forretries(so that inconsistency is perhaps a little unfortunate).@testitem's owntimeout=...value precedence allows a@testitemto set a lower limit than the default, which is useful for tests that should go fast but are at risk of blowup.