| Message ID | 20230227174019.1164205-1-rjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests: Ensure TAP version is printed before other messages | expand |
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
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
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>
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!
"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.
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);
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.
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>
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
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);
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(-)