diff mbox series

[v3,05/11] tests/qtest/qos-test: dump environment variables if verbose

Message ID 8d8b242f38caccd81c27125167862f4457e8a22f.1601655308.git.qemu_oss@crudebyte.com (mailing list archive)
State New, archived
Headers show
Series 9pfs: add tests using local fs driver | expand

Commit Message

Christian Schoenebeck Oct. 2, 2020, 4:15 p.m. UTC
If qtests are run in verbose mode (i.e. if --verbose CL argument
was provided) then print all environment variables to stdout
before running the individual tests.

Instead of using g_test_message() rather use printf() in combination
with g_test_verbose(), to avoid g_test_message() cluttering the
output.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/qos-test.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini Oct. 8, 2020, 12:37 p.m. UTC | #1
On 02/10/20 18:15, Christian Schoenebeck wrote:
> -int main(int argc, char **argv)
> +int main(int argc, char **argv, char** envp)
>  {
>      g_test_init(&argc, &argv, NULL);
> +    if (g_test_verbose()) {
> +        printf("ENVIRONMENT VARIABLES: {\n");
> +        for (char **env = envp; *env != 0; env++) {
> +            printf("\t%s\n", *env);
> +        }
> +        printf("}\n");
> +    }

But doesn't this (and patch 6 as well) break TAP output?  Using
g_test_message + g_test_verbose would be the best of both worlds.

In fact using printf in tests should be forbidden, since glib 2.62 and
newer _always_ emit TAP.

Paolo
Christian Schoenebeck Oct. 8, 2020, 1:09 p.m. UTC | #2
On Donnerstag, 8. Oktober 2020 14:37:00 CEST Paolo Bonzini wrote:
> On 02/10/20 18:15, Christian Schoenebeck wrote:
> > -int main(int argc, char **argv)
> > +int main(int argc, char **argv, char** envp)
> > 
> >  {
> >  
> >      g_test_init(&argc, &argv, NULL);
> > 
> > +    if (g_test_verbose()) {
> > +        printf("ENVIRONMENT VARIABLES: {\n");
> > +        for (char **env = envp; *env != 0; env++) {
> > +            printf("\t%s\n", *env);
> > +        }
> > +        printf("}\n");
> > +    }
> 
> But doesn't this (and patch 6 as well) break TAP output?  Using
> g_test_message + g_test_verbose would be the best of both worlds.

If there was TAP output then yes, patches 4, 5, 6 would probably break it.

How/when is TAP output enabled? I don't see any TAP output by default.

> In fact using printf in tests should be forbidden, since glib 2.62 and
> newer _always_ emit TAP.
> 
> Paolo

The reason why I used printf() was because g_test_message() clutters the 
output tremendously, as it always wraps the text of each g_test_message() call 
into:

	"(MSG: %s)\n"

which is inappropriate for multi-line messages as these proposed patches do.

Is that actually a real-life problem? I mean these patches only output 
anything if --verbose CL switch is used, and I don't see any TAP output 
enabled by default. And the --verbose CL switch is usually just used by 
developers for debugging test case issues, isn't it?

Best regards,
Christian Schoenebeck
Paolo Bonzini Oct. 8, 2020, 1:21 p.m. UTC | #3
On 08/10/20 15:09, Christian Schoenebeck wrote:
>> But doesn't this (and patch 6 as well) break TAP output?  Using
>> g_test_message + g_test_verbose would be the best of both worlds.
> 
> If there was TAP output then yes, patches 4, 5, 6 would probably break it.
> 
> How/when is TAP output enabled? I don't see any TAP output by default.

With "--tap", but with glib 2.62 it will be enabled by default.  For
example on Fedora 32:

  $ ./test-mul64
  # random seed: R02S3efb20d48a41e1897cb761e02393c11b
  1..2
  # Start of host-utils tests
  ok 1 /host-utils/mulu64
  ok 2 /host-utils/muls64
  # End of host-utils tests

I'm okay I guess with using g_test_message on 2.62 or newer, and
assuming people don't use --tap --verbose on older versions.

Paolo

> which is inappropriate for multi-line messages as these proposed patches do.
> 
> Is that actually a real-life problem? I mean these patches only output 
> anything if --verbose CL switch is used, and I don't see any TAP output 
> enabled by default. And the --verbose CL switch is usually just used by 
> developers for debugging test case issues, isn't it?
> 
> Best regards,
> Christian Schoenebeck
> 
>
Christian Schoenebeck Oct. 8, 2020, 1:42 p.m. UTC | #4
On Donnerstag, 8. Oktober 2020 15:21:54 CEST Paolo Bonzini wrote:
> On 08/10/20 15:09, Christian Schoenebeck wrote:
> >> But doesn't this (and patch 6 as well) break TAP output?  Using
> >> g_test_message + g_test_verbose would be the best of both worlds.
> > 
> > If there was TAP output then yes, patches 4, 5, 6 would probably break it.
> > 
> > How/when is TAP output enabled? I don't see any TAP output by default.
> 
> With "--tap", but with glib 2.62 it will be enabled by default.  For
> example on Fedora 32:
> 
>   $ ./test-mul64
>   # random seed: R02S3efb20d48a41e1897cb761e02393c11b
>   1..2
>   # Start of host-utils tests
>   ok 1 /host-utils/mulu64
>   ok 2 /host-utils/muls64
>   # End of host-utils tests
> 
> I'm okay I guess with using g_test_message on 2.62 or newer, and
> assuming people don't use --tap --verbose on older versions.

Simpler solution: just appending '#' character in front of each printf() line, 
that would be both fine for TAP and regular output:
http://testanything.org/tap-specification.html#diagnostics

Unfortunately 'test_tap_log' is a private variable in glib (gtestutils.c), 
otherwise I could have made that conditionally. There is no getter function in 
the glib API for this (TAP on/off) variable.

I could check the CL for --verbose somewhere, but I think that's probably 
overkill.

Best regards,
Christian Schoenebeck
Paolo Bonzini Oct. 8, 2020, 1:52 p.m. UTC | #5
On 08/10/20 15:42, Christian Schoenebeck wrote:
>>
>> I'm okay I guess with using g_test_message on 2.62 or newer, and
>> assuming people don't use --tap --verbose on older versions.
> Simpler solution: just appending '#' character in front of each printf() line, 
> that would be both fine for TAP and regular output:
> http://testanything.org/tap-specification.html#diagnostics

I'm not sure how it would be simpler than a

#if !GLIB_CHECK_VERSION(2, 62, 0)
#define qemu_test_message printf
#else
#define qemu_test_message g_test_message
#endif

but you choose.

Paolo
Christian Schoenebeck Oct. 8, 2020, 3:38 p.m. UTC | #6
On Donnerstag, 8. Oktober 2020 15:52:08 CEST Paolo Bonzini wrote:
> On 08/10/20 15:42, Christian Schoenebeck wrote:
> >> I'm okay I guess with using g_test_message on 2.62 or newer, and
> >> assuming people don't use --tap --verbose on older versions.
> > 
> > Simpler solution: just appending '#' character in front of each printf()
> > line, that would be both fine for TAP and regular output:
> > http://testanything.org/tap-specification.html#diagnostics
> 
> I'm not sure how it would be simpler than a
> 
> #if !GLIB_CHECK_VERSION(2, 62, 0)
> #define qemu_test_message printf
> #else
> #define qemu_test_message g_test_message
> #endif
> 
> but you choose.
> 
> Paolo

Simple yes, but it would not fix the cluttered output problem of 
g_test_message().

So I'll go with prepending '#' for now, and if one day there will be a public 
glib function to check for TAP mode, it can easily be adjusted.

Best regards,
Christian Schoenebeck
diff mbox series

Patch

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index d98ef78613..fe240b32a7 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -313,9 +313,16 @@  static void walk_path(QOSGraphNode *orig_path, int len)
  *   machine/drivers/test objects
  * - Cleans up everything
  */
-int main(int argc, char **argv)
+int main(int argc, char **argv, char** envp)
 {
     g_test_init(&argc, &argv, NULL);
+    if (g_test_verbose()) {
+        printf("ENVIRONMENT VARIABLES: {\n");
+        for (char **env = envp; *env != 0; env++) {
+            printf("\t%s\n", *env);
+        }
+        printf("}\n");
+    }
     qos_graph_init();
     module_call_init(MODULE_INIT_QOM);
     module_call_init(MODULE_INIT_LIBQOS);