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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 18, 2022, 14:02 UTC
Message-ID: <29r609np-s0r1-r904-16o3-6918930o2ror@tzk.qr>
In-Reply-To: <3df77ffd-85a2-3a54-9005-34a24ec6e82d@github.com>

Hi Victoria,

On Wed, 7 Sep 2022, Victoria Dye wrote:

Show 18 quoted lines

> 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?

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.

Show 54 quoted lines

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

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

Show 6 quoted lines

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