Hi Victoria, On Wed, 7 Sep 2022, Victoria Dye wrote: > Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > > > When a test script fails in Git's test suite, the usual course of action > > is to re-run it using options to increase the verbosity of the output, > > e.g. `-v` and `-x`. > > > > Like in Git's CI runs, when running the tests in Visual Studio via the > > CTest route, it is cumbersome or at least requires a very unintuitive > > approach to pass options to the test scripts. > > At first I wondered whether there's a way to make arg specification more > intuitive, rather than silently changing defaults. Unfortunately, it looks > like even in the latest versions of CTest don't really support passing > arguments through to tests [1] (and the workarounds are unpleasant at best). > > But then, you mentioned that there *is* a cumbersome/unintuitive approach to > passing the options; what approach were you thinking? Thank you for pointing out that I assumed this to be more obvious than it actually is. The cumbersome way is to edit this part of the CMake definition (https://github.com/git/git/blob/v2.38.0/contrib/buildsystems/CMakeLists.txt#L1091-L1096): #test foreach(tsh ${test_scipts}) add_test(NAME ${tsh} COMMAND ${SH_EXE} ${tsh} WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) endforeach() To pass an option like `-v --run=1,29`, the user would have to edit the line `COMMAND ${SH_EXE} ${tsh}`, appending the options, and then re-configure the CMake cache. And then not forget to _only_ run the test script in question because those options do not make sense for the other tests. And then not forget to undo the changes and re-configure the CMake cache. > [1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470 > > > > > So let's just pass those options by default: This will not clutter any > > output window but the log that is written to a log file will have > > information necessary to figure out test failures. > > Makes sense, I don't see any harm in providing more verbose output by > default here. > > > > > While at it, also imitate what the Windows jobs in Git's CI runs do to > > accelerate running the test scripts: pass the `--no-bin-wrappers` and > > `--no-chain-lint` options. This makes the test runs noticeably faster > > because the `bin-wrappers/` scripts as well as the `chain-lint` code > > make heavy use of POSIX shell scripting, which is really, really slow on > > Windows due to the need to emulate POSIX behavior via the MSYS2 runtime. > > I'm a bit more hesitant on including these. I see how the performance > benefit (on Windows in particular) would make typical user experience nicer. > But, if someone develops locally with '--no-chain-lint' specified, they'll > be much more likely to miss a broken && chain (personally, I get caught by > chain lint errors *all the time* when I'm adding/updating tests) and cause > unnecessary churn in their patch submissions. So, my recommendation would be > to drop '--no-chain-lint' here (unless it's less important to the average > developer than I think it is). > > It's also possible that '--no-bin-wrappers' does something weird with their > local installation of Git but I think it's safe enough to make the default > if the performance gain is substantial. > > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > > --- > > contrib/buildsystems/CMakeLists.txt | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt > > index 1b23f2440d8..4aee1e24342 100644 > > --- a/contrib/buildsystems/CMakeLists.txt > > +++ b/contrib/buildsystems/CMakeLists.txt > > @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") > > #test > > foreach(tsh ${test_scipts}) > > add_test(NAME ${tsh} > > - COMMAND ${SH_EXE} ${tsh} > > + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx > > As you have it here, I don't think there's a way for a user to override > these defaults (unless there's something about the manual workaround you > mentioned earlier that allows it). Since a user could feasibly want to set > their own options, could you add a build variable to CMakeLists.txt like > 'GIT_TEST_OPTS'? You could use it to set the default options you have here, > but a user could still specify their own value at build time to override. Indeed, there is no way to override this, unless the user edits the (Git-tracked) `CMakeLists.txt` file and then re-configures the cache. In my experience, the most common scenario where a developer needs to pass options to test script is when they need to restrict execution to individual test cases via, say, `--run=1,29`. Such a use case cannot be covered via the CMake approach because it is not fine-grained enough: any options the user could set would apply to the entire test suite. I added a comment to the commit message advising users to run the test script manually via Git Bash instead, if they wish to pass a different set of command-line options to the test script. Thanks, Dscho > > > WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) > > endforeach() > > > >
