Re: [Qemu-devel] [PATCH for 3.0 2/4] tests: don't silence error reporting for all tests

[Philippe Mathieu-Daudé]

Hi Daniel,

On 07/18/2018 06:38 AM, Daniel P. Berrangé wrote:
> The test-vmstate test is a bit chatty because it triggers various
> expected failure scenarios and the code in question uses error_report
> instead of accepting 'Error **errp' parameters. To silence this test the
> stubs for error_vprintf() were changed to send errors via
> g_test_message() instead of stderr:
> 
>   commit 28017e010ddf6849cfa830e898da3e44e6610952
>   Author: Paolo Bonzini <address@hidden>
>   Date:   Mon Oct 24 18:31:03 2016 +0200
> 
>     tests: send error_report to test log
> 
>     Implement error_vprintf to send the output of error_report to
>     the test log.  This silences test-vmstate.
> 
>     Signed-off-by: Paolo Bonzini <address@hidden>
>     Message-Id: <address@hidden>
> 
> Unfortunately this change has global impact across the entire test suite
> and means that when tests fail for unexpected reasons, the message is
> not displayed on stderr. eg when using &error_abort in a call the test
> merely prints
> 
>   Unexpected error in qcrypto_tls_session_check_certificate() at 
> crypto/tlssession.c:280:
> 
> and the actual error message is hidden, making it impossible to diagnose
> the failure. This is especially problematic in CI or build systems where
> it isn't possible to easily pass the --debug-log flag to tests and
> re-run with the test log visible.
> 
> This change makes the previous big hammer much more nuanced, providing a
> flag in the stub error_vprintf() that can used on a per-test basis to
> silence the errors. Only the test-vmstate silences errors initially.
> 
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>  stubs/error-printf.c | 5 ++++-
>  tests/test-vmstate.c | 3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> index ac6b92aa69..2199d79d28 100644
> --- a/stubs/error-printf.c
> +++ b/stubs/error-printf.c
> @@ -2,9 +2,12 @@
>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
>  
> +bool silence_test_errors;

This is not used.

> +
>  void error_vprintf(const char *fmt, va_list ap)
>  {
> -    if (g_test_initialized() && !g_test_subprocess()) {
> +    if (g_test_initialized() && !g_test_subprocess() &&
> +        getenv("QTEST_SILENT_ERRORS")) {
>          char *msg = g_strdup_vprintf(fmt, ap);
>          g_test_message("%s", msg);
>          g_free(msg);
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index 087844b6c8..42923bb1df 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -32,6 +32,7 @@
>  #include "../migration/qemu-file-channel.h"
>  #include "../migration/savevm.h"
>  #include "qemu/coroutine.h"
> +#include "qemu/error-report.h"

Why? This doesn't seem necessary, neither related to this patch.

>  #include "io/channel-file.h"
>  
>  static char temp_file[] = "/tmp/vmst.test.XXXXXX";
> @@ -859,6 +860,8 @@ int main(int argc, char **argv)
>  
>      module_call_init(MODULE_INIT_QOM);
>  
> +    setenv("QTEST_SILENT_ERRORS", "1", 1);
> +
>      g_test_init(&argc, &argv, NULL);
>      g_test_add_func("/vmstate/simple/primitive", test_simple_primitive);
>      g_test_add_func("/vmstate/versioned/load/v1", test_load_v1);
> 

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
Tested-by: Philippe Mathieu-Daudé <address@hidden>