On Wed, Sep 13, 2017 at 03:07:23PM +0200, David Hildenbrand wrote:
>
> > 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.
I somehow forgot there was an outer for loop... So, yeah, this is fine.
>
> >
> >> + } 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.
I can retrain my brain to think this way, and then the terminology makes
sense. I just wonder if I'm the only one who needs retraining, or if
the API naming will always be a source of confusion? Anyway, the API
naming can be left to GTest.
>
> 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?
Yes, I prefer the GTest idea as well. I just wanted to review this patch
too, in case any of the code would still be used when converting it.
Thanks,
drew
