[PATCH 1/2] Add error message check in qemuxml2argv tests

[Peter Krempa]

On Tue, Sep 22, 2020 at 11:57:39 +0000, Sebastian Mitterle wrote:
> When an error is expected, the error message will be checked.
> This is expressed by creating an additional ".err" file containing
> the expected error message.
> 
> It is added in order to make sure the expected errors
> are not masked by other errors during test execution while
> leveraging the existing framework.
> 
> In order to keep it simple, an input file cannot be reused
> anymore to cover several expected error cases configured
> in the test code. An input file can still be reused by creating
> a test case specific symlink.
> 
> For consistency, the mock needs to report an error now, too,
> as every failure must have an error; otherwise a test case will
> fail.
> 
> Require LC_ALL=C explicitly to make sure error messages are not
> localized for testing.
> 
> Lastly, remove trailing blank in error message for domain_addr.c
> virDomainCCWAddressAssign, uncovered by this change.
> 
> Signed-off-by: Sebastian Mitterle <smitterl at redhat.com>
> Suggested-by: Peter Krempa <pkrempa at redhat.com>
> ---

[...]

> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 1bfa164a47..6e77a72f7c 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1401,7 +1401,7 @@ virDomainCCWAddressAssign(virDomainDeviceInfoPtr dev,
>  
>          if (virHashLookup(addrs->defined, addr)) {
>              virReportError(VIR_ERR_XML_ERROR,
> -                           _("The CCW devno '%s' is in use already "),
> +                           _("The CCW devno '%s' is in use already"),
>                             addr);
>              return -1;
>          }

This should have been done in a separate commit.

> diff --git a/tests/qemuxml2argvdata/440fx-ide-address-conflict.err b/tests/qemuxml2argvdata/440fx-ide-address-conflict.err
> new file mode 100644
> index 0000000000..36dfc6cd08
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/440fx-ide-address-conflict.err
> @@ -0,0 +1 @@
> +XML error: Attempted double use of PCI Address 0000:00:01.1

[...]

> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> index e5841bc8e3..17be4bedfc 100644
> --- a/tests/qemuxml2argvmock.c
> +++ b/tests/qemuxml2argvmock.c
> @@ -23,6 +23,7 @@
>  #include "vircommand.h"
>  #include "vircrypto.h"
>  #include "virmock.h"
> +#include "virlog.h"
>  #include "virnetdev.h"
>  #include "virnetdevip.h"
>  #include "virnetdevtap.h"
> @@ -91,6 +92,8 @@ virNumaNodesetIsAvailable(virBitmapPtr nodeset)
>          if (virNumaNodeIsAvailable(bit))
>              continue;
>  
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "Mock: no numa node set is available at bit %li.", bit);
>          return false;

This also seems a good candidate for splitting out.

>      }

[...]

> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index e93948e3fc..882a6837b0 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c

[...]

> @@ -615,7 +616,13 @@ testCompareXMLToArgv(const void *data)
>      if (!(vm->def = virDomainDefParseFile(info->infile,
>                                            driver.xmlopt,
>                                            NULL, parseFlags))) {
> -        if (flags & FLAG_EXPECT_PARSE_ERROR)
> +        err = virGetLastError();
> +        if (!err) {
> +            VIR_TEST_DEBUG("no error was reported for expected parse error");

Technically at this point the failure is not yet expected, but the error
should have been reported anyways.

IMO just removing the 'expected' word will be okay, but theoretically
this could be sold as another improvement to the testsuited (checking
whether an error is actually reported when failure is returned).

> +            goto cleanup;
> +        }
> +        if (flags & FLAG_EXPECT_PARSE_ERROR &&
> +            virTestCompareToFile(err->message, info->errfile) >= 0)
>              goto ok;
>          goto cleanup;
>      }
> @@ -651,7 +658,13 @@ testCompareXMLToArgv(const void *data)
>  
>      if (!(cmd = testCompareXMLToArgvCreateArgs(&driver, vm, migrateURI, info,
>                                                 flags, false))) {
> -        if (flags & FLAG_EXPECT_FAILURE)
> +        err = virGetLastError();
> +        if (!err) {
> +            VIR_TEST_DEBUG("no error was reported for expected failure");

Same here.

> +            goto cleanup;
> +        }
> +        if (flags & FLAG_EXPECT_FAILURE &&
> +            virTestCompareToFile(err->message, info->errfile) >= 0)
>              goto ok;
>          goto cleanup;
>      }

I'm going through all the reported errors to see whether they make sense
in terms of the context of the test. Problems there will not be a
problem of this series and we can fix them later.

If nobody objects once I get through the test data I'll split out and
push a version with the stuff above addressed.

Reviewed-by: Peter Krempa <pkrempa at redhat.com>