Re: [PATCH v2 0/4] kselftest: add fixture parameters

[Kees Cook <keescook@chromium.org>]

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.

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

and instanced fixtures will be:

	fixture.wayA.test1
	fixture.wayA.test2
	fixture.wayB.test1
	fixture.wayB.test2


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)


Thanks for working on this!

-Kees

-- 
Kees Cook