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

[Daniel P . Berrangé]

On Wed, Jul 18, 2018 at 10:44:11AM -0300, Philippe Mathieu-Daudé wrote:
> 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.

Yes, accidentally left over from earlier version of patch

> 
> > +
> >  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.

Left over from an earlier version of the patch, I'll cull it.

> 
> >  #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>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|