[Libosinfo] [PATCH] Enhance test-isodetect: continue after failure

[Fabiano Fidêncio]

Věra,

I, personally, would split this patch in 2 different ones:
- The first patch I'd just move from using `while (tmp)` to using `for
(...)` ...
- Then, in the second patch I'd introduce the changes you did;

Would you mind to do that?

Now, let's go for the review of the changes provided ...

On Mon, Oct 15, 2018 at 7:50 PM Věra Cholasta <vbudikov at redhat.com> wrote:
>
> ---
>  tests/test-isodetect.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/tests/test-isodetect.c b/tests/test-isodetect.c
> index 76f0c5a..f40f5cc 100644
> --- a/tests/test-isodetect.c
> +++ b/tests/test-isodetect.c
> @@ -390,17 +390,16 @@ static void test_one(const gchar *vendor)
>
>      g_assert_nonnull(isos);
>
> -    tmp = isos;
> -    while (tmp) {
> +    for (tmp = isos; tmp; tmp = tmp->next) {
>          struct ISOInfo *info  = tmp->data;
>          gboolean matched = osinfo_db_identify_media(db, info->media);
>          OsinfoOs *os;
>
>          g_test_message("checking OS %s for ISO %s",
>                         info->shortid, info->filename);
> +        g_assert_true(matched);
>          if (!matched) {
> -            g_error("ISO %s was not matched by OS %s",
> -                    info->filename, info->shortid);

I would not entirely remove the message as this is what gives us the
info about what's the exact file that's causing the issue.
For instance, let's see how the logs look like without this message:
/isodetect/sled: **
ERROR:test-isodetect.c:400:test_one: 'matched' should be TRUE
**
ERROR:test-isodetect.c:400:test_one: 'matched' should be TRUE
**
ERROR:test-isodetect.c:400:test_one: 'matched' should be TRUE
...

While I'm assuming that what you want to achieve is something like:
/isodetect/sled: **
ERROR:test-isodetect.c:400:test_one: 'matched' should be TRUE
** Message: ISO SLED-11-SP1-DVD-i586-GM-DVD1.iso.txt was not matched
by OS sled11sp1
**
ERROR:test-isodetect.c:400:test_one: 'matched' should be TRUE
** Message: ISO SLED-11-SP1-DVD-x86_64-GM-DVD1.iso.txt was not matched
by OS sled11sp1
**
ERROR:test-isodetect.c:400:test_one: 'matched' should be TRUE
** Message: ISO SLE-12-Desktop-DVD-x86_64-GM-DVD1.iso.txt was not
matched by OS sled12
....

In order to achieve so, I'd suggest you to just tone down the
g_error() (which would cause an abort) to g_message().
What do you think?.

> +            continue;
>          }
>
>          g_object_get(info->media, "os", &os, NULL);
> @@ -408,8 +407,6 @@ static void test_one(const gchar *vendor)
>          g_assert_cmpstr(shortid, ==, info->shortid);
>          g_object_unref(G_OBJECT(os));
>          test_langs(info);
> -
> -        tmp = tmp->next;
>      }
>
>      g_list_foreach(isos, (GFunc)free_iso, NULL);
> @@ -422,6 +419,7 @@ int
>  main(int argc, char *argv[])
>  {
>      g_test_init(&argc, &argv, NULL);
> +    g_test_set_nonfatal_assertions();

This API has only been introduced as part of GLib 2.38.
It means that:
- make check will just break with the following warning:
test-isodetect.c: In function 'main':
test-isodetect.c:424:5: error: 'g_test_set_nonfatal_assertions' is
deprecated (declared at /usr/include/glib-2.0/glib/gtestutils.h:182):
Not available before 2.38 [-Werror=deprecated-declarations]
     g_test_set_nonfatal_assertions();
     ^
cc1: all warnings being treated as errors

In order to solve this, you'd have to bump GLIB_MINIMUM_VERSION (and
GLIB_ENCODED_VERSION) from 2.36 to 2.38.

When doing those bumps, we'd have to check whether the most
common/used distros (as ubuntu LTS, suse, debian, rhel) support the
version we're going to require. I'd say it's not a problem for us in
this case as libvirt-glib already requires 2.38.

>
>      GList *vendors = load_vendors(NULL);
>      GList *it;
> --
> 1.8.3.1
>
> _______________________________________________
> Libosinfo mailing list
> Libosinfo at redhat.com
> https://www.redhat.com/mailman/listinfo/libosinfo

Please, let me know in case something is not clear and thanks for your
contribution!
-- 
Fabiano Fidêncio