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