Message-ID: <20200316130416.4ec9103b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 16 Mar 2020 13:04:16 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     "Bird, Tim" <Tim.Bird@...y.com>
Cc:     Kees Cook <keescook@...omium.org>,
        "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

On Mon, 16 Mar 2020 15:55:12 +0000 Bird, Tim wrote:
> > -----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

No worries, it took me a little bit of internal back and forth to
produce v1, and it's still not at all perfect :S

> > 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?)

I like parameter or argument, since the data provided to the test 
is constant, and used to guide the instantiation (i.e. "setup"). 
"self" looks more like an instance of a class from OOP point of view.

Variant sounds good too, although the abbreviation would be VAR?
Which isn't ideal. 

But I really don't care so whoever cares the most please speak up :P

> 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);

Do I have to make it NULL or is it okay to test p->name[0] ?
That way we can save one ternary operator from the litany..

> > 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

Interesting! That makes sense to me, thanks for sharing the experience.
That's why I just appended the param/instance/variant name to the
fixture name.

To me global.default.XYZ is a mouthful. so in my example (perhaps that
should have been part of the cover letter) I got:

[ RUN      ] global.keysizes             <= non-fixture test
[       OK ] global.keysizes             
[ RUN      ] tls_basic.base_base         <= fixture: "tls_basic", no params
[       OK ] tls_basic.base_base         
[ RUN      ] tls12.sendfile              <= fixture: "tls", param: "12"
[       OK ] tls12.sendfile                 
[ RUN      ] tls13.sendfile              <= fixture: "tls", param: "13"
[       OK ] tls13.sendfile                 (same fixture, diff param)

And users can start inserting underscores themselves if they really
want. (For TLS I was considering different ciphers but they don't impact
testing much.)

> > 
> > 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