On Tue, Apr 10, 2018 at 12:29 PM, Christophe Fergeau
<cfergeau@xxxxxxxxxx> 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@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.
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
_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo
