Messages in this thread | | | Date | Sat, 6 Aug 2022 16:37:12 +0800 | From | Leo Yan <> | Subject | Re: [PATCH v5 01/14] perf test: Refactor shell tests allowing subdirs |
| |
On Thu, Jul 28, 2022 at 03:52:43PM +0100, carsten.haitzler@foss.arm.com wrote:
[...]
> +int list_script_max_width(void) > +{ > + list_script_files(); /* Ensure we have scanned all scriptd */
s/scriptd/scripts/
> + return files_max_width; > +}
[...]
> struct shell_test { > const char *dir; > const char *file; > @@ -385,33 +302,17 @@ static int shell_test__run(struct test_suite *test, int subdir __maybe_unused) > static int run_shell_tests(int argc, const char *argv[], int i, int width, > struct intlist *skiplist) > { > - struct dirent **entlist; > - struct dirent *ent; > - int n_dirs, e; > - char path_dir[PATH_MAX]; > - struct shell_test st = { > - .dir = shell_tests__dir(path_dir, sizeof(path_dir)), > - }; > - > - if (st.dir == NULL) > - return -1; > + struct shell_test st; > + const struct script_file *files, *file; > > - n_dirs = scandir(st.dir, &entlist, NULL, alphasort); > - if (n_dirs == -1) { > - pr_err("failed to open shell test directory: %s\n", > - st.dir); > - return -1; > - } > - > - for_each_shell_test(entlist, n_dirs, st.dir, ent) { > + files = list_script_files(); > + if (!files) > + return 0; > + for (file = files; file->dir; file++) { > int curr = i++; > - char desc[256]; > struct test_case test_cases[] = { > { > - .desc = shell_test__description(desc, > - sizeof(desc), > - st.dir, > - ent->d_name), > + .desc = file->desc, > .run_case = shell_test__run, > }, > { .name = NULL, } > @@ -421,12 +322,13 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width, > .test_cases = test_cases, > .priv = &st, > }; > + st.dir = file->dir; > > if (test_suite.desc == NULL || > !perf_test__matches(test_suite.desc, curr, argc, argv)) > continue; > > - st.file = ent->d_name; > + st.file = file->file;
I am just wandering if we can remove "st" in this function, finally I found you are right, the "st" (struct shell_test) will be used in the function shell_test__run(), so let's keep as it is.
> pr_info("%3d: %-*s:", i, width, test_suite.desc); > > if (intlist__find(skiplist, i)) { > @@ -436,10 +338,6 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width, > > test_and_print(&test_suite, 0); > } > - > - for (e = 0; e < n_dirs; e++) > - zfree(&entlist[e]); > - free(entlist); > return 0; > } > > @@ -448,7 +346,7 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist) > struct test_suite *t; > unsigned int j, k; > int i = 0; > - int width = shell_tests__max_desc_width(); > + int width = list_script_max_width(); > > for_each_test(j, k, t) { > int len = strlen(test_description(t, -1)); > @@ -529,36 +427,22 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist) > > static int perf_test__list_shell(int argc, const char **argv, int i) > { > - struct dirent **entlist; > - struct dirent *ent; > - int n_dirs, e; > - char path_dir[PATH_MAX]; > - const char *path = shell_tests__dir(path_dir, sizeof(path_dir)); > - > - if (path == NULL) > - return -1; > + const struct script_file *files, *file; > > - n_dirs = scandir(path, &entlist, NULL, alphasort); > - if (n_dirs == -1) > - return -1; > - > - for_each_shell_test(entlist, n_dirs, path, ent) { > + files = list_script_files(); > + if (!files) > + return 0; > + for (file = files; file->dir; file++) { > int curr = i++; > - char bf[256]; > struct test_suite t = { > - .desc = shell_test__description(bf, sizeof(bf), path, ent->d_name), > + .desc = file->desc > }; > > if (!perf_test__matches(t.desc, curr, argc, argv)) > continue; > > pr_info("%3d: %s\n", i, t.desc); > - > } > - > - for (e = 0; e < n_dirs; e++) > - zfree(&entlist[e]); > - free(entlist); > return 0; > }
Except a minor typo, the patch looks good to me, it's a good refactoring and enhancement for shell script testing.
I reviewed the change one by one line, at least I cannot find any logic error.
With typo fixing:
Reviewed-by: Leo Yan <leo.yan@linaro.org>
I'd leave this patch for maintainers to review it. Just a caveat, given it's a big patch, as Carsten replied it's good that take the patch as a total new code for searching shell scripts, this would be easier for understanding the change.
Thanks, Leo
| |