[PATCH] tests: Ensure TAP version is printed before other messages

[PATCH] tests: Ensure TAP version is printed before other messages

[Richard W.M. Jones]

These two tests were failing with this error:

  stderr:
  TAP parsing error: version number must be on the first line
  [...]
  Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.

This can be fixed by ensuring we always call g_test_init first in the
body of main.

Thanks: Daniel Berrange, for diagnosing the problem
Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 tests/qtest/fuzz-lsi53c895a-test.c | 4 ++--
 tests/qtest/rtl8139-test.c         | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
index a9254b455d..2012bd54b7 100644
--- a/tests/qtest/fuzz-lsi53c895a-test.c
+++ b/tests/qtest/fuzz-lsi53c895a-test.c
@@ -112,12 +112,12 @@ static void test_lsi_do_dma_empty_queue(void)
 
 int main(int argc, char **argv)
 {
+    g_test_init(&argc, &argv, NULL);
+
     if (!qtest_has_device("lsi53c895a")) {
         return 0;
     }
 
-    g_test_init(&argc, &argv, NULL);
-
     qtest_add_func("fuzz/lsi53c895a/lsi_do_dma_empty_queue",
                    test_lsi_do_dma_empty_queue);
 
diff --git a/tests/qtest/rtl8139-test.c b/tests/qtest/rtl8139-test.c
index 1beb83805c..4bd240e9ee 100644
--- a/tests/qtest/rtl8139-test.c
+++ b/tests/qtest/rtl8139-test.c
@@ -207,9 +207,10 @@ int main(int argc, char **argv)
         verbosity_level = atoi(v_env);
     }
 
-    qtest_start("-device rtl8139");
-
     g_test_init(&argc, &argv, NULL);
+
+    qtest_start("-device rtl8139");
+
     qtest_add_func("/rtl8139/nop", nop);
     qtest_add_func("/rtl8139/timer", test_init);
 
-- 
2.39.2

Re: [PATCH] tests: Ensure TAP version is printed before other messages

[Daniel P. Berrangé]

