[igt-dev] [PATCH] runner: add --list-all and --blacklist
Petri Latvala
petri.latvala at intel.com
Fri Jun 14 14:28:38 UTC 2019
On Fri, Jun 14, 2019 at 04:24:29PM +0300, Oleg Vasilev wrote:
> Currently, runner already collects all subtest names into job_list.
> --list-all allows to output it to stdout.
>
> --blacklist option takes a filename as an argument and adds all regexes
> from that file to the exclusion list.
>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Signed-off-by: Oleg Vasilev <oleg.vasilev at intel.com>
> ---
> runner/job_list.c | 18 +++++++
> runner/job_list.h | 1 +
> runner/runner.c | 5 ++
> runner/runner_tests.c | 14 ++++-
> runner/settings.c | 118 ++++++++++++++++++++++++++++++++++--------
> runner/settings.h | 1 +
> 6 files changed, 135 insertions(+), 22 deletions(-)
>
> diff --git a/runner/job_list.c b/runner/job_list.c
> index 4a16742f..2a50dc5c 100644
> --- a/runner/job_list.c
> +++ b/runner/job_list.c
> @@ -291,6 +291,24 @@ static bool job_list_from_test_list(struct job_list *job_list,
> return any;
> }
>
> +void list_all_tests(struct job_list* lst)
> +{
> + for (size_t test_idx = 0; test_idx < lst->size; ++test_idx){
> + struct job_list_entry* current_entry = lst->entries+test_idx;
> + if (current_entry->subtest_count==0) {
> + printf("igt@%s\n", current_entry->binary);
> + continue;
> + }
> + for (size_t subtest_idx = 0;
> + subtest_idx < current_entry->subtest_count;
> + ++subtest_idx) {
> + char *subtest_name = current_entry->subtests[subtest_idx];
> + printf("igt@%s@%s\n", current_entry->binary, subtest_name);
> + }
> + }
> +}
> +
> +
> static char *lowercase(const char *str)
> {
> char *ret = malloc(strlen(str) + 1);
> diff --git a/runner/job_list.h b/runner/job_list.h
> index f8bbbddc..cee4bff6 100644
> --- a/runner/job_list.h
> +++ b/runner/job_list.h
> @@ -36,5 +36,6 @@ bool create_job_list(struct job_list *job_list, struct settings *settings);
>
> bool serialize_job_list(struct job_list *job_list, struct settings *settings);
> bool read_job_list(struct job_list *job_list, int dirfd);
> +void list_all_tests(struct job_list* lst);
>
> #endif
> diff --git a/runner/runner.c b/runner/runner.c
> index e1a6ccba..909ec861 100644
> --- a/runner/runner.c
> +++ b/runner/runner.c
> @@ -24,6 +24,11 @@ int main(int argc, char **argv)
> return 1;
> }
>
> + if(settings.list_all) {
Add space after if
> + list_all_tests(&job_list);
> + return 0;
> + }
> +
> if (!initialize_execute_state(&state, &settings, &job_list)) {
> return 1;
> }
> diff --git a/runner/runner_tests.c b/runner/runner_tests.c
> index c09cda70..00bb9de3 100644
> --- a/runner/runner_tests.c
> +++ b/runner/runner_tests.c
> @@ -388,6 +388,8 @@ igt_main
> "-t", "pattern2",
> "-x", "xpattern1",
> "-x", "xpattern2",
> + "-b", "../runner/testdata/test-blacklist.txt",
> + "--blacklist", "../runner/testdata/test-blacklist2.txt",
Don't use relative paths, you don't know what the CWD is.
Use the variable testdatadir to find runner/testdata. And remember to git add those files.
> "-s",
> "-l", "verbose",
> "--overwrite",
> @@ -410,9 +412,11 @@ igt_main
> igt_assert_eq(settings->include_regexes.size, 2);
> igt_assert_eqstr(settings->include_regexes.regex_strings[0], "pattern1");
> igt_assert_eqstr(settings->include_regexes.regex_strings[1], "pattern2");
> - igt_assert_eq(settings->exclude_regexes.size, 2);
> + igt_assert_eq(settings->exclude_regexes.size, 4);
> igt_assert_eqstr(settings->exclude_regexes.regex_strings[0], "xpattern1");
> igt_assert_eqstr(settings->exclude_regexes.regex_strings[1], "xpattern2");
> + igt_assert_eqstr(settings->exclude_regexes.regex_strings[2], "xpattern3"); /* From blacklist */
> + igt_assert_eqstr(settings->exclude_regexes.regex_strings[3], "xpattern4"); /* From blacklist2 */
> igt_assert(settings->sync);
> igt_assert_eq(settings->log_level, LOG_LEVEL_VERBOSE);
> igt_assert(settings->overwrite);
> @@ -426,6 +430,14 @@ igt_main
> igt_assert(settings->piglit_style_dmesg);
> igt_assert_eq(settings->dmesg_warn_level, 3);
> }
> + igt_subtest("parse-list-all") {
> + const char *argv[] = { "runner",
> + "--list-all",
> + "test-root-dir"};
> +
> + igt_assert(parse_options(ARRAY_SIZE(argv), (char**)argv, settings));
> + igt_assert_eq(settings->list_all, 1);
> + }
>
> igt_subtest("dmesg-warn-level-inferred") {
> const char *argv[] = { "runner",
> diff --git a/runner/settings.c b/runner/settings.c
> index ad38ae8d..2692370f 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -1,5 +1,6 @@
> #include "settings.h"
>
> +#include <ctype.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <getopt.h>
> @@ -30,6 +31,8 @@ enum {
> OPT_MULTIPLE = 'm',
> OPT_TIMEOUT = 'c',
> OPT_WATCHDOG = 'g',
> + OPT_BLACKLIST = 'b',
> + OPT_LIST_ALL = 'L',
> };
>
> static struct {
> @@ -117,7 +120,8 @@ static bool parse_abort_conditions(struct settings *settings, const char *optarg
> }
>
> static const char *usage_str =
> - "usage: runner [options] [test_root] results-path\n\n"
> + "usage: runner [options] [test_root] results-path\n"
> + " or: runner --list-all [options] [test_root] \n\n"
> "Options:\n"
> " Piglit compatible:\n"
> " -h, --help Show this help message and exit\n"
> @@ -172,6 +176,10 @@ static const char *usage_str =
> " (longer) filter list means the test result should\n"
> " change. KERN_NOTICE dmesg level is treated as warn,\n"
> " unless overridden with --dmesg-warn-level.\n"
> + " -b, --blacklist FILENAME\n"
> + " Exclude all test matching to regexes from FILENAME\n"
> + " (can be used more than once)\n"
> + " -L, --list-all List all matching subtests instead of running\n"
> " [test_root] Directory that contains the IGT tests. The environment\n"
> " variable IGT_TEST_ROOT will be used if set, overriding\n"
> " this option if given.\n"
> @@ -213,6 +221,51 @@ static bool add_regex(struct regex_list *list, char *new)
> return true;
> }
>
> +static bool parse_blacklist(struct regex_list *exclude_regexes, char *blacklist_filename)
> +{
> + FILE *f;
> + char *line = NULL;
> + size_t line_len = 0;
> + bool status;
> +
> +
You have an extra newline here.
> + if ((f = fopen(blacklist_filename, "r")) == NULL) {
> + fprintf(stderr, "Cannot open blacklist file %s\n", blacklist_filename);
> + return false;
> + }
> + while (1) {
> + size_t str_size = 0, idx = 0;
> +
> + if (getline(&line, &line_len, f) == -1) {
> + if (errno == EINTR)
> + continue;
> + else
> + break;
> + }
> +
> + while (true) {
> + if(line[idx]=='\n' ||
> + line[idx]=='#' || /* # starts a comment */
> + line[idx]=='\0')
> + break;
> + idx++;
> + if (!isspace(line[str_size]))
> + str_size = idx;
> + }
> + if (str_size > 0) {
> + char * test_regex = strndup(line, str_size);
> + status = add_regex(exclude_regexes, test_regex);
> + if(!status){
> + break;
> + }
> + }
There's something in this parsing code that feels funky but as long as
it works (more on that later)...
> + }
> +
> + free(line);
> + fclose(f);
> + return status;
> +}
> +
> static void free_regexes(struct regex_list *regexes)
> {
> size_t i;
> @@ -272,6 +325,8 @@ bool parse_options(int argc, char **argv,
> {"use-watchdog", no_argument, NULL, OPT_WATCHDOG},
> {"piglit-style-dmesg", no_argument, NULL, OPT_PIGLIT_DMESG},
> {"dmesg-warn-level", required_argument, NULL, OPT_DMESG_WARN_LEVEL},
> + {"blacklist", required_argument, NULL, OPT_BLACKLIST},
> + {"list-all", no_argument, NULL, OPT_LIST_ALL},
> { 0, 0, 0, 0},
> };
>
> @@ -281,7 +336,7 @@ bool parse_options(int argc, char **argv,
>
> settings->dmesg_warn_level = -1;
>
> - while ((c = getopt_long(argc, argv, "hn:dt:x:sl:om", long_options, NULL)) != -1) {
> + while ((c = getopt_long(argc, argv, "hn:dt:x:sl:omb:L", long_options, NULL)) != -1) {
> switch (c) {
> case OPT_HELP:
> usage(NULL, stdout);
> @@ -342,6 +397,13 @@ bool parse_options(int argc, char **argv,
> case OPT_DMESG_WARN_LEVEL:
> settings->dmesg_warn_level = atoi(optarg);
> break;
> + case OPT_BLACKLIST:
> + if (!parse_blacklist(&settings->exclude_regexes, absolute_path(optarg)))
> + goto error;
> + break;
> + case OPT_LIST_ALL:
> + settings->list_all = true;
> + break;
> case '?':
> usage(NULL, stderr);
> goto error;
> @@ -354,20 +416,39 @@ bool parse_options(int argc, char **argv,
> if (settings->dmesg_warn_level < 0)
> settings->dmesg_warn_level = 4; /* KERN_WARN */
>
> - switch (argc - optind) {
> - case 2:
> - settings->test_root = absolute_path(argv[optind]);
> - ++optind;
> - /* fallthrough */
> - case 1:
> - settings->results_path = absolute_path(argv[optind]);
> - break;
> - case 0:
> - usage("Results-path missing", stderr);
> - goto error;
> - default:
> - usage("Extra arguments after results-path", stderr);
> - goto error;
> + if (settings->list_all) { /* --list-all doesn't requrie results path */
Typo, require
> + switch (argc - optind) {
> + case 1:
> + settings->test_root = absolute_path(argv[optind]);
> + ++optind;
> + /* fallthrough */
> + case 0:
> + break;
> + default:
> + usage("Too many arguments for --list-all", stderr);
> + goto error;
> + }
> + } else {
> + switch (argc - optind) {
> + case 2:
> + settings->test_root = absolute_path(argv[optind]);
> + ++optind;
> + /* fallthrough */
> + case 1:
> + settings->results_path = absolute_path(argv[optind]);
> + break;
> + case 0:
> + usage("Results-path missing", stderr);
> + goto error;
> + default:
> + usage("Extra arguments after results-path", stderr);
> + goto error;
> + }
> + if (!settings->name) {
> + char *name = strdup(settings->results_path);
> + settings->name = strdup(basename(name));
> + free(name);
> + }
TODO for later: Refactor this duplicated switch.
Looks good otherwise, but nack for merging as of yet, because:
While performance is better:
# time ./scripts/run-tests.sh -l
.......
real 0m5.188s
user 0m3.890s
sys 0m1.354s
# time build/runner/igt_runner -L build/tests
.......
real 0m3.065s
user 0m2.222s
sys 0m0.895s
(I won't compare listing with blacklist processing, that's a horrible
shell kludge)
the results are different:
original-kludge.txt - blacklist-processed testlist with horrible shell
kludge
runner.txt - igt_runner -L -b tests/intel-ci/blacklist.txt
build/tests, sorted and downcased to compare apples to apples
diff -u original-kludge.txt runner.txt
+igt at gem_exec_lut_handle
+igt at gem_fd_exhaustion
+igt at gem_gtt_hog
+igt at gem_gtt_speed
+igt at gem_lut_handle
+igt at gem_ring_sync_loop
Those are blacklisted with e.g.
igt at gem_exec_lut_handle(@.*)?
Please check whether the fault is glib regex being unable to process
that or our usage of it.
--
Petri Latvala
More information about the igt-dev
mailing list