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