| Message ID | c8448406d71151714e89893208c46b8a4c369cb5.1706921262.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: > When running tests through `test-tool run-command testsuite`, we > currently hardcode `sh` as the command interpreter. As discussed in [1], > this is incorrect, and we should be using the shell set in > TEST_SHELL_PATH instead. > > Add a shell_path field in struct testsuite so that we can pass this to > the task runner callback. If this is non-null, we'll use it as the > argv[0] of the subprocess. Otherwise, we'll just execute the test > program directly. That sounds nice in theory, but ... > When setting up the struct testsuite in testsuite(), use the value > of TEST_SHELL_PATH if it's set, otherwise default to `sh`. ... this done in the testsuite() function, doesn't suite.shell_path always gets some non-NULL value? Perhaps in a later step we will add a mechanism to allow suite.shell_path to be NULL when we know we are running an executable, or something? Leaving readers in a bit of suspense may, especially in a series that is short like this, be a good technique to entice them to keep reading, perhaps, but anyway, if that is what is intended, a simple "this feature is not used in this step, but we will take advantage of it soon in a later step" would be a good idea.
On 2024.02.07 12:55, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > > When running tests through `test-tool run-command testsuite`, we > > currently hardcode `sh` as the command interpreter. As discussed in [1], > > this is incorrect, and we should be using the shell set in > > TEST_SHELL_PATH instead. > > > > Add a shell_path field in struct testsuite so that we can pass this to > > the task runner callback. If this is non-null, we'll use it as the > > argv[0] of the subprocess. Otherwise, we'll just execute the test > > program directly. > > That sounds nice in theory, but ... > > > When setting up the struct testsuite in testsuite(), use the value > > of TEST_SHELL_PATH if it's set, otherwise default to `sh`. > > ... this done in the testsuite() function, doesn't suite.shell_path > always gets some non-NULL value? Perhaps in a later step we will > add a mechanism to allow suite.shell_path to be NULL when we know > we are running an executable, or something? > > Leaving readers in a bit of suspense may, especially in a series > that is short like this, be a good technique to entice them to keep > reading, perhaps, but anyway, if that is what is intended, a simple > "this feature is not used in this step, but we will take advantage > of it soon in a later step" would be a good idea. Reworded in V3.
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index c0ed8722c8..a41a54d9cb 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -65,6 +65,7 @@ struct testsuite { struct string_list tests, failed; int next; int quiet, immediate, verbose, verbose_log, trace, write_junit_xml; + const char *shell_path; }; #define TESTSUITE_INIT { \ .tests = STRING_LIST_INIT_DUP, \ @@ -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->shell_path) + strvec_push(&cp->args, suite->shell_path); + strvec_push(&cp->args, test); if (suite->quiet) strvec_push(&cp->args, "--quiet"); if (suite->immediate) @@ -162,6 +165,10 @@ static int testsuite(int argc, const char **argv) if (max_jobs <= 0) max_jobs = online_cpus(); + suite.shell_path = getenv("TEST_SHELL_PATH"); + if (!suite.shell_path) + suite.shell_path = "sh"; + dir = opendir("."); if (!dir) die("Could not open the current directory");
When running tests through `test-tool run-command testsuite`, we currently hardcode `sh` as the command interpreter. As discussed in [1], this is incorrect, and we should be using the shell set in TEST_SHELL_PATH instead. Add a shell_path field in struct testsuite so that we can pass this to the task runner callback. If this is non-null, we'll use it as the argv[0] of the subprocess. Otherwise, we'll just execute the test program directly. When setting up the struct testsuite in testsuite(), use the value of TEST_SHELL_PATH if it's set, otherwise default to `sh`. [1] https://lore.kernel.org/git/20240123005913.GB835964@coredump.intra.peff.net/ Signed-off-by: Josh Steadmon <steadmon@google.com> --- t/helper/test-run-command.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)