[PATCH 0/8] tests: qemumonitor: Make fake qemu monitor failures more obvious
See patch 4 and 8 for details. Peter Krempa (8): tests: monitor: Rename qemuMonitorReportError to qemuMonitorTestAddErrorResponse qemuhotplugtest: detach: Add expected 'object-del' to disk-scsi-multipath case qemuhotplugtest: cpu: x86-modern-individual: Remove invalid test case qemumonitortestutils: Make test monitor failures more prominent qemucapabilitiesdata: riscv: Remove call to 'query-machines' qemuhotplugtest: Remove 'drive_del' expectation from failed cases qemumonitortestutils: Store a string identifying test monitor entry qemumonitortestutils: Enforce consumption of all items in test monitor build-aux/syntax-check.mk | 1 + tests/cputest.c | 2 + tests/qemuagenttest.c | 31 +-- .../caps_3.0.0.riscv32.replies | 42 ----- .../caps_3.0.0.riscv64.replies | 42 ----- .../caps_4.0.0.riscv32.replies | 42 ----- .../caps_4.0.0.riscv64.replies | 42 ----- tests/qemuhotplugtest.c | 12 +- tests/qemumonitorjsontest.c | 2 + tests/qemumonitortestutils.c | 176 +++++++++++------- tests/qemumonitortestutils.h | 5 +- 11 files changed, 141 insertions(+), 256 deletions(-) -- 2.26.0
It's a method of the test monitor and it adds a response to the monitor output. The original qemuMonitorTestAddErrorResponse method is renamed to qemuMonitorTestAddErrorResponseInternal Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuagenttest.c | 14 +++--- tests/qemumonitortestutils.c | 82 ++++++++++++++++++------------------ tests/qemumonitortestutils.h | 2 +- 3 files changed, 49 insertions(+), 49 deletions(-) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 42ef81ac9a..943251df77 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -413,7 +413,7 @@ qemuAgentShutdownTestMonitorHandler(qemuMonitorTestPtr test, return -1; if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - ret = qemuMonitorReportError(test, "Missing command name in %s", cmdstr); + ret = qemuMonitorTestAddErrorResponse(test, "Missing command name in %s", cmdstr); goto cleanup; } @@ -424,20 +424,20 @@ qemuAgentShutdownTestMonitorHandler(qemuMonitorTestPtr test, } if (!(args = virJSONValueObjectGet(val, "arguments"))) { - ret = qemuMonitorReportError(test, - "Missing arguments section"); + ret = qemuMonitorTestAddErrorResponse(test, + "Missing arguments section"); goto cleanup; } if (!(mode = virJSONValueObjectGetString(args, "mode"))) { - ret = qemuMonitorReportError(test, "Missing shutdown mode"); + ret = qemuMonitorTestAddErrorResponse(test, "Missing shutdown mode"); goto cleanup; } if (STRNEQ(mode, data->mode)) { - ret = qemuMonitorReportError(test, - "expected shutdown mode '%s' got '%s'", - data->mode, mode); + ret = qemuMonitorTestAddErrorResponse(test, + "expected shutdown mode '%s' got '%s'", + data->mode, mode); goto cleanup; } diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 992d2a4e71..dc753dd417 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -122,8 +122,8 @@ qemuMonitorTestAddResponse(qemuMonitorTestPtr test, static int -qemuMonitorTestAddErrorResponse(qemuMonitorTestPtr test, - const char *usermsg) +qemuMonitorTestAddErrorResponseInternal(qemuMonitorTestPtr test, + const char *usermsg) { virBuffer buf = VIR_BUFFER_INITIALIZER; g_autofree char *escapemsg = NULL; @@ -161,7 +161,7 @@ qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test, msg = g_strdup_printf("unexpected command: '%s'", command); - return qemuMonitorTestAddErrorResponse(test, msg); + return qemuMonitorTestAddErrorResponseInternal(test, msg); } @@ -175,12 +175,12 @@ qemuMonitorTestAddInvalidCommandResponse(qemuMonitorTestPtr test, msg = g_strdup_printf("expected command '%s' got '%s'", expectedcommand, actualcommand); - return qemuMonitorTestAddErrorResponse(test, msg); + return qemuMonitorTestAddErrorResponseInternal(test, msg); } int G_GNUC_PRINTF(2, 3) -qemuMonitorReportError(qemuMonitorTestPtr test, const char *errmsg, ...) +qemuMonitorTestAddErrorResponse(qemuMonitorTestPtr test, const char *errmsg, ...) { va_list msgargs; g_autofree char *msg = NULL; @@ -527,9 +527,9 @@ qemuMonitorTestProcessCommandDefaultValidate(qemuMonitorTestPtr test, if (virQEMUQAPISchemaPathGet(schemapath, test->qapischema, &schemaroot) < 0 || !schemaroot) { - if (qemuMonitorReportError(test, - "command '%s' not found in QAPI schema", - cmdname) == 0) + if (qemuMonitorTestAddErrorResponse(test, + "command '%s' not found in QAPI schema", + cmdname) == 0) return 1; return -1; } @@ -546,11 +546,11 @@ qemuMonitorTestProcessCommandDefaultValidate(qemuMonitorTestPtr test, cmdname, NULLSTR(argstr), virBufferCurrentContent(&debug)); } - if (qemuMonitorReportError(test, - "failed to validate arguments of '%s' " - "against QAPI schema " - "(to see debug output use VIR_TEST_DEBUG=2)", - cmdname) == 0) + if (qemuMonitorTestAddErrorResponse(test, + "failed to validate arguments of '%s' " + "against QAPI schema " + "(to see debug output use VIR_TEST_DEBUG=2)", + cmdname) == 0) return 1; return -1; } @@ -574,7 +574,7 @@ qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, return -1; if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) - return qemuMonitorReportError(test, "Missing command name in %s", cmdstr); + return qemuMonitorTestAddErrorResponse(test, "Missing command name in %s", cmdstr); cmdargs = virJSONValueObjectGet(val, "arguments"); if ((rc = qemuMonitorTestProcessCommandDefaultValidate(test, cmdname, cmdargs)) < 0) @@ -648,7 +648,7 @@ qemuMonitorTestProcessCommandVerbatim(qemuMonitorTestPtr test, if (data->cmderr) { errmsg = g_strdup_printf("%s: %s", data->cmderr, cmdstr); - ret = qemuMonitorTestAddErrorResponse(test, errmsg); + ret = qemuMonitorTestAddErrorResponseInternal(test, errmsg); } else { ret = qemuMonitorTestAddInvalidCommandResponse(test, data->command_name, @@ -718,7 +718,7 @@ qemuMonitorTestProcessGuestAgentSync(qemuMonitorTestPtr test, return -1; if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - ret = qemuMonitorReportError(test, "Missing guest-sync command name"); + ret = qemuMonitorTestAddErrorResponse(test, "Missing guest-sync command name"); goto cleanup; } @@ -728,12 +728,12 @@ qemuMonitorTestProcessGuestAgentSync(qemuMonitorTestPtr test, } if (!(args = virJSONValueObjectGet(val, "arguments"))) { - ret = qemuMonitorReportError(test, "Missing arguments for guest-sync"); + ret = qemuMonitorTestAddErrorResponse(test, "Missing arguments for guest-sync"); goto cleanup; } if (virJSONValueObjectGetNumberUlong(args, "id", &id)) { - ret = qemuMonitorReportError(test, "Missing id for guest sync"); + ret = qemuMonitorTestAddErrorResponse(test, "Missing id for guest sync"); goto cleanup; } @@ -782,7 +782,7 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, return -1; if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - ret = qemuMonitorReportError(test, "Missing command name in %s", cmdstr); + ret = qemuMonitorTestAddErrorResponse(test, "Missing command name in %s", cmdstr); goto cleanup; } @@ -794,9 +794,9 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, } if (!(args = virJSONValueObjectGet(val, "arguments"))) { - ret = qemuMonitorReportError(test, - "Missing arguments section for command '%s'", - NULLSTR(data->command_name)); + ret = qemuMonitorTestAddErrorResponse(test, + "Missing arguments section for command '%s'", + NULLSTR(data->command_name)); goto cleanup; } @@ -804,10 +804,10 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, for (i = 0; i < data->nargs; i++) { qemuMonitorTestCommandArgsPtr arg = &data->args[i]; if (!(argobj = virJSONValueObjectGet(args, arg->argname))) { - ret = qemuMonitorReportError(test, - "Missing argument '%s' for command '%s'", - arg->argname, - NULLSTR(data->command_name)); + ret = qemuMonitorTestAddErrorResponse(test, + "Missing argument '%s' for command '%s'", + arg->argname, + NULLSTR(data->command_name)); goto cleanup; } @@ -817,13 +817,13 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, /* verify that the argument value is expected */ if (STRNEQ(argstr, arg->argval)) { - ret = qemuMonitorReportError(test, - "Invalid value of argument '%s' " - "of command '%s': " - "expected '%s' got '%s'", - arg->argname, - NULLSTR(data->command_name), - arg->argval, argstr); + ret = qemuMonitorTestAddErrorResponse(test, + "Invalid value of argument '%s' " + "of command '%s': " + "expected '%s' got '%s'", + arg->argname, + NULLSTR(data->command_name), + arg->argval, argstr); goto cleanup; } @@ -908,7 +908,7 @@ qemuMonitorTestProcessCommandWithArgStr(qemuMonitorTestPtr test, return -1; if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - ret = qemuMonitorReportError(test, "Missing command name in %s", cmdstr); + ret = qemuMonitorTestAddErrorResponse(test, "Missing command name in %s", cmdstr); goto cleanup; } @@ -919,9 +919,9 @@ qemuMonitorTestProcessCommandWithArgStr(qemuMonitorTestPtr test, } if (!(args = virJSONValueObjectGet(val, "arguments"))) { - ret = qemuMonitorReportError(test, - "Missing arguments section for command '%s'", - data->command_name); + ret = qemuMonitorTestAddErrorResponse(test, + "Missing arguments section for command '%s'", + data->command_name); goto cleanup; } @@ -931,10 +931,10 @@ qemuMonitorTestProcessCommandWithArgStr(qemuMonitorTestPtr test, /* verify that the argument value is expected */ if (STRNEQ(argstr, data->expectArgs)) { - ret = qemuMonitorReportError(test, - "%s: expected arguments: '%s', got: '%s'", - data->command_name, - data->expectArgs, argstr); + ret = qemuMonitorTestAddErrorResponse(test, + "%s: expected arguments: '%s', got: '%s'", + data->command_name, + data->expectArgs, argstr); goto cleanup; } diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index 8e213ec921..c693b626fc 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -47,7 +47,7 @@ int qemuMonitorTestAddInvalidCommandResponse(qemuMonitorTestPtr test, void *qemuMonitorTestItemGetPrivateData(qemuMonitorTestItemPtr item); -int qemuMonitorReportError(qemuMonitorTestPtr test, const char *errmsg, ...); +int qemuMonitorTestAddErrorResponse(qemuMonitorTestPtr test, const char *errmsg, ...); int qemuMonitorTestAddItem(qemuMonitorTestPtr test, const char *command_name, -- 2.26.0
The test verifies unplug of a disk with the persistent reservations helper. The 'object-del' used to remove the helper was not mentioned in the list of expected commands. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuhotplugtest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 9a787f9d11..628689d27a 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -763,7 +763,8 @@ mymain(void) "human-monitor-command", HMP("")); DO_TEST_DETACH("base-live", "disk-scsi-multipath", false, false, "device_del", QMP_DEVICE_DELETED("scsi0-0-0-0") QMP_OK, - "human-monitor-command", HMP("")); + "human-monitor-command", HMP(""), + "object-del", QMP_OK); DO_TEST_ATTACH("base-live", "qemu-agent", false, true, "chardev-add", QMP_OK, -- 2.26.0
One of the test cases attempted to use test data meant for modern qemu without asserting the 'modern' flag. Since that changes the commands used to query state it won't work with data meant for the modern case. Remove the invalid test case. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuhotplugtest.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 628689d27a..8afe7f7faa 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -886,7 +886,6 @@ mymain(void) DO_TEST_CPU_INDIVIDUAL("x86-modern-individual-add", "7", true, true, false); DO_TEST_CPU_INDIVIDUAL("x86-modern-individual-add", "6,7", true, true, true); DO_TEST_CPU_INDIVIDUAL("x86-modern-individual-add", "7", false, true, true); - DO_TEST_CPU_INDIVIDUAL("x86-modern-individual-add", "7", true, false, true); DO_TEST_CPU_INDIVIDUAL("ppc64-modern-individual", "16-23", true, true, false); DO_TEST_CPU_INDIVIDUAL("ppc64-modern-individual", "16-22", true, true, true); -- 2.26.0
Until now we've tried to report errors from the test monitor code by passing them back as failures from the qemu we simulate. This doesn't work well in cases when the monitor logic does not detect failures or has fallback code. Additionally there isn't much use for continuing the test execution after first failure as in most cases the test data will be misaligned and all other calls will fail as well. To make the errors more obvious this patch moves away from reporting them via the simulated monitor to reporting them to stderr and exit()ing afterwards. While this might be less convenient when developing tests it actually makes failures in the test suite really obvious and doesn't require any opt-in from the tests themselves. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- build-aux/syntax-check.mk | 1 + tests/qemumonitortestutils.c | 121 +++++++++++++++++------------------ 2 files changed, 61 insertions(+), 61 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index cbcdf445aa..bf8832a2a5 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -900,6 +900,7 @@ sc_flake8: sc_prohibit_exit_in_tests: @prohibit='\<exit *\(' \ in_vc_files='tests/.*\.c$$' \ + exclude='exempt from syntax-check' \ halt='use return, not exit(), in tests' \ $(_sc_search_regexp) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index dc753dd417..bc3b2f5f92 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -153,18 +153,6 @@ qemuMonitorTestAddErrorResponseInternal(qemuMonitorTestPtr test, } -static int -qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test, - const char *command) -{ - g_autofree char *msg = NULL; - - msg = g_strdup_printf("unexpected command: '%s'", command); - - return qemuMonitorTestAddErrorResponseInternal(test, msg); -} - - int qemuMonitorTestAddInvalidCommandResponse(qemuMonitorTestPtr test, const char *expectedcommand, @@ -201,6 +189,33 @@ qemuMonitorTestAddErrorResponse(qemuMonitorTestPtr test, const char *errmsg, ... } +static void G_GNUC_NORETURN G_GNUC_PRINTF(1, 2) +qemuMonitorTestError(const char *errmsg, + ...) +{ + va_list msgargs; + + va_start(msgargs, errmsg); + + fflush(stderr); + g_fprintf(stderr, "\n"); + g_vfprintf(stderr, errmsg, msgargs); + g_fprintf(stderr, "\n"); + fflush(stderr); + exit(EXIT_FAILURE); /* exempt from syntax-check */ +} + + +static void G_GNUC_NORETURN +qemuMonitorTestErrorInvalidCommand(const char *expectedcommand, + const char *actualcommand) +{ + qemuMonitorTestError("expected command '%s' got '%s'", + expectedcommand, actualcommand); +} + + + static int qemuMonitorTestProcessCommand(qemuMonitorTestPtr test, const char *cmdstr) @@ -210,7 +225,7 @@ qemuMonitorTestProcessCommand(qemuMonitorTestPtr test, VIR_DEBUG("Processing string from monitor handler: '%s", cmdstr); if (test->nitems == 0) { - return qemuMonitorTestAddUnexpectedErrorResponse(test, cmdstr); + qemuMonitorTestError("unexpected command: '%s'", cmdstr); } else { qemuMonitorTestItemPtr item = test->items[0]; ret = (item->cb)(test, item, cmdstr); @@ -527,10 +542,7 @@ qemuMonitorTestProcessCommandDefaultValidate(qemuMonitorTestPtr test, if (virQEMUQAPISchemaPathGet(schemapath, test->qapischema, &schemaroot) < 0 || !schemaroot) { - if (qemuMonitorTestAddErrorResponse(test, - "command '%s' not found in QAPI schema", - cmdname) == 0) - return 1; + qemuMonitorTestError("command '%s' not found in QAPI schema", cmdname); return -1; } @@ -546,12 +558,10 @@ qemuMonitorTestProcessCommandDefaultValidate(qemuMonitorTestPtr test, cmdname, NULLSTR(argstr), virBufferCurrentContent(&debug)); } - if (qemuMonitorTestAddErrorResponse(test, - "failed to validate arguments of '%s' " - "against QAPI schema " - "(to see debug output use VIR_TEST_DEBUG=2)", - cmdname) == 0) - return 1; + qemuMonitorTestError("failed to validate arguments of '%s' " + "against QAPI schema " + "(to see debug output use VIR_TEST_DEBUG=2)", + cmdname); return -1; } @@ -573,8 +583,10 @@ qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, if (!(val = virJSONValueFromString(cmdstr))) return -1; - if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) - return qemuMonitorTestAddErrorResponse(test, "Missing command name in %s", cmdstr); + if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { + qemuMonitorTestError("Missing command name in %s", cmdstr); + return -1; + } cmdargs = virJSONValueObjectGet(val, "arguments"); if ((rc = qemuMonitorTestProcessCommandDefaultValidate(test, cmdname, cmdargs)) < 0) @@ -583,8 +595,8 @@ qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, return 0; if (data->command_name && STRNEQ(data->command_name, cmdname)) { - return qemuMonitorTestAddInvalidCommandResponse(test, data->command_name, - cmdname); + qemuMonitorTestErrorInvalidCommand(data->command_name, cmdname); + return -1; } else { return qemuMonitorTestAddResponse(test, data->response); } @@ -617,7 +629,6 @@ qemuMonitorTestProcessCommandVerbatim(qemuMonitorTestPtr test, { struct qemuMonitorTestHandlerData *data = item->opaque; g_autofree char *reformatted = NULL; - g_autofree char *errmsg = NULL; g_autoptr(virJSONValue) json = NULL; virJSONValuePtr cmdargs; const char *cmdname; @@ -646,13 +657,9 @@ qemuMonitorTestProcessCommandVerbatim(qemuMonitorTestPtr test, ret = qemuMonitorTestAddResponse(test, data->response); } else { if (data->cmderr) { - errmsg = g_strdup_printf("%s: %s", data->cmderr, cmdstr); - - ret = qemuMonitorTestAddErrorResponseInternal(test, errmsg); + qemuMonitorTestError("%s: %s", data->cmderr, cmdstr); } else { - ret = qemuMonitorTestAddInvalidCommandResponse(test, - data->command_name, - cmdstr); + qemuMonitorTestErrorInvalidCommand(data->command_name, cmdstr); } } @@ -782,21 +789,19 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, return -1; if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - ret = qemuMonitorTestAddErrorResponse(test, "Missing command name in %s", cmdstr); + qemuMonitorTestError("Missing command name in %s", cmdstr); goto cleanup; } if (data->command_name && STRNEQ(data->command_name, cmdname)) { - ret = qemuMonitorTestAddInvalidCommandResponse(test, data->command_name, - cmdname); + qemuMonitorTestErrorInvalidCommand(data->command_name, cmdname); goto cleanup; } if (!(args = virJSONValueObjectGet(val, "arguments"))) { - ret = qemuMonitorTestAddErrorResponse(test, - "Missing arguments section for command '%s'", - NULLSTR(data->command_name)); + qemuMonitorTestError("Missing arguments section for command '%s'", + NULLSTR(data->command_name)); goto cleanup; } @@ -804,10 +809,9 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, for (i = 0; i < data->nargs; i++) { qemuMonitorTestCommandArgsPtr arg = &data->args[i]; if (!(argobj = virJSONValueObjectGet(args, arg->argname))) { - ret = qemuMonitorTestAddErrorResponse(test, - "Missing argument '%s' for command '%s'", - arg->argname, - NULLSTR(data->command_name)); + qemuMonitorTestError("Missing argument '%s' for command '%s'", + arg->argname, + NULLSTR(data->command_name)); goto cleanup; } @@ -817,13 +821,11 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, /* verify that the argument value is expected */ if (STRNEQ(argstr, arg->argval)) { - ret = qemuMonitorTestAddErrorResponse(test, - "Invalid value of argument '%s' " - "of command '%s': " - "expected '%s' got '%s'", - arg->argname, - NULLSTR(data->command_name), - arg->argval, argstr); + qemuMonitorTestError("Invalid value of argument '%s' of command '%s': " + "expected '%s' got '%s'", + arg->argname, + NULLSTR(data->command_name), + arg->argval, argstr); goto cleanup; } @@ -908,20 +910,18 @@ qemuMonitorTestProcessCommandWithArgStr(qemuMonitorTestPtr test, return -1; if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - ret = qemuMonitorTestAddErrorResponse(test, "Missing command name in %s", cmdstr); + qemuMonitorTestError("Missing command name in %s", cmdstr); goto cleanup; } if (STRNEQ(data->command_name, cmdname)) { - ret = qemuMonitorTestAddInvalidCommandResponse(test, data->command_name, - cmdname); + qemuMonitorTestErrorInvalidCommand(data->command_name, cmdname); goto cleanup; } if (!(args = virJSONValueObjectGet(val, "arguments"))) { - ret = qemuMonitorTestAddErrorResponse(test, - "Missing arguments section for command '%s'", - data->command_name); + qemuMonitorTestError("Missing arguments section for command '%s'", + data->command_name); goto cleanup; } @@ -931,10 +931,9 @@ qemuMonitorTestProcessCommandWithArgStr(qemuMonitorTestPtr test, /* verify that the argument value is expected */ if (STRNEQ(argstr, data->expectArgs)) { - ret = qemuMonitorTestAddErrorResponse(test, - "%s: expected arguments: '%s', got: '%s'", - data->command_name, - data->expectArgs, argstr); + qemuMonitorTestError("%s: expected arguments: '%s', got: '%s'", + data->command_name, + data->expectArgs, argstr); goto cleanup; } -- 2.26.0
On 4/23/20 5:22 PM, Peter Krempa wrote:
Until now we've tried to report errors from the test monitor code by passing them back as failures from the qemu we simulate. This doesn't work well in cases when the monitor logic does not detect failures or has fallback code. Additionally there isn't much use for continuing the test execution after first failure as in most cases the test data will be misaligned and all other calls will fail as well.
To make the errors more obvious this patch moves away from reporting them via the simulated monitor to reporting them to stderr and exit()ing afterwards. While this might be less convenient when developing tests it actually makes failures in the test suite really obvious and doesn't require any opt-in from the tests themselves.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- build-aux/syntax-check.mk | 1 + tests/qemumonitortestutils.c | 121 +++++++++++++++++------------------ 2 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index cbcdf445aa..bf8832a2a5 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -900,6 +900,7 @@ sc_flake8: sc_prohibit_exit_in_tests: @prohibit='\<exit *\(' \ in_vc_files='tests/.*\.c$$' \ + exclude='exempt from syntax-check' \ halt='use return, not exit(), in tests' \ $(_sc_search_regexp)
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index dc753dd417..bc3b2f5f92 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -153,18 +153,6 @@ qemuMonitorTestAddErrorResponseInternal(qemuMonitorTestPtr test, }
-static int -qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test, - const char *command) -{ - g_autofree char *msg = NULL; - - msg = g_strdup_printf("unexpected command: '%s'", command); - - return qemuMonitorTestAddErrorResponseInternal(test, msg); -} - - int qemuMonitorTestAddInvalidCommandResponse(qemuMonitorTestPtr test, const char *expectedcommand, @@ -201,6 +189,33 @@ qemuMonitorTestAddErrorResponse(qemuMonitorTestPtr test, const char *errmsg, ... }
+static void G_GNUC_NORETURN G_GNUC_PRINTF(1, 2) +qemuMonitorTestError(const char *errmsg, + ...) +{ + va_list msgargs; + + va_start(msgargs, errmsg); + + fflush(stderr); + g_fprintf(stderr, "\n"); + g_vfprintf(stderr, errmsg, msgargs); + g_fprintf(stderr, "\n"); + fflush(stderr); + exit(EXIT_FAILURE); /* exempt from syntax-check */
Are these fflush() calls needed? I mean, stderr is unbuffered, isn't it? Michal
The riscv capabilities code doesn't use the data. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../caps_3.0.0.riscv32.replies | 42 ------------------- .../caps_3.0.0.riscv64.replies | 42 ------------------- .../caps_4.0.0.riscv32.replies | 42 ------------------- .../caps_4.0.0.riscv64.replies | 42 ------------------- 4 files changed, 168 deletions(-) diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.replies b/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.replies index efb18678a2..8159b26d19 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.replies +++ b/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.replies @@ -14718,45 +14718,3 @@ ], "id": "libvirt-33" } - -{ - "execute": "query-machines", - "id": "libvirt-34" -} - -{ - "return": [ - { - "hotpluggable-cpus": false, - "name": "spike_v1.9.1", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "sifive_e", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "none", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "spike_v1.10", - "is-default": true, - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "virt", - "cpu-max": 8 - }, - { - "hotpluggable-cpus": false, - "name": "sifive_u", - "cpu-max": 1 - } - ], - "id": "libvirt-34" -} diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.replies b/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.replies index 018e52f8da..995ca86784 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.replies +++ b/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.replies @@ -14718,45 +14718,3 @@ ], "id": "libvirt-33" } - -{ - "execute": "query-machines", - "id": "libvirt-34" -} - -{ - "return": [ - { - "hotpluggable-cpus": false, - "name": "spike_v1.9.1", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "sifive_e", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "none", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "spike_v1.10", - "is-default": true, - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "virt", - "cpu-max": 8 - }, - { - "hotpluggable-cpus": false, - "name": "sifive_u", - "cpu-max": 1 - } - ], - "id": "libvirt-34" -} diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies index 2d63851d3a..93fd47e8d3 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies @@ -17963,45 +17963,3 @@ ], "id": "libvirt-40" } - -{ - "execute": "query-machines", - "id": "libvirt-41" -} - -{ - "return": [ - { - "hotpluggable-cpus": false, - "name": "virt", - "cpu-max": 8 - }, - { - "hotpluggable-cpus": false, - "name": "none", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "spike_v1.10", - "is-default": true, - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "sifive_u", - "cpu-max": 4 - }, - { - "hotpluggable-cpus": false, - "name": "sifive_e", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "spike_v1.9.1", - "cpu-max": 1 - } - ], - "id": "libvirt-41" -} diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies index 4df475d7c0..448c9d2402 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies @@ -17963,45 +17963,3 @@ ], "id": "libvirt-40" } - -{ - "execute": "query-machines", - "id": "libvirt-41" -} - -{ - "return": [ - { - "hotpluggable-cpus": false, - "name": "virt", - "cpu-max": 8 - }, - { - "hotpluggable-cpus": false, - "name": "none", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "spike_v1.10", - "is-default": true, - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "sifive_u", - "cpu-max": 4 - }, - { - "hotpluggable-cpus": false, - "name": "sifive_e", - "cpu-max": 1 - }, - { - "hotpluggable-cpus": false, - "name": "spike_v1.9.1", - "cpu-max": 1 - } - ], - "id": "libvirt-41" -} -- 2.26.0
On failure 'drive_del' is not issued. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuhotplugtest.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 8afe7f7faa..65867a0122 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -748,8 +748,7 @@ mymain(void) /* Disk added */ "device_add", QMP_OK); DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", true, true, - "device_del", QMP_OK, - "human-monitor-command", HMP("")); + "device_del", QMP_OK); DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", false, false, "device_del", QMP_DEVICE_DELETED("scsi3-0-5-6") QMP_OK, "human-monitor-command", HMP("")); @@ -759,8 +758,7 @@ mymain(void) "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); DO_TEST_DETACH("base-live", "disk-scsi-multipath", true, true, - "device_del", QMP_OK, - "human-monitor-command", HMP("")); + "device_del", QMP_OK); DO_TEST_DETACH("base-live", "disk-scsi-multipath", false, false, "device_del", QMP_DEVICE_DELETED("scsi0-0-0-0") QMP_OK, "human-monitor-command", HMP(""), -- 2.26.0
For each test monitor entry store an optional string which will allow to identify it. This will be used later when checking that all registered monitor commands were used. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuagenttest.c | 17 ++++++++++++----- tests/qemumonitortestutils.c | 10 ++++++++++ tests/qemumonitortestutils.h | 1 + 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 943251df77..2c3a1efb09 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -471,7 +471,8 @@ testQemuAgentShutdown(const void *data) priv.event = QEMU_AGENT_EVENT_SHUTDOWN; priv.mode = "halt"; - if (qemuMonitorTestAddHandler(test, qemuAgentShutdownTestMonitorHandler, + if (qemuMonitorTestAddHandler(test, "guest-shutdown", + qemuAgentShutdownTestMonitorHandler, &priv, NULL) < 0) goto cleanup; @@ -485,7 +486,8 @@ testQemuAgentShutdown(const void *data) priv.event = QEMU_AGENT_EVENT_SHUTDOWN; priv.mode = "powerdown"; - if (qemuMonitorTestAddHandler(test, qemuAgentShutdownTestMonitorHandler, + if (qemuMonitorTestAddHandler(test, "guest-shutdown", + qemuAgentShutdownTestMonitorHandler, &priv, NULL) < 0) goto cleanup; @@ -499,7 +501,9 @@ testQemuAgentShutdown(const void *data) priv.event = QEMU_AGENT_EVENT_RESET; priv.mode = "reboot"; - if (qemuMonitorTestAddHandler(test, qemuAgentShutdownTestMonitorHandler, + if (qemuMonitorTestAddHandler(test, + "guest-shutdown", + qemuAgentShutdownTestMonitorHandler, &priv, NULL) < 0) goto cleanup; @@ -720,7 +724,8 @@ testQemuAgentTimeout(const void *data) goto cleanup; } - if (qemuMonitorTestAddHandler(test, qemuAgentTimeoutTestMonitorHandler, + if (qemuMonitorTestAddHandler(test, NULL, + qemuAgentTimeoutTestMonitorHandler, NULL, NULL) < 0) goto cleanup; @@ -734,7 +739,9 @@ testQemuAgentTimeout(const void *data) if (qemuMonitorTestAddAgentSyncResponse(test) < 0) goto cleanup; - if (qemuMonitorTestAddHandler(test, qemuAgentTimeoutTestMonitorHandler, + if (qemuMonitorTestAddHandler(test, + NULL, + qemuAgentTimeoutTestMonitorHandler, NULL, NULL) < 0) goto cleanup; diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index bc3b2f5f92..1af56c6d87 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -43,6 +43,7 @@ VIR_LOG_INIT("tests.qemumonitortestutils"); struct _qemuMonitorTestItem { + char *identifier; qemuMonitorTestResponseCallback cb; void *opaque; virFreeCallback freecb; @@ -88,6 +89,8 @@ qemuMonitorTestItemFree(qemuMonitorTestItemPtr item) if (!item) return; + g_free(item->identifier); + if (item->freecb) (item->freecb)(item->opaque); @@ -436,6 +439,7 @@ qemuMonitorTestFree(qemuMonitorTestPtr test) int qemuMonitorTestAddHandler(qemuMonitorTestPtr test, + const char *identifier, qemuMonitorTestResponseCallback cb, void *opaque, virFreeCallback freecb) @@ -445,6 +449,7 @@ qemuMonitorTestAddHandler(qemuMonitorTestPtr test, if (VIR_ALLOC(item) < 0) goto error; + item->identifier = g_strdup(identifier); item->cb = cb; item->freecb = freecb; item->opaque = opaque; @@ -617,6 +622,7 @@ qemuMonitorTestAddItem(qemuMonitorTestPtr test, data->response = g_strdup(response); return qemuMonitorTestAddHandler(test, + command_name, qemuMonitorTestProcessCommandDefault, data, qemuMonitorTestHandlerDataFree); } @@ -700,6 +706,7 @@ qemuMonitorTestAddItemVerbatim(qemuMonitorTestPtr test, goto error; return qemuMonitorTestAddHandler(test, + command, qemuMonitorTestProcessCommandVerbatim, data, qemuMonitorTestHandlerDataFree); @@ -766,6 +773,7 @@ qemuMonitorTestAddAgentSyncResponse(qemuMonitorTestPtr test) } return qemuMonitorTestAddHandler(test, + "agent-sync", qemuMonitorTestProcessGuestAgentSync, NULL, NULL); } @@ -884,6 +892,7 @@ qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, va_end(args); return qemuMonitorTestAddHandler(test, + cmdname, qemuMonitorTestProcessCommandWithArgs, data, qemuMonitorTestHandlerDataFree); @@ -988,6 +997,7 @@ qemuMonitorTestAddItemExpect(qemuMonitorTestPtr test, } return qemuMonitorTestAddHandler(test, + cmdname, qemuMonitorTestProcessCommandWithArgStr, data, qemuMonitorTestHandlerDataFree); diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index c693b626fc..384002d086 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -34,6 +34,7 @@ typedef int (*qemuMonitorTestResponseCallback)(qemuMonitorTestPtr test, const char *message); int qemuMonitorTestAddHandler(qemuMonitorTestPtr test, + const char *identifier, qemuMonitorTestResponseCallback cb, void *opaque, virFreeCallback freecb); -- 2.26.0
To prevent unexpected situations where a change in code would stop looking at some of the tested commands go unnoticed add a mechanism to force consumption of all test items. Since there are a few tests which would be hard to fix add also a mechanism to opt-out of the check. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/cputest.c | 2 ++ tests/qemuhotplugtest.c | 2 ++ tests/qemumonitorjsontest.c | 2 ++ tests/qemumonitortestutils.c | 31 ++++++++++++++++++++++++++++++- tests/qemumonitortestutils.h | 2 ++ 5 files changed, 38 insertions(+), 1 deletion(-) diff --git a/tests/cputest.c b/tests/cputest.c index 869d016ffc..21f47a6853 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -482,6 +482,8 @@ cpuTestMakeQEMUCaps(const struct data *data) if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true))) goto error; + qemuMonitorTestAllowUnusedCommands(testMon); + cpu = virCPUDefNew(); cpu->model = g_strdup("host"); diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 65867a0122..9a215ab303 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -453,6 +453,8 @@ testQemuHotplugCpuPrepare(const char *test, &driver, data->vm, qmpschema))) goto error; + qemuMonitorTestAllowUnusedCommands(data->mon); + priv->mon = qemuMonitorTestGetMonitor(data->mon); virObjectUnlock(priv->mon); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 615bc8c102..60c816d1d1 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -796,6 +796,8 @@ qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt, if (!(data.test = qemuMonitorTestNewSchema(xmlopt, schema))) goto cleanup; + qemuMonitorTestAllowUnusedCommands(data.test); + if (qemuMonitorTestAddItemExpect(data.test, "chardev-add", expectargs, true, jsonreply) < 0) goto cleanup; diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 1af56c6d87..0b6188b4ca 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -57,6 +57,8 @@ struct _qemuMonitorTest { bool running; bool started; + bool allowUnusedCommands; + char *incoming; size_t incomingLength; size_t incomingCapacity; @@ -423,8 +425,15 @@ qemuMonitorTestFree(qemuMonitorTestPtr test) VIR_FREE(test->incoming); VIR_FREE(test->outgoing); - for (i = 0; i < test->nitems; i++) + for (i = 0; i < test->nitems; i++) { + if (!test->allowUnusedCommands) { + g_fprintf(stderr, + "\nunused test monitor item '%s'\n", + NULLSTR(test->items[i]->identifier)); + } + qemuMonitorTestItemFree(test->items[i]); + } VIR_FREE(test->items); if (test->tmpdir && rmdir(test->tmpdir) < 0) @@ -432,6 +441,11 @@ qemuMonitorTestFree(qemuMonitorTestPtr test) VIR_FREE(test->tmpdir); + if (!test->allowUnusedCommands && + test->nitems != 0) { + qemuMonitorTestError("unused test monitor items are not allowed for this test\n"); + } + virMutexDestroy(&test->lock); VIR_FREE(test); } @@ -1290,6 +1304,21 @@ qemuMonitorTestNewFromFile(const char *fileName, } +/** + * qemuMonitorTestAllowUnusedCommands: + * @test: test monitor object + * + * By default all test items/commands must be used by the test. This function + * allows to override the requirement for individual tests e.g. if it's necessary + * to test some negative scenarios which would not use all commands. + */ +void +qemuMonitorTestAllowUnusedCommands(qemuMonitorTestPtr test) +{ + test->allowUnusedCommands = true; +} + + static int qemuMonitorTestFullAddItem(qemuMonitorTestPtr test, const char *filename, diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index 384002d086..f45e850000 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -50,6 +50,8 @@ void *qemuMonitorTestItemGetPrivateData(qemuMonitorTestItemPtr item); int qemuMonitorTestAddErrorResponse(qemuMonitorTestPtr test, const char *errmsg, ...); +void qemuMonitorTestAllowUnusedCommands(qemuMonitorTestPtr test); + int qemuMonitorTestAddItem(qemuMonitorTestPtr test, const char *command_name, const char *response); -- 2.26.0
On 4/23/20 5:21 PM, Peter Krempa wrote:
See patch 4 and 8 for details.
Peter Krempa (8): tests: monitor: Rename qemuMonitorReportError to qemuMonitorTestAddErrorResponse qemuhotplugtest: detach: Add expected 'object-del' to disk-scsi-multipath case qemuhotplugtest: cpu: x86-modern-individual: Remove invalid test case qemumonitortestutils: Make test monitor failures more prominent qemucapabilitiesdata: riscv: Remove call to 'query-machines' qemuhotplugtest: Remove 'drive_del' expectation from failed cases qemumonitortestutils: Store a string identifying test monitor entry qemumonitortestutils: Enforce consumption of all items in test monitor
build-aux/syntax-check.mk | 1 + tests/cputest.c | 2 + tests/qemuagenttest.c | 31 +-- .../caps_3.0.0.riscv32.replies | 42 ----- .../caps_3.0.0.riscv64.replies | 42 ----- .../caps_4.0.0.riscv32.replies | 42 ----- .../caps_4.0.0.riscv64.replies | 42 ----- tests/qemuhotplugtest.c | 12 +- tests/qemumonitorjsontest.c | 2 + tests/qemumonitortestutils.c | 176 +++++++++++------- tests/qemumonitortestutils.h | 5 +- 11 files changed, 141 insertions(+), 256 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Privoznik -
Peter Krempa