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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxx>
> + */
> +
> +#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.

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

> +            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.

> +
> +                    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.

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

> +
> +    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 ;)

Apart from these comments, looks good to me.

Christophe
diff --git a/tests/test-os-resources.c b/tests/test-os-resources.c
index 9942d9c6..07aac0a6 100644
--- a/tests/test-os-resources.c
+++ b/tests/test-os-resources.c
@@ -79,6 +79,7 @@ test_minimum_recommended_resources(void)
     OsinfoDb *db = osinfo_loader_get_db(loader);
     OsinfoOsList *oslist;
     GList *oses;
+    GList *oses_it;
     GError *error = NULL;
 
     g_assert_true(OSINFO_IS_LOADER(loader));
@@ -90,10 +91,11 @@ test_minimum_recommended_resources(void)
     oslist = osinfo_db_get_os_list(db);
     oses = osinfo_list_get_elements(OSINFO_LIST(oslist));
 
-    for (; oses != NULL; oses = oses->next) {
+    for (oses_it = oses; oses_it != NULL; oses_it = oses_it->next) {
         OsinfoOs *os = oses->data;
         OsinfoResourcesList *minimum_list, *recommended_list;
         GList *minimum_resources, *recommended_resources;
+        GList *resources_it;
         const gchar *minimum_arch, * recommended_arch;
 
         minimum_list = osinfo_os_get_minimum_resources(os);
@@ -104,11 +106,10 @@ test_minimum_recommended_resources(void)
 
         /* That's fine as not all OSes have those fields filled */
         if (minimum_resources == NULL || recommended_resources == NULL)
-            continue;
+            goto next;
 
-        for (; minimum_resources != NULL;
-                minimum_resources = minimum_resources->next) {
-            OsinfoResources *minimum = minimum_resources->data;
+        for (resources_it = minimum_resources; resources_it != NULL; resources_it = resources_it->next) {
+            OsinfoResources *minimum = resources_it->data;
             GList *tmp = recommended_resources;
 
             minimum_arch = osinfo_resources_get_architecture(minimum);
@@ -136,12 +137,16 @@ test_minimum_recommended_resources(void)
             }
         }
 
+next:
+        g_list_free(minimum_resources);
+        g_list_free(recommended_resources);
         g_object_unref(minimum_list);
         g_object_unref(recommended_list);
     }
 
     if (oslist)
         g_object_unref(oslist);
+    g_list_free(oses);
 
     g_object_unref(loader);
 }

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Fedora Users]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]

  Powered by Linux