lkml.org 
[lkml]   [2023]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] kunit: Warn if tests are slow
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@kernel.org> 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@kernel.org>
> >
>
> 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@linux.dev>
> > To: David Gow <davidgow@google.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Rae Moar <rmoar@google.com>
> > Cc: linux-kselftest@vger.kernel.org
> > Cc: kunit-dev@googlegroups.com
> > Cc: linux-kernel@vger.kernel.org
> >
> > 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@kernel.org/
> > ---
> > 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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2023-10-25 11:18    [W:0.086 / U:2.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site