Date:   Mon, 16 Mar 2020 15:55:12 +0000
From:   "Bird, Tim" <Tim.Bird@...y.com>
To:     Kees Cook <keescook@...omium.org>, Jakub Kicinski <kuba@...nel.org>
CC:     "shuah@...nel.org" <shuah@...nel.org>,
        "luto@...capital.net" <luto@...capital.net>,
        "wad@...omium.org" <wad@...omium.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel-team@...com" <kernel-team@...com>
Subject: RE: [PATCH v2 0/4] kselftest: add fixture parameters

> -----Original Message-----
> From: Kees Cook
> 
> On Fri, Mar 13, 2020 at 05:54:57PM -0700, Jakub Kicinski wrote:
> > Note that we loose a little bit of type safety
> > without passing parameters as an explicit argument.
> > If user puts the name of the wrong fixture as argument
> > to CURRENT_FIXTURE() it will happily cast the type.
> 
> This got me to take a much closer look at things. I really didn't like
> needing to repeat the fixture name in CURRENT_FIXTURE() calls, and then
> started coming to all the same conclusions you did in your v1, that I
> just didn't quite see yet in my first review. :P
> 
> Apologies for my wishy-washy-ness on this, but here's me talking myself
> out of my earlier criticisms:
> 
> - "I want tests to be run in declaration order" In v1, this is actually
>   mostly retained: they're still in declaration order, but they're
>   grouped by fixture (which are run in declaration order). That, I think,
>   is totally fine. Someone writing code that interleaves between fixtures
>   is madness, and having the report retain that ordering seems awful. I
>   had thought the declaration ordering was entirely removed, but I see on
>   closer inspection that's not true.
> 
> - "I'd like everything attached to _metadata" This results in the
>   type unsafety you call out here. And I stared at your v2 trying to
>   find a way around it, but to get the type attached, it has to be
>   part of the __TEST_F_IMPL() glue, and that means passing it along
>   side "self", which means plumbing it as a function argument
>   everywhere.
> 
> So, again, sorry for asking to iterate on v1 instead of v2, though the
> v2 _really_ helped me see the problems better. ;)
> 
> Something I'd like for v3: instead of "parameters" can we call it
> "instances"? It provides a way to run separate instances of the same
> fixtures. Those instances have parameters (i.e. struct fields), so I'd
> prefer the "instance" naming.

Could I humbly suggest "variant" as a possible name here?
IMHO "instance" carries along some semantics related to object
oriented programming, which I think is a bit confusing.  (Maybe that's
intentional though, and you prefer that?)

BTW - Fuego has a similar feature for naming a collection of test
parameters with specific values (if I understand this proposed
feature correctly).  Fuego's feature was named a long time ago
(incorrectly, I think) and it continues to bug me to this day.
It was named 'specs', and after giving it considerable thought
I've been meaning to change it to 'variants'.

Just a suggestion for consideration.  The fact that Fuego got this
wrong is what motivates my suggestion today.  You have to live
with this kind of stuff a long time. :-)

We ran into some issues in Fuego with this concept, that motivate
the comments below.  I'll use your 'instance' terminology in my comments
although the terminology is different in Fuego.

> 
> Also a change in reporting:
> 
> 	struct __fixture_params_metadata no_param = { .name = "", };
> 
> Let's make ".name = NULL" here, and then we can detect instantiation:
> 
> 	printf("[ RUN      ] %s%s%s.%s\n", f->name, p->name ? "." : "",
> 				p->name ?: "", t->name);
> 
> That'll give us single-instance fixtures an unchanged name:
> 
> 	fixture.test1
> 	fixture.test2

We ended up in Fuego adding a 'default' instance name for 
all tests.  That way, all the parsers don't have to be coded to distinguish
if the test identifier includes an instance name or not, which turns
out to be a tough problem.

So single-instance tests would be:
            fixture.default.test1
            fixture.default.test2
> 
> and instanced fixtures will be:
> 
> 	fixture.wayA.test1
> 	fixture.wayA.test2
> 	fixture.wayB.test1
> 	fixture.wayB.test2
> 

Parsing of the test identifiers starts to become a thorny issue 
as you get longer and longer sequences of test-name parts
(test suite, test fixture, sub-test, test-case, measurement, instance, etc.)
It becomes considerably more difficult if  you have more than
one optional element in the identifier, so it's useful to
avoid any optional element you can.

> 
> And finally, since we're in the land of endless macros, I think it
> could be possible to make a macro to generate the __register_foo()
> routine bodies. By the end of the series there are three nearly identical
> functions in the harness for __register_test(), __register_fixture(), and
> __register_fixture_instance(). Something like this as an earlier patch to
> refactor the __register_test() that can be used by the latter two in their
> patches (and counting will likely need to be refactored earlier too):
> 
> #define __LIST_APPEND(head, item)				\
> {								\
> 	/* Circular linked list where only prev is circular. */	\
> 	if (head == NULL) {					\
> 		head = item;					\
> 		item->next = NULL;				\
> 		item->prev = item;				\
> 		return;						\
> 	}							\
> 	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {\
> 		item->next = NULL;				\
> 		item->prev = head->prev;			\
> 		item->prev->next = item;			\
> 		head->prev = item;				\
> 	} else {						\
> 		p->next = head;					\
> 		p->next->prev = item;				\
> 		p->prev = item;					\
> 		head = item;					\
> 	}							\
> }
> 
> Which should let it be used, ultimately, as:
> 
> static inline void __register_test(struct __test_metadata *t)
> __LIST_APPEND(__test_list, t)
> 
> static inline void __register_fixture(struct __fixture_metadata *f)
> __LIST_APPEND(__fixture_list, f)
> 
> static inline void
> __register_fixture_instance(struct __fixture_metadata *f,
> 			    struct __fixture_instance_metadata *p)
> __LIST_APPEND(f->instances, p)

With my suggestion of 'variant', this would change to:

static inline void
__register_fixture_variant(struct __fixture_metadata *f,
			    struct __fixture_variant_metadata *p)
__LIST_APPEND(f->variants, p)


Just my 2 cents.
 -- Tim

Powered by blists - more mailing lists