[libvirt] [PATCH 0/3] tests: Drop virtTestResult
virtTestResult poorly duplicates logic from virtTestRun. We could fix it, but there's really only one legitimate user in eventtest.c, so drop the improper users, and opencode the legit usage. More justification in the last patch Thanks, Cole Cole Robinson (3): tests: sheepdog: Drop use of virtTestResult tests: eventtest: Open code virtTestResult testutils: Drop virtTestResult tests/eventtest.c | 57 ++++++++++++++++++++++++++++------ tests/storagebackendsheepdogtest.c | 63 ++++++++++++++++++++++++++++---------- tests/testutils.c | 38 ----------------------- tests/testutils.h | 2 -- 4 files changed, 94 insertions(+), 66 deletions(-) -- 2.5.0
Instead use the same pattern that most other test files use. --- tests/storagebackendsheepdogtest.c | 63 ++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 16 deletions(-) diff --git a/tests/storagebackendsheepdogtest.c b/tests/storagebackendsheepdogtest.c index 7744119..2b0f4db 100644 --- a/tests/storagebackendsheepdogtest.c +++ b/tests/storagebackendsheepdogtest.c @@ -44,15 +44,28 @@ typedef struct { uint64_t expected_allocation; } collie_test; +struct testNodeInfoParserData { + collie_test data; + const char *poolxml; +}; + +struct testVDIListParserData { + collie_test data; + const char *poolxml; + const char *volxml; +}; + static int -test_node_info_parser(collie_test test, char *poolxml) +test_node_info_parser(const void *opaque) { + const struct testNodeInfoParserData *data = opaque; + collie_test test = data->data; int ret = -1; char *output = NULL; virStoragePoolDefPtr pool = NULL; - if (!(pool = virStoragePoolDefParseFile(poolxml))) + if (!(pool = virStoragePoolDefParseFile(data->poolxml))) goto cleanup; if (VIR_STRDUP(output, test.output) < 0) @@ -78,17 +91,19 @@ test_node_info_parser(collie_test test, char *poolxml) } static int -test_vdi_list_parser(collie_test test, char *poolxml, char *volxml) +test_vdi_list_parser(const void *opaque) { + const struct testVDIListParserData *data = opaque; + collie_test test = data->data; int ret = -1; char *output = NULL; virStoragePoolDefPtr pool = NULL; virStorageVolDefPtr vol = NULL; - if (!(pool = virStoragePoolDefParseFile(poolxml))) + if (!(pool = virStoragePoolDefParseFile(data->poolxml))) goto cleanup; - if (!(vol = virStorageVolDefParseFile(pool, volxml, 0))) + if (!(vol = virStorageVolDefParseFile(pool, data->volxml, 0))) goto cleanup; if (VIR_STRDUP(output, test.output) < 0) @@ -118,7 +133,7 @@ test_vdi_list_parser(collie_test test, char *poolxml, char *volxml) static int mymain(void) { - int ret = -1; + int ret = 0; char *poolxml = NULL; char *volxml = NULL; @@ -170,26 +185,42 @@ mymain(void) abs_srcdir) < 0) goto cleanup; +#define DO_TEST_NODE(collie) \ + do { \ + struct testNodeInfoParserData data = { \ + .data = collie, \ + .poolxml = poolxml, \ + }; \ + if (virtTestRun("node_info_parser", test_node_info_parser, \ + &data) < 0) \ + ret = -1; \ + } while (0) + while (test->output != NULL) { - ret = test_node_info_parser(*test, poolxml); - virtTestResult("node_info_parser", ret, NULL); + DO_TEST_NODE(*test); ++test; - if (ret < 0) - return EXIT_FAILURE; } + +#define DO_TEST_VDI(collie) \ + do { \ + struct testVDIListParserData data = { \ + .data = collie, \ + .poolxml = poolxml, \ + .volxml = volxml, \ + }; \ + if (virtTestRun("vdi_list_parser", test_vdi_list_parser, \ + &data) < 0) \ + ret = -1; \ + } while (0) + test = vdi_list_tests; while (test->output != NULL) { - ret = test_vdi_list_parser(*test, poolxml, volxml); - virtTestResult("vdi_list_parser", ret, NULL); + DO_TEST_VDI(*test); ++test; - if (ret < 0) - return EXIT_FAILURE; } - ret = 0; - cleanup: VIR_FREE(poolxml); VIR_FREE(volxml); -- 2.5.0
On Tue, Sep 29, 2015 at 07:56:45PM -0400, Cole Robinson wrote:
Instead use the same pattern that most other test files use. --- tests/storagebackendsheepdogtest.c | 63 ++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 16 deletions(-)
ACK
These event tests aren't run synchronously, so there isn't an obvious function to pass to virtTestRun. Instead, open code roughly what virtTestResult did before: printing an error message if a test failed. --- tests/eventtest.c | 57 +++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 10 deletions(-) diff --git a/tests/eventtest.c b/tests/eventtest.c index 13adbf6..ab08181 100644 --- a/tests/eventtest.c +++ b/tests/eventtest.c @@ -63,6 +63,43 @@ enum { EV_ERROR_DATA, }; +struct testEventResultData { + bool failed; + const char *msg; +}; + +static int +testEventResultCallback(const void *opaque) +{ + const struct testEventResultData *data = opaque; + + if (data->failed && data->msg) { + fprintf(stderr, "%s", data->msg); + } + return data->failed; +} + +static void +ATTRIBUTE_FMT_PRINTF(3,4) +testEventReport(const char *name, bool failed, const char *msg, ...) +{ + va_list vargs; + va_start(vargs, msg); + char *str = NULL; + struct testEventResultData data; + + if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) { + failed = true; + } + + data.failed = failed; + data.msg = msg; + virtTestRun(name, testEventResultCallback, &data); + + va_end(vargs); + VIR_FREE(str); +} + static void testPipeReader(int watch, int fd, int events, void *data) { @@ -148,13 +185,13 @@ verifyFired(const char *name, int handle, int timer) for (i = 0; i < NUM_FDS; i++) { if (handles[i].fired) { if (i != handle) { - virtTestResult(name, 1, + testEventReport(name, 1, "Handle %zu fired, but expected %d\n", i, handle); return EXIT_FAILURE; } else { if (handles[i].error != EV_ERROR_NONE) { - virtTestResult(name, 1, + testEventReport(name, 1, "Handle %zu fired, but had error %d\n", i, handles[i].error); return EXIT_FAILURE; @@ -163,7 +200,7 @@ verifyFired(const char *name, int handle, int timer) } } else { if (i == handle) { - virtTestResult(name, 1, + testEventReport(name, 1, "Handle %d should have fired, but didn't\n", handle); return EXIT_FAILURE; @@ -171,7 +208,7 @@ verifyFired(const char *name, int handle, int timer) } } if (handleFired != 1 && handle != -1) { - virtTestResult(name, 1, + testEventReport(name, 1, "Something weird happened, expecting handle %d\n", handle); return EXIT_FAILURE; @@ -181,12 +218,12 @@ verifyFired(const char *name, int handle, int timer) for (i = 0; i < NUM_TIME; i++) { if (timers[i].fired) { if (i != timer) { - virtTestResult(name, 1, + testEventReport(name, 1, "Timer %zu fired, but expected %d\n", i, timer); return EXIT_FAILURE; } else { if (timers[i].error != EV_ERROR_NONE) { - virtTestResult(name, 1, + testEventReport(name, 1, "Timer %zu fired, but had error %d\n", i, timers[i].error); return EXIT_FAILURE; @@ -195,7 +232,7 @@ verifyFired(const char *name, int handle, int timer) } } else { if (i == timer) { - virtTestResult(name, 1, + testEventReport(name, 1, "Timer %d should have fired, but didn't\n", timer); return EXIT_FAILURE; @@ -203,7 +240,7 @@ verifyFired(const char *name, int handle, int timer) } } if (timerFired != 1 && timer != -1) { - virtTestResult(name, 1, + testEventReport(name, 1, "Something weird happened, expecting timer %d\n", timer); return EXIT_FAILURE; @@ -234,14 +271,14 @@ finishJob(const char *name, int handle, int timer) rc = pthread_cond_timedwait(&eventThreadJobCond, &eventThreadMutex, &waitTime); if (rc != 0) { - virtTestResult(name, 1, "Timed out waiting for pipe event\n"); + testEventReport(name, 1, "Timed out waiting for pipe event\n"); return EXIT_FAILURE; } if (verifyFired(name, handle, timer) != EXIT_SUCCESS) return EXIT_FAILURE; - virtTestResult(name, 0, NULL); + testEventReport(name, 0, NULL); return EXIT_SUCCESS; } -- 2.5.0
On 09/29/2015 07:56 PM, Cole Robinson wrote:
These event tests aren't run synchronously, so there isn't an obvious function to pass to virtTestRun. Instead, open code roughly what virtTestResult did before: printing an error message if a test failed. --- tests/eventtest.c | 57 +++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/tests/eventtest.c b/tests/eventtest.c index 13adbf6..ab08181 100644 --- a/tests/eventtest.c +++ b/tests/eventtest.c @@ -63,6 +63,43 @@ enum { EV_ERROR_DATA, };
+struct testEventResultData { + bool failed; + const char *msg; +}; + +static int +testEventResultCallback(const void *opaque) +{ + const struct testEventResultData *data = opaque; + + if (data->failed && data->msg) { + fprintf(stderr, "%s", data->msg); + } + return data->failed; +} + +static void +ATTRIBUTE_FMT_PRINTF(3,4) +testEventReport(const char *name, bool failed, const char *msg, ...) +{ + va_list vargs; + va_start(vargs, msg); + char *str = NULL; + struct testEventResultData data; + + if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) { + failed = true; + } + + data.failed = failed; + data.msg = msg; + virtTestRun(name, testEventResultCallback, &data); + + va_end(vargs); + VIR_FREE(str); +} + static void testPipeReader(int watch, int fd, int events, void *data) { @@ -148,13 +185,13 @@ verifyFired(const char *name, int handle, int timer) for (i = 0; i < NUM_FDS; i++) { if (handles[i].fired) { if (i != handle) { - virtTestResult(name, 1, + testEventReport(name, 1, "Handle %zu fired, but expected %d\n", i, handle); return EXIT_FAILURE; } else { if (handles[i].error != EV_ERROR_NONE) { - virtTestResult(name, 1, + testEventReport(name, 1, "Handle %zu fired, but had error %d\n", i, handles[i].error); return EXIT_FAILURE; @@ -163,7 +200,7 @@ verifyFired(const char *name, int handle, int timer) } } else { if (i == handle) { - virtTestResult(name, 1, + testEventReport(name, 1, "Handle %d should have fired, but didn't\n", handle); return EXIT_FAILURE; @@ -171,7 +208,7 @@ verifyFired(const char *name, int handle, int timer) } } if (handleFired != 1 && handle != -1) { - virtTestResult(name, 1, + testEventReport(name, 1, "Something weird happened, expecting handle %d\n", handle); return EXIT_FAILURE; @@ -181,12 +218,12 @@ verifyFired(const char *name, int handle, int timer) for (i = 0; i < NUM_TIME; i++) { if (timers[i].fired) { if (i != timer) { - virtTestResult(name, 1, + testEventReport(name, 1, "Timer %zu fired, but expected %d\n", i, timer); return EXIT_FAILURE; } else { if (timers[i].error != EV_ERROR_NONE) { - virtTestResult(name, 1, + testEventReport(name, 1, "Timer %zu fired, but had error %d\n", i, timers[i].error); return EXIT_FAILURE; @@ -195,7 +232,7 @@ verifyFired(const char *name, int handle, int timer) } } else { if (i == timer) { - virtTestResult(name, 1, + testEventReport(name, 1, "Timer %d should have fired, but didn't\n", timer); return EXIT_FAILURE; @@ -203,7 +240,7 @@ verifyFired(const char *name, int handle, int timer) } } if (timerFired != 1 && timer != -1) { - virtTestResult(name, 1, + testEventReport(name, 1, "Something weird happened, expecting timer %d\n", timer); return EXIT_FAILURE; @@ -234,14 +271,14 @@ finishJob(const char *name, int handle, int timer) rc = pthread_cond_timedwait(&eventThreadJobCond, &eventThreadMutex, &waitTime); if (rc != 0) { - virtTestResult(name, 1, "Timed out waiting for pipe event\n"); + testEventReport(name, 1, "Timed out waiting for pipe event\n"); return EXIT_FAILURE; }
if (verifyFired(name, handle, timer) != EXIT_SUCCESS) return EXIT_FAILURE;
- virtTestResult(name, 0, NULL); + testEventReport(name, 0, NULL); return EXIT_SUCCESS; }
I squashed this in locally to appease syntax-check: diff --git a/tests/eventtest.c b/tests/eventtest.c index ab08181..d0faf27 100644 --- a/tests/eventtest.c +++ b/tests/eventtest.c @@ -73,14 +73,13 @@ testEventResultCallback(const void *opaque) { const struct testEventResultData *data = opaque; - if (data->failed && data->msg) { + if (data->failed && data->msg) fprintf(stderr, "%s", data->msg); - } return data->failed; } static void -ATTRIBUTE_FMT_PRINTF(3,4) +ATTRIBUTE_FMT_PRINTF(3, 4) testEventReport(const char *name, bool failed, const char *msg, ...) { va_list vargs; @@ -88,9 +87,8 @@ testEventReport(const char *name, bool failed, const char *msg, ...) char *str = NULL; struct testEventResultData data; - if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) { + if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) failed = true; - } data.failed = failed; data.msg = msg; - Cole
On Tue, Sep 29, 2015 at 07:56:46PM -0400, Cole Robinson wrote:
These event tests aren't run synchronously, so there isn't an obvious function to pass to virtTestRun. Instead, open code roughly what virtTestResult did before: printing an error message if a test failed. --- tests/eventtest.c | 57 +++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/tests/eventtest.c b/tests/eventtest.c index 13adbf6..ab08181 100644 --- a/tests/eventtest.c +++ b/tests/eventtest.c @@ -63,6 +63,43 @@ enum { EV_ERROR_DATA, };
+struct testEventResultData { + bool failed; + const char *msg; +}; + +static int +testEventResultCallback(const void *opaque) +{ + const struct testEventResultData *data = opaque; + + if (data->failed && data->msg) { + fprintf(stderr, "%s", data->msg); + } + return data->failed; +} + +static void +ATTRIBUTE_FMT_PRINTF(3,4) +testEventReport(const char *name, bool failed, const char *msg, ...) +{ + va_list vargs; + va_start(vargs, msg); + char *str = NULL; + struct testEventResultData data; + + if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) { + failed = true; + } + + data.failed = failed; + data.msg = msg;
I think you meant data.msg = str; here. ACK with that changed and your amendment squashed in. P.S.: If you'd really like, I think this test could be made to use virTestRun as well ;)
On 09/30/2015 01:38 AM, Martin Kletzander wrote:
On Tue, Sep 29, 2015 at 07:56:46PM -0400, Cole Robinson wrote:
These event tests aren't run synchronously, so there isn't an obvious function to pass to virtTestRun. Instead, open code roughly what virtTestResult did before: printing an error message if a test failed. --- tests/eventtest.c | 57 +++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/tests/eventtest.c b/tests/eventtest.c index 13adbf6..ab08181 100644 --- a/tests/eventtest.c +++ b/tests/eventtest.c @@ -63,6 +63,43 @@ enum { EV_ERROR_DATA, };
+struct testEventResultData { + bool failed; + const char *msg; +}; + +static int +testEventResultCallback(const void *opaque) +{ + const struct testEventResultData *data = opaque; + + if (data->failed && data->msg) { + fprintf(stderr, "%s", data->msg); + } + return data->failed; +} + +static void +ATTRIBUTE_FMT_PRINTF(3,4) +testEventReport(const char *name, bool failed, const char *msg, ...) +{ + va_list vargs; + va_start(vargs, msg); + char *str = NULL; + struct testEventResultData data; + + if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) { + failed = true; + } + + data.failed = failed; + data.msg = msg;
I think you meant data.msg = str; here.
ACK with that changed and your amendment squashed in.
Changed locally, thanks for the review. I'll push after the release
P.S.: If you'd really like, I think this test could be made to use virTestRun as well ;)
Maybe for another patch :) - Cole
On 09/29/2015 07:56 PM, Cole Robinson wrote:
These event tests aren't run synchronously, so there isn't an obvious function to pass to virtTestRun. Instead, open code roughly what virtTestResult did before: printing an error message if a test failed. --- tests/eventtest.c | 57 +++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/tests/eventtest.c b/tests/eventtest.c index 13adbf6..ab08181 100644 --- a/tests/eventtest.c +++ b/tests/eventtest.c @@ -63,6 +63,43 @@ enum { EV_ERROR_DATA, };
+struct testEventResultData { + bool failed; + const char *msg; +}; + +static int +testEventResultCallback(const void *opaque) +{ + const struct testEventResultData *data = opaque; + + if (data->failed && data->msg) { + fprintf(stderr, "%s", data->msg); + } + return data->failed; +} + +static void +ATTRIBUTE_FMT_PRINTF(3,4) +testEventReport(const char *name, bool failed, const char *msg, ...) +{ + va_list vargs; + va_start(vargs, msg); + char *str = NULL; + struct testEventResultData data; + + if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) { + failed = true; + } + + data.failed = failed; + data.msg = msg; + virtTestRun(name, testEventResultCallback, &data);
Now that these are pushed Coverity is complaining: (3) Event check_return: Calling "virtTestRun" without checking return value (as is done elsewhere 2547 out of 2548 times). Also see events: John
+ + va_end(vargs); + VIR_FREE(str); +} + static void testPipeReader(int watch, int fd, int events, void *data) { @@ -148,13 +185,13 @@ verifyFired(const char *name, int handle, int timer) for (i = 0; i < NUM_FDS; i++) { if (handles[i].fired) { if (i != handle) { - virtTestResult(name, 1, + testEventReport(name, 1, "Handle %zu fired, but expected %d\n", i, handle); return EXIT_FAILURE; } else { if (handles[i].error != EV_ERROR_NONE) { - virtTestResult(name, 1, + testEventReport(name, 1, "Handle %zu fired, but had error %d\n", i, handles[i].error); return EXIT_FAILURE; @@ -163,7 +200,7 @@ verifyFired(const char *name, int handle, int timer) } } else { if (i == handle) { - virtTestResult(name, 1, + testEventReport(name, 1, "Handle %d should have fired, but didn't\n", handle); return EXIT_FAILURE; @@ -171,7 +208,7 @@ verifyFired(const char *name, int handle, int timer) } } if (handleFired != 1 && handle != -1) { - virtTestResult(name, 1, + testEventReport(name, 1, "Something weird happened, expecting handle %d\n", handle); return EXIT_FAILURE; @@ -181,12 +218,12 @@ verifyFired(const char *name, int handle, int timer) for (i = 0; i < NUM_TIME; i++) { if (timers[i].fired) { if (i != timer) { - virtTestResult(name, 1, + testEventReport(name, 1, "Timer %zu fired, but expected %d\n", i, timer); return EXIT_FAILURE; } else { if (timers[i].error != EV_ERROR_NONE) { - virtTestResult(name, 1, + testEventReport(name, 1, "Timer %zu fired, but had error %d\n", i, timers[i].error); return EXIT_FAILURE; @@ -195,7 +232,7 @@ verifyFired(const char *name, int handle, int timer) } } else { if (i == timer) { - virtTestResult(name, 1, + testEventReport(name, 1, "Timer %d should have fired, but didn't\n", timer); return EXIT_FAILURE; @@ -203,7 +240,7 @@ verifyFired(const char *name, int handle, int timer) } } if (timerFired != 1 && timer != -1) { - virtTestResult(name, 1, + testEventReport(name, 1, "Something weird happened, expecting timer %d\n", timer); return EXIT_FAILURE; @@ -234,14 +271,14 @@ finishJob(const char *name, int handle, int timer) rc = pthread_cond_timedwait(&eventThreadJobCond, &eventThreadMutex, &waitTime); if (rc != 0) { - virtTestResult(name, 1, "Timed out waiting for pipe event\n"); + testEventReport(name, 1, "Timed out waiting for pipe event\n"); return EXIT_FAILURE; }
if (verifyFired(name, handle, timer) != EXIT_SUCCESS) return EXIT_FAILURE;
- virtTestResult(name, 0, NULL); + testEventReport(name, 0, NULL); return EXIT_SUCCESS; }
virtTestResult is suboptimal for a few reasons: - It poorly duplicates virtTestRun pass/fail reporting logic - It doesn't have virtTestRun's alloc testing support - It only reports the test name _after_ the test has run. - It doesn't follow the standard virtTestRun pattern that most other tests use. There's no users left, so drop it. If any other async tests like eventtest spring up that don't cleanly fit the virtTestRun pattern, I suggest they just open code the support for it around virtTestRun --- tests/testutils.c | 38 -------------------------------------- tests/testutils.h | 2 -- 2 files changed, 40 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index bd4ff73..857e819 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -119,44 +119,6 @@ static void virTestAllocHook(int nalloc ATTRIBUTE_UNUSED, } #endif -void virtTestResult(const char *name, int ret, const char *msg, ...) -{ - va_list vargs; - va_start(vargs, msg); - - if (testCounter == 0 && !virTestGetVerbose()) - fprintf(stderr, " "); - - testCounter++; - if (virTestGetVerbose()) { - fprintf(stderr, "%3zu) %-60s ", testCounter, name); - if (ret == 0) { - fprintf(stderr, "OK\n"); - } else { - fprintf(stderr, "FAILED\n"); - if (msg) { - char *str; - if (virVasprintfQuiet(&str, msg, vargs) == 0) { - fprintf(stderr, "%s", str); - VIR_FREE(str); - } - } - } - } else { - if (testCounter != 1 && - !((testCounter-1) % 40)) { - fprintf(stderr, " %-3zu\n", (testCounter-1)); - fprintf(stderr, " "); - } - if (ret == 0) - fprintf(stderr, "."); - else - fprintf(stderr, "!"); - } - - va_end(vargs); -} - #ifdef TEST_OOM_TRACE static void virTestShowTrace(void) diff --git a/tests/testutils.h b/tests/testutils.h index f34a393..ccf1d29 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -48,8 +48,6 @@ extern char *progname; bool virtTestOOMActive(void); -void virtTestResult(const char *name, int ret, const char *msg, ...) - ATTRIBUTE_FMT_PRINTF(3,4); int virtTestRun(const char *title, int (*body)(const void *data), const void *data); -- 2.5.0
On Tue, Sep 29, 2015 at 07:56:47PM -0400, Cole Robinson wrote:
virtTestResult is suboptimal for a few reasons:
- It poorly duplicates virtTestRun pass/fail reporting logic - It doesn't have virtTestRun's alloc testing support - It only reports the test name _after_ the test has run. - It doesn't follow the standard virtTestRun pattern that most other tests use.
There's no users left, so drop it. If any other async tests like eventtest spring up that don't cleanly fit the virtTestRun pattern, I suggest they just open code the support for it around virtTestRun ---
ACK
participants (3)
-
Cole Robinson -
John Ferlan -
Martin Kletzander