On Wed, Sep 20, 2023 at 4:49 AM Maxime Ripard <[email protected]> wrote: > > Kunit recently gained support to setup attributes, the first one being > the speed of a given test, then allowing to filter out slow tests. > > A slow test is defined in the documentation as taking more than one > second. There's an another speed attribute called "super slow" but whose > definition is less clear. > > Add support to the test runner to check the test execution time, and > report tests that should be marked as slow but aren't. > > Signed-off-by: Maxime Ripard <[email protected]> >
Hello! Thanks for following up! Sorry for the delay in this response. This looks great to me. I do have one comment below regarding the KUNIT_SPEED_SLOW_THRESHOLD_S macro but other than that I would be happy with this patch. This patch does bring up the question of how to handle KUnit warnings as mentioned before. But I am happy to approach that in a future patch. And I do still have concerns with this being annoying for those on slower architectures but again that would depend on how we deal with KUnit warnings. Thanks! -Rae > --- > > To: Brendan Higgins <[email protected]> > To: David Gow <[email protected]> > Cc: Jani Nikula <[email protected]> > Cc: Rae Moar <[email protected]> > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > > Changes from v1: > - Split the patch out of the series > - Change to trigger the warning only if the runtime is twice the > threshold (Jani, Rae) > - Split the speed check into a separate function (Rae) > - Link: > https://lore.kernel.org/all/[email protected]/ > --- > lib/kunit/test.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 49698a168437..a1d5dd2bf87d 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -372,6 +372,25 @@ void kunit_init_test(struct kunit *test, const char > *name, char *log) > } > EXPORT_SYMBOL_GPL(kunit_init_test); > > +#define KUNIT_SPEED_SLOW_THRESHOLD_S 1 > + > +static void kunit_run_case_check_speed(struct kunit *test, > + struct kunit_case *test_case, > + struct timespec64 duration) > +{ > + enum kunit_speed speed = test_case->attr.speed; > + > + if (duration.tv_sec < (2 * KUNIT_SPEED_SLOW_THRESHOLD_S)) I think I would prefer that KUNIT_SPEED_SLOW_THRESHOLD_S is instead set to 2 rather than using 2 as the multiplier. I realize the actual threshold for the attributes is 1 sec but for the practical use of this warning it is 2 sec. Also I would still be open to this being 1 sec depending on others opinions. David, what are your thoughts on this? > + return; > + > + if (speed == KUNIT_SPEED_VERY_SLOW || speed == KUNIT_SPEED_SLOW) > + return; > + > + kunit_warn(test, > + "Test should be marked slow (runtime: %lld.%09lds)", > + duration.tv_sec, duration.tv_nsec); > +} > + > /* > * Initializes and runs test case. Does not clean up or do post validations. > */ > @@ -379,6 +398,8 @@ static void kunit_run_case_internal(struct kunit *test, > struct kunit_suite *suite, > struct kunit_case *test_case) > { > + struct timespec64 start, end; > + > if (suite->init) { > int ret; > > @@ -390,7 +411,13 @@ static void kunit_run_case_internal(struct kunit *test, > } > } > > + ktime_get_ts64(&start); > + > test_case->run_case(test); > + > + ktime_get_ts64(&end); > + > + kunit_run_case_check_speed(test, test_case, timespec64_sub(end, > start)); > } > > static void kunit_case_internal_cleanup(struct kunit *test) > -- > 2.41.0 >
