Message-ID: <CABVgOS=4ioKpNOh-hPGEv70LSVwFh2SC0BL_dK1pWZR3pLwrtQ@mail.gmail.com>
Date:   Sat, 10 Jun 2023 16:29:16 +0800
From:   David Gow <davidgow@...gle.com>
To:     Rae Moar <rmoar@...gle.com>
Cc:     shuah@...nel.org, dlatypov@...gle.com, brendan.higgins@...ux.dev,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org, keescook@...omium.org,
        linux-hardening@...r.kernel.org, jstultz@...gle.com,
        tglx@...utronix.de, sboyd@...nel.org
Subject: Re: [RFC v1 2/6] kunit: Add speed attribute

On Sat, 10 Jun 2023 at 08:52, Rae Moar <rmoar@...gle.com> wrote:
>
> Add speed attribute to the test attribute API. This attribute will allow
> users to mark tests with a category of speed.
>
> Currently the categories of speed proposed are: fast, normal, slow, and
> very_slow. These are outlined in the enum kunit_speed. Note the speed
> attribute can also be left as unset and then, will act as the default which
> is "normal", during filtering.

Do we need both "fast" and "normal". KUnit tests are normally very
fast: I'm not sure there's a way to make them faster enough to make a
separate category here useful.

>
> Note speed is intended to be marked based on relative speeds rather than
> quantitative speeds of KUnit tests. This is because tests may run on
> various architectures at different speeds.

My rule of thumb here is that a test is slow if it takes more than a
"trivial" amount of time (<1s), regardless of the machine it's running
on.

While the actual speed taken varies a lot (the time_test_cases take ~3
seconds on most fast, modern machines, even under something like qemu,
but ~15 minutes on an old 486), it's the idea that a test is doing
some significant amount of work (loops over many thousands or millions
of entries, etc) that pretty comfortably divides these into "normal"
and "slow".

Most tests run very, very quickly on even very slow systems, as all
they're doing is checking the result of one or two trivial
calculations or functions.

> Add the macro KUNIT_CASE_SLOW to set a test as slow, as this is likely a
> common use of the attributes API.

I'd ask if we need a KUNIT_CASE_VERY_SLOW() as well, but let's leave
that until we have something which uses it.


> Add an example of marking a slow test to kunit-example-test.c.
>
> Signed-off-by: Rae Moar <rmoar@...gle.com>
> ---
>  include/kunit/test.h           | 31 ++++++++++++++++++++++-
>  lib/kunit/attributes.c         | 45 +++++++++++++++++++++++++++++++++-
>  lib/kunit/kunit-example-test.c |  9 +++++++
>  3 files changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 1fc9155988e9..3d684723ae57 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -63,8 +63,26 @@ enum kunit_status {
>         KUNIT_SKIPPED,
>  };
>
> +/* Attribute struct/enum definitions */
> +
> +/*
> + * Speed Attribute is stored as an enum and separated into categories of
> + * speed: very_slowm, slow, normal, and fast. These speeds are relative
> + * to other KUnit tests.
> + */
> +enum kunit_speed {
> +       KUNIT_SPEED_UNSET,
> +       KUNIT_SPEED_VERY_SLOW,
> +       KUNIT_SPEED_SLOW,
> +       KUNIT_SPEED_NORMAL,
> +       KUNIT_SPEED_FAST,
> +       KUNIT_SPEED_MAX = KUNIT_SPEED_FAST,
> +};

Question: Does it make sense to have these in this order: slow ->
fast? I think it does ("speed>slow" seems more correct than
"speed<slow"), but it'd be the other way round if we wanted to call
this, e.g., size instead of speed.

That being said, if it went the other way, we could rely on the fact
that the default is fast, and not need a separate "unset" default...

