Messages in this thread | | | Date | Wed, 25 Oct 2023 11:17:19 +0200 | From | Maxime Ripard <> | Subject | Re: [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] | |