Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs

[Victoria Dye <vdye@github.com>]

Johannes Schindelin via GitGitGadget wrote:
> 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.

> 
> 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@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 -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.

>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>  endforeach()
>