[Libosinfo] [libosinfo] tests: Add test-os-resources

[Fabiano Fidêncio]

On Tue, Apr 10, 2018 at 12:29 PM, Christophe Fergeau
<cfergeau at redhat.com> wrote:
> On Fri, Nov 17, 2017 at 03:53:27PM +0100, Fabiano Fidêncio wrote:
>> test-os-resources has been written to avoid bug as having minimum
>> resources greater than the recommended resources in osinfo-db.
>>
>> It has been hitting (without any harm no far) by RHEL/CentOS data files,
>> which caused so really bad UI effect on clients using those two
>> attributes (as in GNOME Boxes).
>>
>> Signed-off-by: Fabiano Fidêncio <fabiano at fidencio.org>
>> ---
>>  tests/Makefile.am         |   5 ++
>>  tests/test-os-resources.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 189 insertions(+)
>>  create mode 100644 tests/test-os-resources.c
>>
>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>> index 7566d3c..06f81bf 100644
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -17,6 +17,7 @@ check_PROGRAMS = \
>>       test-loader \
>>       test-isodetect \
>>       test-install-script \
>> +     test-os-resources \
>>          $(NULL)
>>
>>  if HAVE_CURL
>> @@ -111,6 +112,10 @@ test_install_script_LDADD = $(COMMON_LDADD)
>>  test_install_script_CFLAGS = $(COMMON_CFLAGS)
>>  test_install_script_SOURCES = test-install-script.c
>>
>> +test_os_resources_LDADD = $(COMMON_LDADD)
>> +test_os_resources_CFLAGS = $(COMMON_CFLAGS)
>> +test_os_resources_SOURCES = test-os-resources.c
>> +
>>  TESTS = $(check_PROGRAMS) \
>>       $(NULL)
>>
>> diff --git a/tests/test-os-resources.c b/tests/test-os-resources.c
>> new file mode 100644
>> index 0000000..9942d9c
>> --- /dev/null
>> +++ b/tests/test-os-resources.c
>> @@ -0,0 +1,184 @@
>> +/*
>> + * Copyright (C) 2017 Fabiano Fidêncio
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, write to the Free Software Foundation,
>> + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
>> + *
>> + * Authors:
>> + *   Fabiano Fidêncio <fabiano at fidencio.org>
>> + */
>> +
>> +#include <config.h>
>> +
>> +#include <stdlib.h>
>> +#include <osinfo/osinfo.h>
>> +
>> +
>> +static void test_n_cpus(OsinfoResources *minimum, OsinfoResources *recommended)
>> +{
>> +    gint minimum_cpus, recommended_cpus;
>> +
>> +    minimum_cpus = osinfo_resources_get_n_cpus(minimum);
>> +    recommended_cpus = osinfo_resources_get_n_cpus(recommended);
>> +
>> +    if (recommended_cpus >= 0 && minimum_cpus >= 0)
>> +        g_assert_true(recommended_cpus >= minimum_cpus);
>
> Is it 0 if unspecified in the XML? Aren't we going to miss the case when
> a minimum CPU value is specified, but not a recommended CPU value? or is
> it fine in this case? Same question for the other tests.

If it's unspecified in the XML we get -1.
The apps using libosinfo should be aware of this (it's documented) and
use the minimum in case the recommended is not found.

Does this answer your question?

>
>> +static void
>> +test_minimum_recommended_resources(void)
>> +{
>> +    OsinfoLoader *loader = osinfo_loader_new();
>> +    OsinfoDb *db = osinfo_loader_get_db(loader);
>> +    OsinfoOsList *oslist;
>> +    GList *oses;
>> +    GError *error = NULL;
>> +
>> +    g_assert_true(OSINFO_IS_LOADER(loader));
>> +    g_assert_true(OSINFO_IS_DB(db));
>> +
>> +    osinfo_loader_process_default_path(loader, &error);
>> +    g_assert_no_error(error);
>> +
>> +    oslist = osinfo_db_get_os_list(db);
>> +    oses = osinfo_list_get_elements(OSINFO_LIST(oslist));
>> +
>> +    for (; oses != NULL; oses = oses->next) {
>> +        OsinfoOs *os = oses->data;
>> +        OsinfoResourcesList *minimum_list, *recommended_list;
>> +        GList *minimum_resources, *recommended_resources;
>> +        const gchar *minimum_arch, * recommended_arch;
>> +
>> +        minimum_list = osinfo_os_get_minimum_resources(os);
>> +        minimum_resources = osinfo_list_get_elements(OSINFO_LIST(minimum_list));
>> +
>> +        recommended_list = osinfo_os_get_recommended_resources(os);
>> +        recommended_resources = osinfo_list_get_elements(OSINFO_LIST(recommended_list));
>> +
>> +        /* That's fine as not all OSes have those fields filled */
>> +        if (minimum_resources == NULL || recommended_resources == NULL)
>> +            continue;
>> +
>> +        for (; minimum_resources != NULL;
>> +                minimum_resources = minimum_resources->next) {
>> +            OsinfoResources *minimum = minimum_resources->data;
>> +            GList *tmp = recommended_resources;
>> +
>> +            minimum_arch = osinfo_resources_get_architecture(minimum);
>> +
>> +
>
> Maybe get rid of one of the extra spaces here

Done!

>
>> +            for (; tmp != NULL; tmp = tmp->next) {
>> +                OsinfoResources *recommended = tmp->data;
>> +
>> +                recommended_arch = osinfo_resources_get_architecture(recommended);
>> +
>> +                 if (g_str_equal(minimum_arch, recommended_arch)) {
>> +                    const gchar *name;
>> +
>> +                    name = osinfo_product_get_name(OSINFO_PRODUCT(os));
>> +
>> +                    g_test_message("checking %s (architecture: %s)",
>> +                                   name, minimum_arch);
>
> Is there a way to get this message to be displayed? I only could get it
> through --debug-log, but this seems a bit too low-level. I could not
> find it in the test logs.

One thing that could be done (and not only for this test) would be to
use g_debug() instead of g_test_message().
For now, I'd stick with the pattern seen/used in the other tests (just
using g_test_message()).

>
>> +
>> +                    test_n_cpus(minimum, recommended);
>> +                    test_cpu(minimum, recommended);
>> +                    test_ram(minimum, recommended);
>> +                    test_storage(minimum, recommended);
>> +                    break;
>> +                }
>> +            }
>> +        }
>> +
>> +        g_object_unref(minimum_list);
>> +        g_object_unref(recommended_list);
>> +    }
>> +
>> +    if (oslist)
>> +        g_object_unref(oslist);
>
> Higher up in the function, you did:
>
> +    oslist = osinfo_db_get_os_list(db);
> +    oses = osinfo_list_get_elements(OSINFO_LIST(oslist));
>
> I would expect the test to have failed at this point if 'oslist' is
> NULL. In other word, I think you can just remove the if (oslist) check.

True, done!

>
> I fixed a few leaks in that function, see attached patch.

I've squashed your patch into mine.

>
>> +
>> +    g_object_unref(loader);
>> +}
>> +
>> +
>> +int
>> +main(int argc, char *argv[])
>> +{
>> +    int ret;
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    g_test_add_func("/os/resources/minimum_recommended", test_minimum_recommended_resources);
>> +
>> +    /* Make sure we catch unexpected g_warning() */
>> +    g_log_set_always_fatal(G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL | G_LOG_LEVEL_WARNING);
>
> Nope, this is not needed when using GTest, actually I have a patch on
> the mailing list removing it from all the tests which is waiting for a
> review ;)

I've merged your patch a month or so ago. :-)
Unfortunately my patch was sent before yours was merged. Anyways, fixed.

>
> Apart from these comments, looks good to me.
>
> Christophe

I'll submit a v2 soon. Thanks for the review.

-- 
Fabiano Fidêncio