Messages in this thread | | | Subject | Re: [PATCH] selftests/kselftest/runner.sh: Add 45 second timeout per test | From | shuah <> | Date | Fri, 20 Sep 2019 14:24:04 -0600 |
| |
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
| |