> We have the under utilized lib/util.[ch] files already. I think either
> these functions should go there, or the functions there should get merged
> into whatever we want to call the files for these functions, and util.[ch]
> should then be removed. Actually, I think Radim wanted to rework the one
> and only function in util.[ch] too, to improve its interface...
>
In general, I think I'd prefer to drop util.X and rather give the child
a name. :)
>
>> @@ -0,0 +1,89 @@
>> +/*
>> + * Utility functions for running/configuring a set of tests
>> + *
>> + * Copyright (c) 2017 Red Hat Inc
>> + *
>> + * Authors:
>> + * Thomas Huth <thuth@xxxxxxxxxx>
>> + * David Hildenbrand <david@xxxxxxxxxx>
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Library General Public License version 2.
>> + */
>> +
>> +#include <testrun.h>
>> +#include <asm/time.h>
>> +
>> +void parse_test_args(const int argc, const char **argv, struct test *test)
>> +{
>> + bool run_all = true;
>> + int i, ti;
>> +
>> + test->iterations = 0;
>> + test->time_to_run = 0;
>> +
>> + for (i = 1; i < argc; i++) {
>> + if (!strcmp("-i", argv[i])) {
>> + i++;
>> + if (i >= argc)
>> + report_abort("-i needs a parameter");
>> + test->iterations = atol(argv[i]);
>> + } else if (!strcmp("-t", argv[i])) {
>> + i++;
>> + if (i >= argc)
>> + report_abort("-t needs a parameter");
>> + test->time_to_run = atol(argv[i]);
>
> I'd prefer we use long form args, like --iterations=<num> and
> --time-to-run=<seconds>. That way unittests.cfg files will be self
> documenting.
I just moved this code from intercept.c. So this sure can be done.
>
>> + } else for (ti = 0; test->test_cases[ti].name != NULL; ti++) {
>
> Unusual (at least to me) coding style. I'd prefer the more usual (to me)
>
Absolutely, I also just moved it. But this really looks ... special :)
> else {
> for ... {
> }
> }
>
>> + if (!strcmp(test->test_cases[ti].name, argv[i])) {
>> + run_all = false;
>> + test->test_cases[ti].run_it = true;
>> + break;
>
> The break means we can only list a single test to run, right? What about
> being able to run more than one by listing all you want?
That should be supported, no? The break will simply start looking for a
test named "argv[i]". The next one (argv[i + 1]) would be processed
afterwards.
>
>> + } else if (test->test_cases[ti + 1].name == NULL) {
>> + report_abort("Unsupported parameter '%s'",
>> + argv[i]);
>
> Instead of this, we could use a counter to track number of recognized
> args and then, after running the loop, if the counter doesn't match argc
> complain/abort.
Yes, this could be refactored a lot, especially if we want to include
parsing test specific parameters (specified in struct test.).
>
>> + }
>> + }
>> + }
>> +
>> + if (test->iterations == 0 && test->time_to_run == 0)
>> + test->iterations = 1;
>> +
>> + if ((test->iterations > 1 || test->time_to_run) &&
>> + !test->multirun_supported)
>> + report_abort("Test does not support multiruns.");
>> +
>> + if (run_all) {
>> + for (ti = 0; test->test_cases[ti].name != NULL; ti++)
>> + test->test_cases[ti].run_it = true;
>> + }
>> +}
>> +
>> +int run_test(struct test *test)
>
> I prefer this be plural, run_tests, and the argument test doesn't
> seem named quite right either, "testset" maybe?
I renamed this at least 10 times :)
We have a test that contains test cases. So it should be singular. and
struct test handed over is not a list but only one element. But we would
have to agree on a common terminology.
Main question is, if it makes sense to work on this or directly try to
implement at least parts of the glib test API as Paolo suggests. Any
thoughts?
Thanks!
--
Thanks,
David
