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