Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
- From
Victoria Dye <vdye@github.com>
- Date
- Sep 7, 2022, 22:10 UTC
- Message-ID
- <3df77ffd-85a2-3a54-9005-34a24ec6e82d@github.com>
- In-Reply-To
- <e00cb37b98ac09cff010e843ef19eeec761f8985.1661243463.git.gitgitgadget@gmail.com>
Johannes Schindelin via GitGitGadget wrote:
Show 9 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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?
[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.
Show 7 quoted lines
> > 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.
Show 16 quoted lines
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 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 -vxAs 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.
> WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
> endforeach()
>