| Message ID | 20231115-nolibc-harness-v1-3-4d61382d9bf3@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/nolibc: introduce new test harness | expand |
On Wed, Nov 15, 2023 at 10:08:21PM +0100, Thomas Weißschuh wrote: > Migrate part of nolibc-test.c to the new test harness. > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++----------------- > 1 file changed, 28 insertions(+), 46 deletions(-) > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c > index 6c1b42b58e3e..c0e7e090a05a 100644 > --- a/tools/testing/selftests/nolibc/nolibc-test.c > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > @@ -1039,10 +1039,13 @@ int run_stdlib(int min, int max) > return ret; > } > > -#define EXPECT_VFPRINTF(c, expected, fmt, ...) \ > - ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__) > +#define ASSERT_VFPRINTF(c, expected, fmt, ...) \ > + enum RESULT res = assert_vfprintf(_metadata, c, expected, fmt, ##__VA_ARGS__); \ > + if (res == SKIPPED) SKIP(return); \ > + if (res == FAIL) FAIL(return); Please enclose this in a "do { } while (0)" block when using more than one statement, because you can be certain that sooner or later someone will put an "if" or "for" above it. This will also avoid the double colon caused by the trailing one. > -static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) > +static enum RESULT assert_vfprintf(struct __test_metadata *_metadata, int c, > + const char *expected, const char *fmt, ...) > { > int ret, fd; > ssize_t w, r; > @@ -1051,25 +1054,20 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm > va_list args; > > fd = open("/tmp", O_TMPFILE | O_EXCL | O_RDWR, 0600); > - if (fd == -1) { > - result(llen, SKIPPED); > - return 0; > - } > + if (fd == -1) > + return SKIPPED; > > memfile = fdopen(fd, "w+"); > - if (!memfile) { > - result(llen, FAIL); > - return 1; > - } > + if (!memfile) > + return FAIL; Till now it looks easier and more readable. > va_start(args, fmt); > w = vfprintf(memfile, fmt, args); > va_end(args); > > if (w != c) { > - llen += printf(" written(%d) != %d", (int)w, c); > - result(llen, FAIL); > - return 1; > + _metadata->exe.llen += printf(" written(%d) != %d", (int)w, c); > + return FAIL; > } Here however I feel like we're already hacking internals of the test system and that doesn't seem natural nor logical. OK I understand that llen contains the lenght of the emitted message, but how should the user easily guess that it's placed into ->exe.llen, and they may or may not touch other fields there, etc ? Also the fact that the variable is prefixed with an underscore signals a warning to the user that they should not fiddle too much with its internals. I'm seeing basically two possible approaches: - one consisting in having a wrapper around printf() that transparently sets the llen field in _metadata->exe. This at least offload this knowledge from the user who can then just know they have to pass an opaque metadata argument and that's all. - one consisting in saying that such functions should not affect the test's metadata themselves and that it ought to be the caller's job instead. The function then only ought to return the printed length, and will not need the metadata at all. I tend to prefer the second option. In addition, you seem to be returning only 3 statuses (ok/skip/fail) so returning a single composite value for this is easy, e.g. (result | llen) with result made of macros only touching the high bits. If in the future more returns are needed, either a larger int or shift can be used, or we can then return pairs or structs. Regards, Willy
On 2023-11-16 08:48:02+0100, Willy Tarreau wrote: > On Wed, Nov 15, 2023 at 10:08:21PM +0100, Thomas Weißschuh wrote: > > Migrate part of nolibc-test.c to the new test harness. > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > --- > > tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++----------------- > > 1 file changed, 28 insertions(+), 46 deletions(-) > > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c > > index 6c1b42b58e3e..c0e7e090a05a 100644 > > --- a/tools/testing/selftests/nolibc/nolibc-test.c > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > > @@ -1039,10 +1039,13 @@ int run_stdlib(int min, int max) > > return ret; > > } > > > > -#define EXPECT_VFPRINTF(c, expected, fmt, ...) \ > > - ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__) > > +#define ASSERT_VFPRINTF(c, expected, fmt, ...) \ > > + enum RESULT res = assert_vfprintf(_metadata, c, expected, fmt, ##__VA_ARGS__); \ > > + if (res == SKIPPED) SKIP(return); \ > > + if (res == FAIL) FAIL(return); > > Please enclose this in a "do { } while (0)" block when using more than > one statement, because you can be certain that sooner or later someone > will put an "if" or "for" above it. This will also avoid the double > colon caused by the trailing one. Ack. > > > -static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) > > +static enum RESULT assert_vfprintf(struct __test_metadata *_metadata, int c, > > + const char *expected, const char *fmt, ...) > > { > > int ret, fd; > > ssize_t w, r; > > @@ -1051,25 +1054,20 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm > > va_list args; > > > > fd = open("/tmp", O_TMPFILE | O_EXCL | O_RDWR, 0600); > > - if (fd == -1) { > > - result(llen, SKIPPED); > > - return 0; > > - } > > + if (fd == -1) > > + return SKIPPED; > > > > memfile = fdopen(fd, "w+"); > > - if (!memfile) { > > - result(llen, FAIL); > > - return 1; > > - } > > + if (!memfile) > > + return FAIL; > > > Till now it looks easier and more readable. > > > va_start(args, fmt); > > w = vfprintf(memfile, fmt, args); > > va_end(args); > > > > if (w != c) { > > - llen += printf(" written(%d) != %d", (int)w, c); > > - result(llen, FAIL); > > - return 1; > > + _metadata->exe.llen += printf(" written(%d) != %d", (int)w, c); > > + return FAIL; > > } > > Here however I feel like we're already hacking internals of the test > system and that doesn't seem natural nor logical. OK I understand that > llen contains the lenght of the emitted message, but how should the > user easily guess that it's placed into ->exe.llen, and they may or may > not touch other fields there, etc ? Also the fact that the variable is > prefixed with an underscore signals a warning to the user that they > should not fiddle too much with its internals. Agree that this is ugly. > I'm seeing basically two possible approaches: > - one consisting in having a wrapper around printf() that transparently > sets the llen field in _metadata->exe. This at least offload this > knowledge from the user who can then just know they have to pass an > opaque metadata argument and that's all. > > - one consisting in saying that such functions should not affect the > test's metadata themselves and that it ought to be the caller's job > instead. The function then only ought to return the printed length, > and will not need the metadata at all. > > I tend to prefer the second option. In addition, you seem to be returning > only 3 statuses (ok/skip/fail) so returning a single composite value for > this is easy, e.g. (result | llen) with result made of macros only touching > the high bits. If in the future more returns are needed, either a larger > int or shift can be used, or we can then return pairs or structs. I am prefering the first option. It will make it easier to adapt the backend of the harness to KTAP I think. If you are fine with the basics of the harness I can convert all of nolibc-test.c and then also add the KTAP output at the end. WDYT? Thomas
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 6c1b42b58e3e..c0e7e090a05a 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1039,10 +1039,13 @@ int run_stdlib(int min, int max) return ret; } -#define EXPECT_VFPRINTF(c, expected, fmt, ...) \ - ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__) +#define ASSERT_VFPRINTF(c, expected, fmt, ...) \ + enum RESULT res = assert_vfprintf(_metadata, c, expected, fmt, ##__VA_ARGS__); \ + if (res == SKIPPED) SKIP(return); \ + if (res == FAIL) FAIL(return); -static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) +static enum RESULT assert_vfprintf(struct __test_metadata *_metadata, int c, + const char *expected, const char *fmt, ...) { int ret, fd; ssize_t w, r; @@ -1051,25 +1054,20 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm va_list args; fd = open("/tmp", O_TMPFILE | O_EXCL | O_RDWR, 0600); - if (fd == -1) { - result(llen, SKIPPED); - return 0; - } + if (fd == -1) + return SKIPPED; memfile = fdopen(fd, "w+"); - if (!memfile) { - result(llen, FAIL); - return 1; - } + if (!memfile) + return FAIL; va_start(args, fmt); w = vfprintf(memfile, fmt, args); va_end(args); if (w != c) { - llen += printf(" written(%d) != %d", (int)w, c); - result(llen, FAIL); - return 1; + _metadata->exe.llen += printf(" written(%d) != %d", (int)w, c); + return FAIL; } fflush(memfile); @@ -1080,46 +1078,30 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm fclose(memfile); if (r != w) { - llen += printf(" written(%d) != read(%d)", (int)w, (int)r); - result(llen, FAIL); - return 1; + _metadata->exe.llen += printf(" written(%d) != read(%d)", (int)w, (int)r); + return FAIL; } buf[r] = '\0'; - llen += printf(" \"%s\" = \"%s\"", expected, buf); + _metadata->exe.llen += printf(" \"%s\" = \"%s\"", expected, buf); ret = strncmp(expected, buf, c); - result(llen, ret ? FAIL : OK); - return ret; + return ret ? FAIL : OK; } -static int run_vfprintf(int min, int max) +TEST(vfprintf, empty) { ASSERT_VFPRINTF(0, "", ""); } +TEST(vfprintf, simple) { ASSERT_VFPRINTF(3, "foo", "foo"); } +TEST(vfprintf, string) { ASSERT_VFPRINTF(3, "foo", "%s", "foo"); } +TEST(vfprintf, number) { ASSERT_VFPRINTF(4, "1234", "%d", 1234); } +TEST(vfprintf, negnumber) { ASSERT_VFPRINTF(5, "-1234", "%d", -1234); } +TEST(vfprintf, unsigned) { ASSERT_VFPRINTF(5, "12345", "%u", 12345); } +TEST(vfprintf, char) { ASSERT_VFPRINTF(1, "c", "%c", 'c'); } +TEST(vfprintf, hex) { ASSERT_VFPRINTF(1, "f", "%x", 0xf); } +TEST(vfprintf, pointer) { ASSERT_VFPRINTF(3, "0x1", "%p", (void *) 0x1); } + +int run_vfprintf(int min, int max) { - int test; - int ret = 0; - - for (test = min; test >= 0 && test <= max; test++) { - int llen = 0; /* line length */ - - /* avoid leaving empty lines below, this will insert holes into - * test numbers. - */ - switch (test + __LINE__ + 1) { - CASE_TEST(empty); EXPECT_VFPRINTF(0, "", ""); break; - CASE_TEST(simple); EXPECT_VFPRINTF(3, "foo", "foo"); break; - CASE_TEST(string); EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break; - CASE_TEST(number); EXPECT_VFPRINTF(4, "1234", "%d", 1234); break; - CASE_TEST(negnumber); EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break; - CASE_TEST(unsigned); EXPECT_VFPRINTF(5, "12345", "%u", 12345); break; - CASE_TEST(char); EXPECT_VFPRINTF(1, "c", "%c", 'c'); break; - CASE_TEST(hex); EXPECT_VFPRINTF(1, "f", "%x", 0xf); break; - CASE_TEST(pointer); EXPECT_VFPRINTF(3, "0x1", "%p", (void *) 0x1); break; - case __LINE__: - return ret; /* must be last */ - /* note: do not set any defaults so as to permit holes above */ - } - } - return ret; + return run_test_suite("vfprintf", min, max); } static int smash_stack(void)
Migrate part of nolibc-test.c to the new test harness. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++----------------- 1 file changed, 28 insertions(+), 46 deletions(-)