Hi Rae, On Tue, Oct 24, 2023 at 03:41:33PM -0400, Rae Moar wrote: > On Wed, Sep 20, 2023 at 4:49 AM Maxime Ripard <mripard@xxxxxxxxxx> 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 <mripard@xxxxxxxxxx> > > > > Hello! > > Thanks for following up! Sorry for the delay in this response. np, I kind of forgot about it too to be fair :) > 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. Yeah, I agree there > > To: Brendan Higgins <brendan.higgins@xxxxxxxxx> > > To: David Gow <davidgow@xxxxxxxxxx> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > > Cc: Rae Moar <rmoar@xxxxxxxxxx> > > Cc: linux-kselftest@xxxxxxxxxxxxxxx > > Cc: kunit-dev@xxxxxxxxxxxxxxxx > > Cc: linux-kernel@xxxxxxxxxxxxxxx > > > > 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/20230911-kms-slow-tests-v1-0-d3800a69a1a1@xxxxxxxxxx/ > > --- > > 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. Right. So I kind of disagree here. To me, the define should match the definition we have for a slow test. We chose to report it only if it exceeds it by a margin, but that's a separate thing from the actual threshold. I guess I could add a new version to make that distinction clearer. Would that work for you? Maxime
Attachment:
signature.asc
Description: PGP signature
