On 9/19/19 3:17 PM, Kees Cook wrote:
On Thu, Sep 19, 2019 at 02:09:37PM -0600, shuah wrote:
On 9/19/19 12:55 PM, Alexandre Belloni wrote:
On 19/09/2019 11:06:44-0700, Kees Cook wrote:
Commit a745f7af3cbd ("selftests/harness: Add 30 second timeout per
test") solves the problem of kselftest_harness.h-using binary tests
possibly hanging forever. However, scripts and other binaries can still
hang forever. This adds a global timeout to each test script run.
Timeout is good, but really tests should not hang. So we have to somehow
indicate that the test needs to be fixed.
Totally agreed, which is why I changed the reporting to call out a
"TIMEOUT" instead of just having it enter the general failure noise.
This timeout is a band-aid and not real solution for the problem. This
arbitrary value doesn't take into account that the test(s) in that
particular directory (TARGET) could be running normally and working
through all the tests.
Even something that looks like it's making progress may still be hung or
won't finish in a reasonable amount of time.
We need some way to differentiate the two cases.
I don't think it's unreasonable to declare that no test should take
longer than some default amount of time that can be tweaked via a
"settings" file. It gives the framework the option of easily removing
tests that take "too long", etc. If the "timeout=..." value was made
mandatory for each test directory, then the framework could actually
filter based on expected worst-case run time.
To make this configurable (e.g. as needed in the "rtc" test case),
include a new per-test-directory "settings" file (similar to "config")
that can contain kselftest-specific settings. The first recognized field
is "timeout".
Seems good to me. I was also wondering whether this is actually
reasonable to have tests running for so long. I wanted to discuss that
at LPC but I missed the session.
There is the individual test times and overall kselftest run time. We
have lots of tests now and it does take long.
This patch seeks to implement a "timeout for a single test from
kselftest's perspective". Some "individual" tests have many subtests
(e.g. anything built with kselftest_harness.h) giving us the whole
subtest issue. I think my solution here is a good middle ground: we
specify the max run time for each executed test binary/script.
It's not clear to me if a v2 is needed? Is this patch fine as-is?
Thanks!
v1 is good. I will pull this in for testing. I do like the way it is
done.
thanks,
-- Shuah
