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