Migrate part of nolibc-test.c to the new test harness.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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);
-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)
--
2.42.1
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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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