On Mon, Feb 27, 2023 at 05:40:19PM +0000, Richard W.M. Jones wrote:
> These two tests were failing with this error:
> 
>   stderr:
>   TAP parsing error: version number must be on the first line
>   [...]
>   Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.
> 
> This can be fixed by ensuring we always call g_test_init first in the
> body of main.
> 
> Thanks: Daniel Berrange, for diagnosing the problem
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  tests/qtest/fuzz-lsi53c895a-test.c | 4 ++--
>  tests/qtest/rtl8139-test.c         | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> 
> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
> index a9254b455d..2012bd54b7 100644
> --- a/tests/qtest/fuzz-lsi53c895a-test.c
> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
> @@ -112,12 +112,12 @@ static void test_lsi_do_dma_empty_queue(void)
>  
>  int main(int argc, char **argv)
>  {
> +    g_test_init(&argc, &argv, NULL);
> +
>      if (!qtest_has_device("lsi53c895a")) {
>          return 0;
[>      }
>  
> -    g_test_init(&argc, &argv, NULL);
> -
>      qtest_add_func("fuzz/lsi53c895a/lsi_do_dma_empty_queue",
>                     test_lsi_do_dma_empty_queue);
>  
> diff --git a/tests/qtest/rtl8139-test.c b/tests/qtest/rtl8139-test.c
> index 1beb83805c..4bd240e9ee 100644
> --- a/tests/qtest/rtl8139-test.c
> +++ b/tests/qtest/rtl8139-test.c
> @@ -207,9 +207,10 @@ int main(int argc, char **argv)
>          verbosity_level = atoi(v_env);
>      }
>  
> -    qtest_start("-device rtl8139");
> -
>      g_test_init(&argc, &argv, NULL);
> +
> +    qtest_start("-device rtl8139");

As a more general point I find it a little dubious that we spawn
QEMU from main() outside the context of the test case. I guess
that makes it slightly more efficient since we have one QEMU for
both test cases, but it is feels slightly oddball and obviously
leads to bugs like this one we see.

> +
>      qtest_add_func("/rtl8139/nop", nop);
>      qtest_add_func("/rtl8139/timer", test_init);
>  
> -- 
> 2.39.2
> 

With 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 :|

Re: [PATCH] tests: Ensure TAP version is printed before other messages

[Darren Kenny]

On Monday, 2023-02-27 at 17:40:19 UTC, Richard W.M. Jones wrote:
> These two tests were failing with this error:
>
>   stderr:
>   TAP parsing error: version number must be on the first line
>   [...]
>   Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.
>
> This can be fixed by ensuring we always call g_test_init first in the
> body of main.
>
> Thanks: Daniel Berrange, for diagnosing the problem
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

> ---
>  tests/qtest/fuzz-lsi53c895a-test.c | 4 ++--
>  tests/qtest/rtl8139-test.c         | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
> index a9254b455d..2012bd54b7 100644
> --- a/tests/qtest/fuzz-lsi53c895a-test.c
> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
> @@ -112,12 +112,12 @@ static void test_lsi_do_dma_empty_queue(void)
>  
>  int main(int argc, char **argv)
>  {
> +    g_test_init(&argc, &argv, NULL);
> +
>      if (!qtest_has_device("lsi53c895a")) {
>          return 0;
>      }
>  
> -    g_test_init(&argc, &argv, NULL);
> -
>      qtest_add_func("fuzz/lsi53c895a/lsi_do_dma_empty_queue",
>                     test_lsi_do_dma_empty_queue);
>  
> diff --git a/tests/qtest/rtl8139-test.c b/tests/qtest/rtl8139-test.c
> index 1beb83805c..4bd240e9ee 100644
> --- a/tests/qtest/rtl8139-test.c
> +++ b/tests/qtest/rtl8139-test.c
> @@ -207,9 +207,10 @@ int main(int argc, char **argv)
>          verbosity_level = atoi(v_env);
>      }
>  
> -    qtest_start("-device rtl8139");
> -
>      g_test_init(&argc, &argv, NULL);
> +
> +    qtest_start("-device rtl8139");
> +
>      qtest_add_func("/rtl8139/nop", nop);
>      qtest_add_func("/rtl8139/timer", test_init);
>  
> -- 
> 2.39.2

Re: [PATCH] tests: Ensure TAP version is printed before other messages

[Alexander Bulekov]

On 230227 1740, Richard W.M. Jones wrote:
> These two tests were failing with this error:
> 
>   stderr:
>   TAP parsing error: version number must be on the first line
>   [...]
>   Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.
> 
> This can be fixed by ensuring we always call g_test_init first in the
> body of main.
> 
> Thanks: Daniel Berrange, for diagnosing the problem
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>

Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

Re: [PATCH] tests: Ensure TAP version is printed before other messages

[Philippe Mathieu-Daudé]

On 27/2/23 18:40, Richard W.M. Jones wrote:
> These two tests were failing with this error:
> 
>    stderr:
>    TAP parsing error: version number must be on the first line
>    [...]
>    Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.
> 
> This can be fixed by ensuring we always call g_test_init first in the
> body of main.
> 
> Thanks: Daniel Berrange, for diagnosing the problem
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>   tests/qtest/fuzz-lsi53c895a-test.c | 4 ++--
>   tests/qtest/rtl8139-test.c         | 5 +++--
>   2 files changed, 5 insertions(+), 4 deletions(-)

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks for tackling this!

Re: [PATCH] tests: Ensure TAP version is printed before other messages

[Alex Bennée]

"Richard W.M. Jones" <rjones@redhat.com> writes:

> These two tests were failing with this error:
>
>   stderr:
>   TAP parsing error: version number must be on the first line
>   [...]
>   Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.
>
> This can be fixed by ensuring we always call g_test_init first in the
> body of main.
>
> Thanks: Daniel Berrange, for diagnosing the problem
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>

Queued to testing/next, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] tests: Ensure TAP version is printed before other messages

[Thomas Huth]

