| Message ID | b5665386b56df91fa5d95ee5b11288b5853546f0.1706921262.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-tool: add unit test suite runner | expand |
Hi Josh On 03/02/2024 00:50, Josh Steadmon wrote: > Teach the testsuite runner in `test-tool run-command testsuite` how to > run unit tests: if TEST_SHELL_PATH is not set, assume that we're running > the programs directly from CWD, rather than defaulting to "sh" as an > interpreter. Judging from the last patch in this series it seems likely that we'll want to run unit tests and integration tests parallel. In which case it might be better to look at the filename extension to decide whether to sh as an interpreter so that we can avoid having to use a wrapper script. Then cd t helper/test-tool run-command testsuite 't[0-9]*.sh' 'unit-tests/bin/*' would run the integration tests via "sh" and the unit-tests directly. We'd need to figure out how to look for tests in both directories as well though... Best Wishes Phillip > With this change, you can now use test-tool to run the unit tests: > $ make > $ cd t/unit-tests/bin > $ ../../helper/test-tool run-command testsuite > > This should be helpful on Windows to allow running tests without > requiring Perl (for `prove`), as discussed in [1] and [2]. > > This again breaks backwards compatibility, as it is now required to set > TEST_SHELL_PATH properly for executing shell scripts, but again, as > noted in [2], there are no longer any such invocations in our codebase. > > [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 | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c > index e6bd792274..a0b8dc6fd7 100644 > --- a/t/helper/test-run-command.c > +++ b/t/helper/test-run-command.c > @@ -158,6 +158,8 @@ 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); > @@ -165,9 +167,14 @@ static int testsuite(int argc, const char **argv) > if (max_jobs <= 0) > max_jobs = online_cpus(); > > + /* > + * If we run without a shell, we have to provide the relative path to > + * the executables. > + */ > suite.shell_path = getenv("TEST_SHELL_PATH"); > if (!suite.shell_path) > - suite.shell_path = "sh"; > + strbuf_addstr(&progpath, "./"); > + path_prefix_len = progpath.len; > > dir = opendir("."); > if (!dir) > @@ -180,13 +187,17 @@ static int testsuite(int argc, const char **argv) > > /* 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; > } > } > @@ -213,6 +224,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; > }
Josh Steadmon <steadmon@google.com> writes: > Teach the testsuite runner in `test-tool run-command testsuite` how to > run unit tests: if TEST_SHELL_PATH is not set, assume that we're running > the programs directly from CWD, rather than defaulting to "sh" as an > interpreter. Hmph. It sounds more like "the run-command testsuite subcommand only runs programs in the current directory", not "assume" (which implies there is a way to override the assumption). Not that the limitation would hurt us in any way, though. > + /* > + * If we run without a shell, we have to provide the relative path to > + * the executables. > + */ It sounds more like "If TEST_SHELL_PATH is not given, then we run them in the current directory.". It is perfectly fine, because ... > suite.shell_path = getenv("TEST_SHELL_PATH"); > if (!suite.shell_path) > - suite.shell_path = "sh"; > + strbuf_addstr(&progpath, "./"); > + path_prefix_len = progpath.len; > > dir = opendir("."); > if (!dir) > @@ -180,13 +187,17 @@ static int testsuite(int argc, const char **argv) > ... > 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; > } > } ... this "prefixing" is done to a path discovered by readdir() from a directory handle obtained by opendir("."). If there were a way to pass paths to executables directly, possibly as absolute paths, the unconditional prefixing of "./" would have been a problem, but we do not have such a facility, so this should be OK.
On 2024.02.05 16:16, phillip.wood123@gmail.com wrote: > Hi Josh > > On 03/02/2024 00:50, Josh Steadmon wrote: > > Teach the testsuite runner in `test-tool run-command testsuite` how to > > run unit tests: if TEST_SHELL_PATH is not set, assume that we're running > > the programs directly from CWD, rather than defaulting to "sh" as an > > interpreter. > > Judging from the last patch in this series it seems likely that we'll want > to run unit tests and integration tests parallel. In which case it might be > better to look at the filename extension to decide whether to sh as an > interpreter so that we can avoid having to use a wrapper script. Then > > cd t > helper/test-tool run-command testsuite 't[0-9]*.sh' 'unit-tests/bin/*' > > would run the integration tests via "sh" and the unit-tests directly. We'd > need to figure out how to look for tests in both directories as well > though... At the moment, I'm not planning on trying to make unit tests and shell tests run under the same test-tool process. If that is a valuable change, hopefully the Windows / CI experts can use this series as a starting point to make additional test-tool changes. However, I will probably not spend any further time on this area.
On 2024.02.07 12:48, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > > Teach the testsuite runner in `test-tool run-command testsuite` how to > > run unit tests: if TEST_SHELL_PATH is not set, assume that we're running > > the programs directly from CWD, rather than defaulting to "sh" as an > > interpreter. > > Hmph. It sounds more like "the run-command testsuite subcommand > only runs programs in the current directory", not "assume" (which > implies there is a way to override the assumption). Not that the > limitation would hurt us in any way, though. > > > + /* > > + * If we run without a shell, we have to provide the relative path to > > + * the executables. > > + */ > > It sounds more like "If TEST_SHELL_PATH is not given, then we run > them in the current directory.". Reworded both of these in V3.
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index e6bd792274..a0b8dc6fd7 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -158,6 +158,8 @@ 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); @@ -165,9 +167,14 @@ static int testsuite(int argc, const char **argv) if (max_jobs <= 0) max_jobs = online_cpus(); + /* + * If we run without a shell, we have to provide the relative path to + * the executables. + */ suite.shell_path = getenv("TEST_SHELL_PATH"); if (!suite.shell_path) - suite.shell_path = "sh"; + strbuf_addstr(&progpath, "./"); + path_prefix_len = progpath.len; dir = opendir("."); if (!dir) @@ -180,13 +187,17 @@ static int testsuite(int argc, const char **argv) /* 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; } } @@ -213,6 +224,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: if TEST_SHELL_PATH is not set, assume that we're running the programs directly from CWD, rather than defaulting to "sh" as an interpreter. With this change, you can now use test-tool to run the unit tests: $ make $ cd t/unit-tests/bin $ ../../helper/test-tool run-command testsuite This should be helpful on Windows to allow running tests without requiring Perl (for `prove`), as discussed in [1] and [2]. This again breaks backwards compatibility, as it is now required to set TEST_SHELL_PATH properly for executing shell scripts, but again, as noted in [2], there are no longer any such invocations in our codebase. [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 | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)