Re: [RFC v1 2/6] kunit: Add speed attribute

On Sat, Jun 10, 2023 at 4:29 AM David Gow davidgow@google.com wrote:

On Sat, 10 Jun 2023 at 08:52, Rae Moar rmoar@google.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.

This is a really interesting discussion. I see your point here. I will remove the "fast" category unless there are any objections.

...

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.

This seems like a great rule of thumb to add to the documentation.

...

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.

I would be happy to add this once something uses the "very_slow" attribute.

...

Add an example of marking a slow test to kunit-example-test.c.

Signed-off-by: Rae Moar rmoar@google.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...

Oh interesting. I hadn't considered changing the order. To me "speed>slow" seems a bit more intuitive but I can see how "speed<slow" would also make sense. Hmm this is an interesting idea. Let me know if anyone has an opinion here else I will most likely keep it this order.

...

/* 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?

Yes, I will change this out.

...

/* 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.

I'm thinking not but it would make the concept clearer. I would definitely change this if it is wanted.

Thanks! -Rae

...

/*

• 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