On 27/02/2023 18.40, Richard W.M. Jones wrote:
> These two tests were failing with this error:
> 
>    stderr:
>    TAP parsing error: version number must be on the first line
>    [...]
>    Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.
> 
> This can be fixed by ensuring we always call g_test_init first in the
> body of main.
> 
> Thanks: Daniel Berrange, for diagnosing the problem
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>   tests/qtest/fuzz-lsi53c895a-test.c | 4 ++--
>   tests/qtest/rtl8139-test.c         | 5 +++--
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
> index a9254b455d..2012bd54b7 100644
> --- a/tests/qtest/fuzz-lsi53c895a-test.c
> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
> @@ -112,12 +112,12 @@ static void test_lsi_do_dma_empty_queue(void)
>   
>   int main(int argc, char **argv)
>   {
> +    g_test_init(&argc, &argv, NULL);
> +
>       if (!qtest_has_device("lsi53c895a")) {
>           return 0;

Could you please double-check that the !lsi53c895a case works fine, too? 
(just temporarily change it into a "if (1) { ..." statement) ... I'm a 
little bit afraid that the TAP protocol might be incomplete without the 
g_test_run() at the end otherwise. If so, you might now need a "goto out" 
instead of the "return 0" here...

  Thomas


>       }
>   
> -    g_test_init(&argc, &argv, NULL);

Re: [PATCH] tests: Ensure TAP version is printed before other messages

[Richard W.M. Jones]

On Tue, Feb 28, 2023 at 09:30:56PM +0100, Thomas Huth wrote:
> On 27/02/2023 18.40, Richard W.M. Jones wrote:
> >These two tests were failing with this error:
> >
> >   stderr:
> >   TAP parsing error: version number must be on the first line
> >   [...]
> >   Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.
> >
> >This can be fixed by ensuring we always call g_test_init first in the
> >body of main.
> >
> >Thanks: Daniel Berrange, for diagnosing the problem
> >Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> >---
> >  tests/qtest/fuzz-lsi53c895a-test.c | 4 ++--
> >  tests/qtest/rtl8139-test.c         | 5 +++--
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> >
> >diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
> >index a9254b455d..2012bd54b7 100644
> >--- a/tests/qtest/fuzz-lsi53c895a-test.c
> >+++ b/tests/qtest/fuzz-lsi53c895a-test.c
> >@@ -112,12 +112,12 @@ static void test_lsi_do_dma_empty_queue(void)
> >  int main(int argc, char **argv)
> >  {
> >+    g_test_init(&argc, &argv, NULL);
> >+
> >      if (!qtest_has_device("lsi53c895a")) {
> >          return 0;
> 
> Could you please double-check that the !lsi53c895a case works fine,
> too? (just temporarily change it into a "if (1) { ..." statement)
> ... I'm a little bit afraid that the TAP protocol might be
> incomplete without the g_test_run() at the end otherwise. If so, you
> might now need a "goto out" instead of the "return 0" here...

Applying ...

diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
index 2012bd54b7..e0c902aac4 100644
--- a/tests/qtest/fuzz-lsi53c895a-test.c
+++ b/tests/qtest/fuzz-lsi53c895a-test.c
@@ -114,7 +114,7 @@ int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
-    if (!qtest_has_device("lsi53c895a")) {
+    if (1) {
         return 0;
     }
 
... and rerunning the tests, everything still passes.

The stdout of the test after this change is:

TAP version 13
# random seed: R02S1c1f371a09fbfdf0dd747f898d55fe97

but apparently this version of TAP doesn't care (perhaps because the
number of tests "1..2" is never printed?)

Anyway it doesn't appear to be a problem.

glib2-2.75.3-4.fc39.x86_64

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [PATCH] tests: Ensure TAP version is printed before other messages

[Thomas Huth]

On 01/03/2023 11.52, Richard W.M. Jones wrote:
> On Tue, Feb 28, 2023 at 09:30:56PM +0100, Thomas Huth wrote:
>> On 27/02/2023 18.40, Richard W.M. Jones wrote:
>>> These two tests were failing with this error:
>>>
>>>    stderr:
>>>    TAP parsing error: version number must be on the first line
>>>    [...]
>>>    Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.
>>>
>>> This can be fixed by ensuring we always call g_test_init first in the
>>> body of main.
>>>
>>> Thanks: Daniel Berrange, for diagnosing the problem
>>> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
>>> ---
>>>   tests/qtest/fuzz-lsi53c895a-test.c | 4 ++--
>>>   tests/qtest/rtl8139-test.c         | 5 +++--
>>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
>>> index a9254b455d..2012bd54b7 100644
>>> --- a/tests/qtest/fuzz-lsi53c895a-test.c
>>> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
>>> @@ -112,12 +112,12 @@ static void test_lsi_do_dma_empty_queue(void)
>>>   int main(int argc, char **argv)
>>>   {
>>> +    g_test_init(&argc, &argv, NULL);
>>> +
>>>       if (!qtest_has_device("lsi53c895a")) {
>>>           return 0;
>>
>> Could you please double-check that the !lsi53c895a case works fine,
>> too? (just temporarily change it into a "if (1) { ..." statement)
>> ... I'm a little bit afraid that the TAP protocol might be
>> incomplete without the g_test_run() at the end otherwise. If so, you
>> might now need a "goto out" instead of the "return 0" here...
> 
> Applying ...
> 
> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
> index 2012bd54b7..e0c902aac4 100644
> --- a/tests/qtest/fuzz-lsi53c895a-test.c
> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
> @@ -114,7 +114,7 @@ int main(int argc, char **argv)
>   {
>       g_test_init(&argc, &argv, NULL);
>   
> -    if (!qtest_has_device("lsi53c895a")) {
> +    if (1) {
>           return 0;
>       }
>   
> ... and rerunning the tests, everything still passes.
> 
> The stdout of the test after this change is:
> 
> TAP version 13
> # random seed: R02S1c1f371a09fbfdf0dd747f898d55fe97
> 
> but apparently this version of TAP doesn't care (perhaps because the
> number of tests "1..2" is never printed?)
> 
> Anyway it doesn't appear to be a problem.

Ok, thanks for checking! I just also checked 
https://testanything.org/tap-version-13-specification.html and it says "The 
plan is optional", so this should be fine.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] tests: Ensure TAP version is printed before other messages

[Daniel P. Berrangé]

On Wed, Mar 01, 2023 at 10:52:14AM +0000, Richard W.M. Jones wrote:
> On Tue, Feb 28, 2023 at 09:30:56PM +0100, Thomas Huth wrote:
> > On 27/02/2023 18.40, Richard W.M. Jones wrote:
> > >These two tests were failing with this error:
> > >
> > >   stderr:
> > >   TAP parsing error: version number must be on the first line
> > >   [...]
> > >   Unknown TAP version. The first line MUST be `TAP version <int>`. Assuming version 12.
> > >
> > >This can be fixed by ensuring we always call g_test_init first in the
> > >body of main.
> > >
> > >Thanks: Daniel Berrange, for diagnosing the problem
> > >Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > >---
> > >  tests/qtest/fuzz-lsi53c895a-test.c | 4 ++--
> > >  tests/qtest/rtl8139-test.c         | 5 +++--
> > >  2 files changed, 5 insertions(+), 4 deletions(-)
> > >
> > >diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
> > >index a9254b455d..2012bd54b7 100644
> > >--- a/tests/qtest/fuzz-lsi53c895a-test.c
> > >+++ b/tests/qtest/fuzz-lsi53c895a-test.c
> > >@@ -112,12 +112,12 @@ static void test_lsi_do_dma_empty_queue(void)
> > >  int main(int argc, char **argv)
> > >  {
> > >+    g_test_init(&argc, &argv, NULL);
> > >+
> > >      if (!qtest_has_device("lsi53c895a")) {
> > >          return 0;
> > 
> > Could you please double-check that the !lsi53c895a case works fine,
> > too? (just temporarily change it into a "if (1) { ..." statement)
> > ... I'm a little bit afraid that the TAP protocol might be
> > incomplete without the g_test_run() at the end otherwise. If so, you
> > might now need a "goto out" instead of the "return 0" here...
> 
> Applying ...
> 
> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
> index 2012bd54b7..e0c902aac4 100644
> --- a/tests/qtest/fuzz-lsi53c895a-test.c
> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
> @@ -114,7 +114,7 @@ int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
>  
> -    if (!qtest_has_device("lsi53c895a")) {
> +    if (1) {
>          return 0;
>      }
>  
> ... and rerunning the tests, everything still passes.
> 
> The stdout of the test after this change is:
> 
> TAP version 13
> # random seed: R02S1c1f371a09fbfdf0dd747f898d55fe97
> 
> but apparently this version of TAP doesn't care (perhaps because the
> number of tests "1..2" is never printed?)

Right, the number of tests cannot be printed by g_test_init as the
tests haven't been registered yet. This will only get run in thue
g_test_run.

I recall sometime in the past I believe we've seen problems with
tests that exit without printing anything, but if that's a problem
it would be pre-existing with this test case as written.

The TAP spec:

   https://testanything.org/tap-version-13-specification.html

says the test plan (aka the '1..2' bit) is optional:

  "The plan is optional but if there is a plan before the
   test points it must be the first non-diagnostic line
   output by the test file."

So having merely the "TAP version 13" should be sufficient,
but then earlier glib doesn't print this at all. As I say
though, the existing test would already suffer from the
problem if it mattered.

> Anyway it doesn't appear to be a problem.

Yep, I think we are probably ok.

With 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 :|