WIP: add a "resilient" option to run all tests unconditionally #23589
Conversation
base/interactiveutil.jl
Outdated
|
|
||
| Run the Julia unit tests listed in `tests`, which can be either a string or an array of | ||
| strings, using `numcores` processors. (not exported) | ||
| strings, using `numcores` processors. | ||
| If one test fails, all remaining tests will be discarded, unless resilient is `true`. |
There was a problem hiding this comment.
Maybe state here explicitly that if resilient = true all remaining tests in other files will be run?
test/runtests.jl
Outdated
| @@ -59,14 +60,18 @@ cd(dirname(@__FILE__)) do | |||
| resp = [e] | |||
| end | |||
| push!(results, (test, resp)) | |||
| if (isa(resp[end], Integer) && (resp[end] > max_worker_rss)) || isa(resp, Exception) | |||
| if resp[1] isa Exception | |||
There was a problem hiding this comment.
sorry if this is stupid but why can't this be if resp[1] isa Exception && !resilient? Since the next condition is an elseif anyway?
There was a problem hiding this comment.
No problem! If resp[1] isa Exception but resilient == true, then the condition in the elseif would be tested, which is invalid because then resp[end] == res[1] is an exception object which can't be compared as an Int.
|
Thanks @kshyatt for reviewing! |
|
@yuyichao what do you think of having "early-exit-on-failure" for CI? |
|
I think this behavior (quit early) is useful for local testing. I don't really have an opinion which one is better for CI. |
|
OK thanks. So I realize now that I can change the "runtests.jl" options in ".yml" files, except that I don't see one for "julia freebsd ci" (@ararslan any idea?). So I will keep this open a couple more days, and if no one makes a call, I will keep as default "early-exit" for local testing, and add the "--resilient" option for CI (status quo). I see one potential advantage of "early-exit" on CI: if there is one failure, free the resources for other jobs. |
|
The FreeBSD CI bot is managed externally by @iblis17 rather than being locally configured via YAML files. Typically we just bug Iblis for FreeBSD configuration changes as needed. |
Previously, this was the default: even if one test fails, all remaining tests are run nonetheless. Now this requires to pass the "--resilient" option to "test/runtests.jl" (or `resilient=true` to the `runtest` function); the new default is to stop running tests as soon as possible when there is one failure.
rfourquet
force-pushed
the
rf/runtests-resilient
branch
from
3873048
to
b055f0b
Compare
|
@tkelman would you mind clarifying to what exactly you reacted with a downvote? Having "early-exit" by default locally, or on CI? |
|
Neither. Not doing early exit was one of the major design goals of revising the test system and adding testsets in the first place. It can be an option but I don't think it should be default anywhere. Having to keep rerunning all tests to identify all the cases a change breaks isn't a good default. |
|
Ok I will change back the default then. Any suggestion for the name of the option? "quitearly", "shortcircuit", "fastexit"? |
|
|
|
So "exit-on-error" be it! It's nicely descriptive, if a bit verbose (so I added non-documented "--eoe" alias). One advantage of the earlier version is that there was a message proposing to use "--resilient" when some tests were skipped, making it more discoverable. But I'm fine with the new version, and will merge when CI turns green. |
test/choosetests.jl
Outdated
|
|
||
| for (i, t) in enumerate(choices) | ||
| if t == "--skip" | ||
| skip_tests = choices[i + 1:end] | ||
| break | ||
| elseif t ∈ ["--exit-on-error", "--eoe"] |
There was a problem hiding this comment.
Please, let's drop the --eoe short version.
|
Is this good to go now @rfourquet? |
On my end it is! |
Previously, this was the default: even if one test fails, all
remaining tests are run nonetheless.
Now this requires to pass the "--resilient" option to "test/runtests.jl"
(or
resilient=trueto theruntestfunction); the new defaultis to stop running tests as soon as possible when there is one failure.
The default was changed because of this comment to which I agree:
But I don't know what the default should be for CIs nor how to pass them this new option.
I tried to even interrupt currently running tests with
interrupt, but then I couldn't find a way to handle the exception gracefully (maybe JuliaLang/Distributed.jl#34?); but it seems good enough to just not run new tests after a failure.DO NOT MERGE: this is based off of #23576 which should be merged first.