Hi!
This set is an attempt to make running tests for different sets of data easier. The direct motivation is the tls test which we'd like to run for TLS 1.2 and TLS 1.3, but currently there is no easy way to invoke the same tests with different parameters.
Tested all users of kselftest_harness.h.
v2: - don't run tests by fixture - don't pass params as an explicit argument
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.
Jakub Kicinski (4): selftests/seccomp: use correct FIXTURE macro kselftest: create fixture objects kselftest: add fixture parameters selftests: tls: run all tests for TLS 1.2 and TLS 1.3
Documentation/dev-tools/kselftest.rst | 3 +- tools/testing/selftests/kselftest_harness.h | 156 ++++++++++++++++-- tools/testing/selftests/net/tls.c | 93 ++--------- tools/testing/selftests/seccomp/seccomp_bpf.c | 10 +- 4 files changed, 168 insertions(+), 94 deletions(-)
Quoting kdoc:
FIXTURE_DATA: * This call may be used when the type of the fixture data * is needed. In general, this should not be needed unless * the *self* is being passed to a helper directly.
FIXTURE: * Defines the data provided to TEST_F()-defined tests as *self*. It should be * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN().
seccomp should use FIXTURE to declare types.
Signed-off-by: Jakub Kicinski kuba@kernel.org Acked-by: Kees Cook keescook@chromium.org --- Kees noted that he posted an equivalent patch already, feel free to drop this one. --- tools/testing/selftests/seccomp/seccomp_bpf.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index ee1b727ede04..7bf82fb07f67 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -909,7 +909,7 @@ TEST(ERRNO_order) EXPECT_EQ(12, errno); }
-FIXTURE_DATA(TRAP) { +FIXTURE(TRAP) { struct sock_fprog prog; };
@@ -1020,7 +1020,7 @@ TEST_F(TRAP, handler) EXPECT_NE(0, (unsigned long)sigsys->_call_addr); }
-FIXTURE_DATA(precedence) { +FIXTURE(precedence) { struct sock_fprog allow; struct sock_fprog log; struct sock_fprog trace; @@ -1509,7 +1509,7 @@ void tracer_poke(struct __test_metadata *_metadata, pid_t tracee, int status, EXPECT_EQ(0, ret); }
-FIXTURE_DATA(TRACE_poke) { +FIXTURE(TRACE_poke) { struct sock_fprog prog; pid_t tracer; long poked; @@ -1817,7 +1817,7 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee, change_syscall(_metadata, tracee, -1, -ESRCH); }
-FIXTURE_DATA(TRACE_syscall) { +FIXTURE(TRACE_syscall) { struct sock_fprog prog; pid_t tracer, mytid, mypid, parent; }; @@ -2321,7 +2321,7 @@ struct tsync_sibling { } \ } while (0)
-FIXTURE_DATA(TSYNC) { +FIXTURE(TSYNC) { struct sock_fprog root_prog, apply_prog; struct tsync_sibling sibling[TSYNC_SIBLINGS]; sem_t started;
Grouping tests by fixture will allow us to parametrize test runs. Create full objects for fixtures.
Add a "global" fixture for tests without a fixture.
Signed-off-by: Jakub Kicinski kuba@kernel.org Acked-by: Kees Cook keescook@chromium.org -- v2: - remove the fixture list, we won't iterate over fixtures so it's not needed --- tools/testing/selftests/kselftest_harness.h | 31 ++++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 5336b26506ab..66c2397d8c51 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -169,8 +169,10 @@ #define __TEST_IMPL(test_name, _signal) \ static void test_name(struct __test_metadata *_metadata); \ static struct __test_metadata _##test_name##_object = \ - { .name = "global." #test_name, \ - .fn = &test_name, .termsig = _signal, \ + { .name = #test_name, \ + .fn = &test_name, \ + .fixture = &_fixture_global, \ + .termsig = _signal, \ .timeout = TEST_TIMEOUT_DEFAULT, }; \ static void __attribute__((constructor)) _register_##test_name(void) \ { \ @@ -212,6 +214,8 @@ * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN(). */ #define FIXTURE(fixture_name) \ + static struct __fixture_metadata _##fixture_name##_fixture_object = \ + { .name = #fixture_name, }; \ static void __attribute__((constructor)) \ _register_##fixture_name##_data(void) \ { \ @@ -309,8 +313,9 @@ } \ static struct __test_metadata \ _##fixture_name##_##test_name##_object = { \ - .name = #fixture_name "." #test_name, \ + .name = #test_name, \ .fn = &wrapper_##fixture_name##_##test_name, \ + .fixture = &_##fixture_name##_fixture_object, \ .termsig = signal, \ .timeout = tmout, \ }; \ @@ -632,9 +637,12 @@ } while (0); OPTIONAL_HANDLER(_assert)
/* Contains all the information for test execution and status checking. */ +struct __fixture_metadata; + struct __test_metadata { const char *name; void (*fn)(struct __test_metadata *); + struct __fixture_metadata *fixture; int termsig; int passed; int trigger; /* extra handler after the evaluation */ @@ -685,6 +693,13 @@ static inline void __register_test(struct __test_metadata *t) } }
+/* Contains all the information about a fixture */ +struct __fixture_metadata { + const char *name; +} _fixture_global __attribute__((unused)) = { + .name = "global", +}; + static inline int __bail(int for_realz, bool no_print, __u8 step) { if (for_realz) { @@ -695,14 +710,15 @@ static inline int __bail(int for_realz, bool no_print, __u8 step) return 0; }
-void __run_test(struct __test_metadata *t) +void __run_test(struct __fixture_metadata *f, + struct __test_metadata *t) { pid_t child_pid; int status;
t->passed = 1; t->trigger = 0; - printf("[ RUN ] %s\n", t->name); + printf("[ RUN ] %s.%s\n", f->name, t->name); alarm(t->timeout); child_pid = fork(); if (child_pid < 0) { @@ -751,7 +767,8 @@ void __run_test(struct __test_metadata *t) status); } } - printf("[ %4s ] %s\n", (t->passed ? "OK" : "FAIL"), t->name); + printf("[ %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"), + f->name, t->name); alarm(0); }
@@ -768,7 +785,7 @@ static int test_harness_run(int __attribute__((unused)) argc, __test_count, __fixture_count + 1); for (t = __test_list; t; t = t->next) { count++; - __run_test(t); + __run_test(t->fixture, t); if (t->passed) pass_count++; else
Allow users to pass parameters to fixtures.
Each test will be run once for each set of its fixture parameter sets (or once if none).
Signed-off-by: Jakub Kicinski kuba@kernel.org -- v2: - don't pass params to functions, use a member of _metadata instead --- Documentation/dev-tools/kselftest.rst | 3 +- tools/testing/selftests/kselftest_harness.h | 133 ++++++++++++++++++-- 2 files changed, 126 insertions(+), 10 deletions(-)
diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst index 61ae13c44f91..8aff58d11937 100644 --- a/Documentation/dev-tools/kselftest.rst +++ b/Documentation/dev-tools/kselftest.rst @@ -301,7 +301,8 @@ Helpers
.. kernel-doc:: tools/testing/selftests/kselftest_harness.h :functions: TH_LOG TEST TEST_SIGNAL FIXTURE FIXTURE_DATA FIXTURE_SETUP - FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN + FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN FIXTURE_PARAMS + FIXTURE_PARAMS_ADD CURRENT_PARAMS
Operators --------- diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 66c2397d8c51..b7e1ecda441c 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -214,6 +214,7 @@ * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN(). */ #define FIXTURE(fixture_name) \ + FIXTURE_PARAMS(fixture_name); \ static struct __fixture_metadata _##fixture_name##_fixture_object = \ { .name = #fixture_name, }; \ static void __attribute__((constructor)) \ @@ -246,6 +247,7 @@ void fixture_name##_setup( \ struct __test_metadata __attribute__((unused)) *_metadata, \ FIXTURE_DATA(fixture_name) __attribute__((unused)) *self) + /** * FIXTURE_TEARDOWN(fixture_name) * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly. @@ -267,6 +269,72 @@ struct __test_metadata __attribute__((unused)) *_metadata, \ FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
+/** + * FIXTURE_PARAMS(fixture_name) - Optionally called once per fixture + * to declare fixture parameters + * + * @fixture_name: fixture name + * + * .. code-block:: c + * + * FIXTURE_PARAMS(datatype name) { + * type property1; + * ... + * }; + * + * Defines type of constant parameters provided to FIXTURE_SETUP() and TEST_F() + * as *params*. + */ +#define FIXTURE_PARAMS(fixture_name) struct _fixture_params_##fixture_name + +/** + * FIXTURE_PARAMS_ADD(fixture_name, params_name) - Called once per fixture + * params to setup the data and register + * + * @fixture_name: fixture name + * @params_name: name of the parameter set + * + * .. code-block:: c + * + * FIXTURE_ADD(datatype name) { + * .property1 = val1; + * ... + * }; + * + * Defines an instance of parameters accessible in FIXTURE_SETUP(), + * FIXTURE_TEARDOWN() and TEST_F(). Tests will be run once for each + * parameter set. + */ +#define FIXTURE_PARAMS_ADD(fixture_name, params_name) \ + extern FIXTURE_PARAMS(fixture_name) \ + _##fixture_name##_##params_name##_params; \ + static struct __fixture_params_metadata \ + _##fixture_name##_##params_name##_object = \ + { .name = #params_name, \ + .data = &_##fixture_name##_##params_name##_params}; \ + static void __attribute__((constructor)) \ + _register_##fixture_name##_##params_name(void) \ + { \ + __register_fixture_params(&_##fixture_name##_fixture_object, \ + &_##fixture_name##_##params_name##_object); \ + } \ + FIXTURE_PARAMS(fixture_name) _##fixture_name##_##params_name##_params = + +/** + * CURRENT_PARAMS(fixture_name) - Access fixture parameters of the current test + * + * @fixture_name: fixture name + * + * .. code-block:: c + * + * CURRENT_PARAMS(fixture name)->property1 + * + * Helper macro for accessing parameters of current test. Can be used inside + * FIXTURE_SETUP(), FIXTURE_TEARDOWN() and TEST_F(). + */ +#define CURRENT_PARAMS(fixture_name) \ + ((const FIXTURE_PARAMS(fixture_name) *) _metadata->current_params->data) + /** * TEST_F(fixture_name, test_name) - Emits test registration and helpers for * fixture-based test cases @@ -638,11 +706,13 @@
/* Contains all the information for test execution and status checking. */ struct __fixture_metadata; +struct __fixture_params_metadata;
struct __test_metadata { const char *name; void (*fn)(struct __test_metadata *); struct __fixture_metadata *fixture; + struct __fixture_params_metadata *current_params; int termsig; int passed; int trigger; /* extra handler after the evaluation */ @@ -696,10 +766,41 @@ static inline void __register_test(struct __test_metadata *t) /* Contains all the information about a fixture */ struct __fixture_metadata { const char *name; + struct __fixture_params_metadata *params; } _fixture_global __attribute__((unused)) = { .name = "global", };
+struct __fixture_params_metadata { + const char *name; + const void *data; + struct __fixture_params_metadata *prev, *next; +}; + +static inline void +__register_fixture_params(struct __fixture_metadata *f, + struct __fixture_params_metadata *p) +{ + /* Circular linked list where only prev is circular. */ + if (f->params == NULL) { + f->params = p; + p->next = NULL; + p->prev = p; + return; + } + if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) { + p->next = NULL; + p->prev = f->params->prev; + p->prev->next = p; + f->params->prev = p; + } else { + p->next = f->params; + p->next->prev = p; + p->prev = p; + f->params = p; + } +} + static inline int __bail(int for_realz, bool no_print, __u8 step) { if (for_realz) { @@ -711,14 +812,21 @@ static inline int __bail(int for_realz, bool no_print, __u8 step) }
void __run_test(struct __fixture_metadata *f, + struct __fixture_params_metadata *p, struct __test_metadata *t) { pid_t child_pid; int status;
+ /* reset test struct */ t->passed = 1; t->trigger = 0; - printf("[ RUN ] %s.%s\n", f->name, t->name); + t->step = 0; + t->no_print = 0; + + t->current_params = p; + + printf("[ RUN ] %s%s.%s\n", f->name, p->name, t->name); alarm(t->timeout); child_pid = fork(); if (child_pid < 0) { @@ -767,14 +875,17 @@ void __run_test(struct __fixture_metadata *f, status); } } - printf("[ %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"), - f->name, t->name); + printf("[ %4s ] %s%s.%s\n", (t->passed ? "OK" : "FAIL"), + f->name, p->name, t->name); alarm(0); }
static int test_harness_run(int __attribute__((unused)) argc, char __attribute__((unused)) **argv) { + struct __fixture_params_metadata no_param = { .name = "", }; + struct __fixture_params_metadata *p; + struct __fixture_metadata *f; struct __test_metadata *t; int ret = 0; unsigned int count = 0; @@ -784,12 +895,16 @@ static int test_harness_run(int __attribute__((unused)) argc, printf("[==========] Running %u tests from %u test cases.\n", __test_count, __fixture_count + 1); for (t = __test_list; t; t = t->next) { - count++; - __run_test(t->fixture, t); - if (t->passed) - pass_count++; - else - ret = 1; + f = t->fixture; + + for (p = f->params ?: &no_param; p; p = p->next) { + count++; + __run_test(f, p, t); + if (t->passed) + pass_count++; + else + ret = 1; + } } printf("[==========] %u / %u tests passed.\n", pass_count, count); printf("[ %s ]\n", (ret ? "FAILED" : "PASSED"));
TLS 1.2 and TLS 1.3 differ in the implementation. Use fixture parameters to run all tests for both versions, and remove the one-off TLS 1.2 test.
Signed-off-by: Jakub Kicinski kuba@kernel.org Reviewed-by: Kees Cook keescook@chromium.org --- tools/testing/selftests/net/tls.c | 93 ++++++------------------------- 1 file changed, 17 insertions(+), 76 deletions(-)
diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c index 0ea44d975b6c..82d48ce0787e 100644 --- a/tools/testing/selftests/net/tls.c +++ b/tools/testing/selftests/net/tls.c @@ -101,6 +101,21 @@ FIXTURE(tls) bool notls; };
+FIXTURE_PARAMS(tls) +{ + unsigned int tls_version; +}; + +FIXTURE_PARAMS_ADD(tls, 12) +{ + .tls_version = TLS_1_2_VERSION, +}; + +FIXTURE_PARAMS_ADD(tls, 13) +{ + .tls_version = TLS_1_3_VERSION, +}; + FIXTURE_SETUP(tls) { struct tls12_crypto_info_aes_gcm_128 tls12; @@ -112,7 +127,7 @@ FIXTURE_SETUP(tls) len = sizeof(addr);
memset(&tls12, 0, sizeof(tls12)); - tls12.info.version = TLS_1_3_VERSION; + tls12.info.version = CURRENT_PARAMS(tls)->tls_version; tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
addr.sin_family = AF_INET; @@ -733,7 +748,7 @@ TEST_F(tls, bidir) struct tls12_crypto_info_aes_gcm_128 tls12;
memset(&tls12, 0, sizeof(tls12)); - tls12.info.version = TLS_1_3_VERSION; + tls12.info.version = CURRENT_PARAMS(tls)->tls_version; tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
ret = setsockopt(self->fd, SOL_TLS, TLS_RX, &tls12, @@ -1258,78 +1273,4 @@ TEST(keysizes) { close(cfd); }
-TEST(tls12) { - int fd, cfd; - bool notls; - - struct tls12_crypto_info_aes_gcm_128 tls12; - struct sockaddr_in addr; - socklen_t len; - int sfd, ret; - - notls = false; - len = sizeof(addr); - - memset(&tls12, 0, sizeof(tls12)); - tls12.info.version = TLS_1_2_VERSION; - tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128; - - addr.sin_family = AF_INET; - addr.sin_addr.s_addr = htonl(INADDR_ANY); - addr.sin_port = 0; - - fd = socket(AF_INET, SOCK_STREAM, 0); - sfd = socket(AF_INET, SOCK_STREAM, 0); - - ret = bind(sfd, &addr, sizeof(addr)); - ASSERT_EQ(ret, 0); - ret = listen(sfd, 10); - ASSERT_EQ(ret, 0); - - ret = getsockname(sfd, &addr, &len); - ASSERT_EQ(ret, 0); - - ret = connect(fd, &addr, sizeof(addr)); - ASSERT_EQ(ret, 0); - - ret = setsockopt(fd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls")); - if (ret != 0) { - notls = true; - printf("Failure setting TCP_ULP, testing without tls\n"); - } - - if (!notls) { - ret = setsockopt(fd, SOL_TLS, TLS_TX, &tls12, - sizeof(tls12)); - ASSERT_EQ(ret, 0); - } - - cfd = accept(sfd, &addr, &len); - ASSERT_GE(cfd, 0); - - if (!notls) { - ret = setsockopt(cfd, IPPROTO_TCP, TCP_ULP, "tls", - sizeof("tls")); - ASSERT_EQ(ret, 0); - - ret = setsockopt(cfd, SOL_TLS, TLS_RX, &tls12, - sizeof(tls12)); - ASSERT_EQ(ret, 0); - } - - close(sfd); - - char const *test_str = "test_read"; - int send_len = 10; - char buf[10]; - - send_len = strlen(test_str) + 1; - EXPECT_EQ(send(fd, test_str, send_len, 0), send_len); - EXPECT_NE(recv(cfd, buf, send_len, 0), -1); - EXPECT_EQ(memcmp(buf, test_str, send_len), 0); - - close(fd); - close(cfd); -} - TEST_HARNESS_MAIN
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
-----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
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
On Mon, Mar 16, 2020 at 01:04:16PM -0700, Jakub Kicinski wrote:
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
Let's go with "variant" and just spell it out.
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..
I did consider Tim's idea of having them all say 'default', but since the bulk of tests aren't going to have variants, I don't want to spam the report with words I have to skip over.
And empty-check (instead of NULL) is fine by me.
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.)
The reason I'd like a dot is just for lay-person grep-ability and to avoid everyone needing to remember to add separator prefixes -- there should just be a common one. e.g. searching for "tls13" in the tree wouldn't find the test (since it's actually named "tls" and "13" is separate places). (I mean, sure, searching for "tls" is also insane, but I think I made my point.)
-Kees
On Mon, 16 Mar 2020 14:01:33 -0700 Kees Cook wrote:
On Mon, Mar 16, 2020 at 01:04:16PM -0700, Jakub Kicinski wrote:
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
Let's go with "variant" and just spell it out.
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..
I did consider Tim's idea of having them all say 'default', but since the bulk of tests aren't going to have variants, I don't want to spam the report with words I have to skip over.
And empty-check (instead of NULL) is fine by me.
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.)
The reason I'd like a dot is just for lay-person grep-ability and to avoid everyone needing to remember to add separator prefixes -- there should just be a common one. e.g. searching for "tls13" in the tree wouldn't find the test (since it's actually named "tls" and "13" is separate places). (I mean, sure, searching for "tls" is also insane, but I think I made my point.)
Ack, can't argue with grep-ability :)
From: Jakub Kicinski kuba@kernel.org Date: Fri, 13 Mar 2020 17:54:57 -0700
This set is an attempt to make running tests for different sets of data easier. The direct motivation is the tls test which we'd like to run for TLS 1.2 and TLS 1.3, but currently there is no easy way to invoke the same tests with different parameters.
Tested all users of kselftest_harness.h.
v2:
- don't run tests by fixture
- don't pass params as an explicit argument
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.
Hmmm, what tree should integrate this patch series?
On Sun, Mar 15, 2020 at 12:05:17AM -0700, David Miller wrote:
From: Jakub Kicinski kuba@kernel.org Date: Fri, 13 Mar 2020 17:54:57 -0700
This set is an attempt to make running tests for different sets of data easier. The direct motivation is the tls test which we'd like to run for TLS 1.2 and TLS 1.3, but currently there is no easy way to invoke the same tests with different parameters.
Tested all users of kselftest_harness.h.
v2:
- don't run tests by fixture
- don't pass params as an explicit argument
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.
Hmmm, what tree should integrate this patch series?
I expect the final version (likely v3) to go via Shuah's selftest tree.
-Kees
linux-kselftest-mirror@lists.linaro.org
-
Bird, Tim -
David Miller -
Jakub Kicinski -
Kees Cook