| Message ID | 5ecbc976e6216b941e760e096e166ab432ee7784.1705443632.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-tool: add unit test suite runner | expand |
Josh Steadmon <steadmon@google.com> writes: > Teach the testsuite runner in `test-tool run-command testsuite` how to > run unit tests, by adding two new flags: > > First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test > binaries directly, rather than trying to interpret them as shell > scripts. Makes perfect sense. > Second "--(no-)require-shell-test-pattern" bypasses the check that the > test filenames match the expected t####-*.sh pattern. This one I am not so sure. Do we still have situations where erroring out when fed a non t[0-9][0-9][0-9][0-9]-*.sh script is problematic? IOW, do we need to keep it as conditional? ... goes and looks ... Ah, this variable/option is misnamed and that is what invited the above nonsense question out of me. The logic this option disables does not "require" (and error out if the requirement is not met); it is used in a loop over "ls *" and "filtering" files out that are not the numbered scripts. But that confusion makes me wonder if non-script side of tests would also want some filtering in the longer run, even if the directory we feed to "test-tool run" happens to contain nothing but what we want to run right now. I wonder if we instead want a variable that holds a pattern used to match programs readdir() discovers and skip those that do not match the pattern? Its default value may be something like "t[0-9][0-9][0-9][0-9]-*.sh" but alternatively you can give, say, "*" to pass everything, or something like that. > With these changes, you can now use test-tool to run the unit tests: > $ make > $ cd t/unit-tests/bin > $ ../../helper/test-tool run-command testsuite --no-run-in-shell \ > --no-require-shell-test-pattern This makes me wonder why we want to do the readdir() loop ourselves. Instead of saying --no-require-shell-test-pattern there, wouldn't it be simpler to say "*" right there, and have testsuite() run the test programs named from the command line? But that is orthogonal to the enhancement we have here.
Junio C Hamano <gitster@pobox.com> writes: > Josh Steadmon <steadmon@google.com> writes: > >> Teach the testsuite runner in `test-tool run-command testsuite` how to >> run unit tests, by adding two new flags: >> >> First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test >> binaries directly, rather than trying to interpret them as shell >> scripts. > > Makes perfect sense. This may be a stupid question, but do we even need the current "push 'sh' to the strvec"? If our executable shell scripts run just fine without, then this may not have to be conditional.
On Tue, Jan 16, 2024 at 03:40:01PM -0800, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Josh Steadmon <steadmon@google.com> writes: > > > >> Teach the testsuite runner in `test-tool run-command testsuite` how to > >> run unit tests, by adding two new flags: > >> > >> First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test > >> binaries directly, rather than trying to interpret them as shell > >> scripts. > > > > Makes perfect sense. > > This may be a stupid question, but do we even need the current "push > 'sh' to the strvec"? If our executable shell scripts run just fine > without, then this may not have to be conditional. It is necessary for the same reason that the indirection provided by patch 4 is necessary: the test scripts are supposed to be run by TEST_SHELL_PATH, even if they may say "#!/bin/sh" in their header. But the "testsuite" helper just hard-codes "sh", which is wrong. It's somewhat academic for Windows where "sh" is already bash, and that's the only reasonable thing anybody would set TEST_SHELL_PATH to anyway. But if the point is to be a drop-in replacement for the existing flow, it should be checking getenv("TEST_SHELL_PATH"). And perhaps the Makefile might need to correctly export the variable. -Peff
On 2024.01.16 15:18, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > With these changes, you can now use test-tool to run the unit tests: > > $ make > > $ cd t/unit-tests/bin > > $ ../../helper/test-tool run-command testsuite --no-run-in-shell \ > > --no-require-shell-test-pattern > > This makes me wonder why we want to do the readdir() loop ourselves. > Instead of saying --no-require-shell-test-pattern there, wouldn't it > be simpler to say "*" right there, and have testsuite() run the test > programs named from the command line? It's speculation on my part, but I wonder if it has something to do with the number of shell tests? Google tells me that on Windows, the maximum command line length is 8191 characters. Which is actually a fair bit smaller than expanding the shell test list: $ echo t????-*.sh | wc -c 25714
Josh Steadmon <steadmon@google.com> writes: > On 2024.01.16 15:18, Junio C Hamano wrote: >> Josh Steadmon <steadmon@google.com> writes: >> > With these changes, you can now use test-tool to run the unit tests: >> > $ make >> > $ cd t/unit-tests/bin >> > $ ../../helper/test-tool run-command testsuite --no-run-in-shell \ >> > --no-require-shell-test-pattern >> >> This makes me wonder why we want to do the readdir() loop ourselves. >> Instead of saying --no-require-shell-test-pattern there, wouldn't it >> be simpler to say "*" right there, and have testsuite() run the test >> programs named from the command line? > > It's speculation on my part, but I wonder if it has something to do with > the number of shell tests? Google tells me that on Windows, the maximum > command line length is 8191 characters. Which is actually a fair bit > smaller than expanding the shell test list: > > $ echo t????-*.sh | wc -c > 25714 OK.
On 2024.01.16 15:18, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > Second "--(no-)require-shell-test-pattern" bypasses the check that the > > test filenames match the expected t####-*.sh pattern. > > This one I am not so sure. Do we still have situations where > erroring out when fed a non t[0-9][0-9][0-9][0-9]-*.sh script is > problematic? IOW, do we need to keep it as conditional? > > ... goes and looks ... > > Ah, this variable/option is misnamed and that is what invited the > above nonsense question out of me. The logic this option disables > does not "require" (and error out if the requirement is not met); it > is used in a loop over "ls *" and "filtering" files out that are not > the numbered scripts. > > But that confusion makes me wonder if non-script side of tests would > also want some filtering in the longer run, even if the directory we > feed to "test-tool run" happens to contain nothing but what we want > to run right now. I wonder if we instead want a variable that holds > a pattern used to match programs readdir() discovers and skip those > that do not match the pattern? Its default value may be something > like "t[0-9][0-9][0-9][0-9]-*.sh" but alternatively you can give, > say, "*" to pass everything, or something like that. The original implementation and this series both still allow passing additional patterns on the command line. Only tests matching the hardcoded t[0-9][0-9][0-9][0-9]-*.sh pattern plus all the command-line patterns will be run. The new flag just bypasses the hardcoded pattern. Given that V2 will already be changing in non-backwards-compatible ways, I think it makes the most sense to just delete the hardcoded pattern and require callers to provide it if they want it.
On 2024.02.01 15:08, Josh Steadmon wrote: > On 2024.01.16 15:18, Junio C Hamano wrote: > > Josh Steadmon <steadmon@google.com> writes: > > > Second "--(no-)require-shell-test-pattern" bypasses the check that the > > > test filenames match the expected t####-*.sh pattern. > > > > This one I am not so sure. Do we still have situations where > > erroring out when fed a non t[0-9][0-9][0-9][0-9]-*.sh script is > > problematic? IOW, do we need to keep it as conditional? > > > > ... goes and looks ... > > > > Ah, this variable/option is misnamed and that is what invited the > > above nonsense question out of me. The logic this option disables > > does not "require" (and error out if the requirement is not met); it > > is used in a loop over "ls *" and "filtering" files out that are not > > the numbered scripts. > > > > But that confusion makes me wonder if non-script side of tests would > > also want some filtering in the longer run, even if the directory we > > feed to "test-tool run" happens to contain nothing but what we want > > to run right now. I wonder if we instead want a variable that holds > > a pattern used to match programs readdir() discovers and skip those > > that do not match the pattern? Its default value may be something > > like "t[0-9][0-9][0-9][0-9]-*.sh" but alternatively you can give, > > say, "*" to pass everything, or something like that. > > The original implementation and this series both still allow passing > additional patterns on the command line. Only tests matching the > hardcoded t[0-9][0-9][0-9][0-9]-*.sh pattern plus all the command-line > patterns will be run. The new flag just bypasses the hardcoded pattern. Actually, I misspoke here. The test must match the hardcoded pattern plus at least one of the command-line patterns (if any were provided). > Given that V2 will already be changing in non-backwards-compatible ways, > I think it makes the most sense to just delete the hardcoded pattern and > require callers to provide it if they want it. Given that it sounds like the testsuite functionality is not really used at the moment, I still feel OK with changing the pattern matching logic here for V2.
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index c0ed8722c8..2db7e1ef03 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -64,11 +64,12 @@ static int task_finished(int result UNUSED, struct testsuite { struct string_list tests, failed; int next; - int quiet, immediate, verbose, verbose_log, trace, write_junit_xml; + int quiet, immediate, verbose, verbose_log, trace, write_junit_xml, run_in_shell; }; #define TESTSUITE_INIT { \ .tests = STRING_LIST_INIT_DUP, \ .failed = STRING_LIST_INIT_DUP, \ + .run_in_shell = 1, \ } static int next_test(struct child_process *cp, struct strbuf *err, void *cb, @@ -80,7 +81,9 @@ static int next_test(struct child_process *cp, struct strbuf *err, void *cb, return 0; test = suite->tests.items[suite->next++].string; - strvec_pushl(&cp->args, "sh", test, NULL); + if (suite->run_in_shell) + strvec_push(&cp->args, "sh"); + strvec_push(&cp->args, test); if (suite->quiet) strvec_push(&cp->args, "--quiet"); if (suite->immediate) @@ -133,7 +136,7 @@ static const char * const testsuite_usage[] = { static int testsuite(int argc, const char **argv) { struct testsuite suite = TESTSUITE_INIT; - int max_jobs = 1, i, ret = 0; + int max_jobs = 1, i, ret = 0, require_shell_test_pattern = 1; DIR *dir; struct dirent *d; struct option options[] = { @@ -147,6 +150,10 @@ static int testsuite(int argc, const char **argv) OPT_BOOL('x', "trace", &suite.trace, "trace shell commands"), OPT_BOOL(0, "write-junit-xml", &suite.write_junit_xml, "write JUnit-style XML files"), + OPT_BOOL(0, "run-in-shell", &suite.run_in_shell, + "run programs in the suite via `sh`"), + OPT_BOOL(0, "require-shell-test-pattern", &require_shell_test_pattern, + "require programs to match 't####-*.sh'"), OPT_END() }; struct run_process_parallel_opts opts = { @@ -155,12 +162,21 @@ static int testsuite(int argc, const char **argv) .task_finished = test_finished, .data = &suite, }; + struct strbuf progpath = STRBUF_INIT; + size_t path_prefix_len; argc = parse_options(argc, argv, NULL, options, testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION); if (max_jobs <= 0) max_jobs = online_cpus(); + /* + * If we run without a shell, we have to provide the relative path to + * the executables. + */ + if (!suite.run_in_shell) + strbuf_addstr(&progpath, "./"); + path_prefix_len = progpath.len; dir = opendir("."); if (!dir) @@ -168,20 +184,27 @@ static int testsuite(int argc, const char **argv) while ((d = readdir(dir))) { const char *p = d->d_name; - if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) || - !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' || - !ends_with(p, ".sh")) + if (!strcmp(p, ".") || !strcmp(p, "..")) continue; + if (require_shell_test_pattern) + if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) || + !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' || + !ends_with(p, ".sh")) + continue; /* No pattern: match all */ if (!argc) { - string_list_append(&suite.tests, p); + strbuf_setlen(&progpath, path_prefix_len); + strbuf_addstr(&progpath, p); + string_list_append(&suite.tests, progpath.buf); continue; } for (i = 0; i < argc; i++) if (!wildmatch(argv[i], p, 0)) { - string_list_append(&suite.tests, p); + strbuf_setlen(&progpath, path_prefix_len); + strbuf_addstr(&progpath, p); + string_list_append(&suite.tests, progpath.buf); break; } } @@ -208,6 +231,7 @@ static int testsuite(int argc, const char **argv) string_list_clear(&suite.tests, 0); string_list_clear(&suite.failed, 0); + strbuf_release(&progpath); return ret; }
Teach the testsuite runner in `test-tool run-command testsuite` how to run unit tests, by adding two new flags: First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test binaries directly, rather than trying to interpret them as shell scripts. Second "--(no-)require-shell-test-pattern" bypasses the check that the test filenames match the expected t####-*.sh pattern. With these changes, you can now use test-tool to run the unit tests: $ make $ cd t/unit-tests/bin $ ../../helper/test-tool run-command testsuite --no-run-in-shell \ --no-require-shell-test-pattern This should be helpful on Windows to allow running tests without requiring Perl (for `prove`), as discussed in [1] and [2]. [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/ [2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/ Signed-off-by: Josh Steadmon <steadmon@google.com> --- t/helper/test-run-command.c | 40 +++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-)