> +
>  /* Holds attributes for each test case and suite */
> -struct kunit_attributes {};
> +struct kunit_attributes {
> +       enum kunit_speed speed;
> +};
>
>  /**
>   * struct kunit_case - represents an individual test case.
> @@ -150,6 +168,17 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
>                 { .run_case = test_name, .name = #test_name,    \
>                   .attr = attributes }
>
> +/**
> + * KUNIT_CASE_SLOW - A helper for creating a &struct kunit_case
> + * with the slow attribute
> + *
> + * @test_name: a reference to a test case function.
> + */
> +
> +#define KUNIT_CASE_SLOW(test_name)                     \
> +               { .run_case = test_name, .name = #test_name,    \
> +                 .attr.speed = KUNIT_SPEED_SLOW }
> +
>  /**
>   * KUNIT_CASE_PARAM - A helper for creation a parameterized &struct kunit_case
>   *
> diff --git a/lib/kunit/attributes.c b/lib/kunit/attributes.c
> index 0ea641be795f..e17889f94693 100644
> --- a/lib/kunit/attributes.c
> +++ b/lib/kunit/attributes.c
> @@ -28,9 +28,52 @@ struct kunit_attr {
>         void *attr_default;
>  };
>
> +/* String Lists for enum Attributes */
> +
> +static const char * const speed_str_list[] = {"unset", "very_slow", "slow", "normal", "fast"};
> +
> +/* To String Methods */
> +
> +static const char *attr_enum_to_string(void *attr, const char * const str_list[], bool *to_free)
> +{
> +       long val = (long)attr;
> +
> +       *to_free = false;
> +       if (!val)
> +               return NULL;
> +       return str_list[val];
> +}
> +
> +static const char *attr_speed_to_string(void *attr, bool *to_free)
> +{
> +       return attr_enum_to_string(attr, speed_str_list, to_free);
> +}
> +
> +/* Get Attribute Methods */
> +
> +static void *attr_speed_get(void *test_or_suite, bool is_test)
> +{
> +       struct kunit_suite *suite = is_test ? NULL : test_or_suite;
> +       struct kunit_case *test = is_test ? test_or_suite : NULL;
> +
> +       if (test)
> +               return ((void *) test->attr.speed);
> +       else
> +               return ((void *) suite->attr.speed);
> +}
> +
> +/* Attribute Struct Definitions */
> +
> +static const struct kunit_attr speed_attr = {
> +       .name = "speed",
> +       .get_attr = attr_speed_get,
> +       .to_string = attr_speed_to_string,
> +       .attr_default = (void *)KUNIT_SPEED_NORMAL,
> +};
> +
>  /* List of all Test Attributes */
>
> -static struct kunit_attr kunit_attr_list[1] = {};
> +static struct kunit_attr kunit_attr_list[1] = {speed_attr};

Nit: Can we remove the hardcoded [1] here, and let the compiler do this for us?

>
>  /* Helper Functions to Access Attributes */
>
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index b69b689ea850..01a769f35e1d 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -220,6 +220,14 @@ static void example_params_test(struct kunit *test)
>         KUNIT_EXPECT_EQ(test, param->value % param->value, 0);
>  }
>
> +/*
> + * This test should always pass. Can be used to practice filtering attributes.
> + */
> +static void example_slow_test(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 1 + 1, 2);
> +}

Would we want to actually make this test slow? e.g. introduce a delay
or a big loop or something.
Probably not (I think it'd be more irritating than illuminating), but
maybe worth thinking of.


> +
>  /*
>   * Here we make a list of all the test cases we want to add to the test suite
>   * below.
> @@ -237,6 +245,7 @@ static struct kunit_case example_test_cases[] = {
>         KUNIT_CASE(example_all_expect_macros_test),
>         KUNIT_CASE(example_static_stub_test),
>         KUNIT_CASE_PARAM(example_params_test, example_gen_params),
> +       KUNIT_CASE_SLOW(example_slow_test),
>         {}
>  };
>
> --
> 2.41.0.162.gfafddb0af9-goog
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)

Powered by blists - more mailing lists