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

Commit Message

Johannes Schindelin Aug. 23, 2022, 8:30 a.m. UTC

From: Johannes Schindelin <[e-mail]>

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.

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.

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.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 contrib/buildsystems/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Victoria Dye Sept. 7, 2022, 10:10 p.m. UTC | #1

Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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()
>

Ævar Arnfjörð Bjarmason Sept. 8, 2022, 7:22 a.m. UTC | #2

On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[e-mail]>
>
> 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.
>
> 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.
>
> 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.
>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>  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
>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>  endforeach()

The -vx part of this looks sensible, and matches the commit message
$subject.

I think the "--no-bin-wrappers --no-chain-lint" and the "while at it"
here should be stripped out, and put into its own commit.

Which, as I commented on in the v1[1] is making the implicit assumption
that this cmake file is only used on Windows, but since 561962479c
(cmake: fix CMakeLists.txt on Linux, 2022-05-24) that isn't the case.

So, perhaps we should have a performance hack due to Windows's slowness,
but:

 A. It should be guarded by some "is windows?" check. Your commit
    message justifies why this is a good idea on Windows, but completely
    elides over the fact that this isn't Windows-specific code anymore.

 B. We can still build wind "make" or "cmake", I don't see a reason for
    why such a perf hack (if we decide to keep it) should depend on the
    build system.

 C. Since I sent [1] we've had submitted chainlint.pl in-flight
    series. It's partly trying to take special considerations to be fast
    on Windows. I don't think it's the case with that series that this
    needs to be skipped on Windows anymore (and if it is, Eric would
    like to know).

1.  https://lore.kernel.org/git/[e-mail]/

Eric Sunshine Sept. 28, 2022, 6:55 a.m. UTC | #3

On Thu, Sep 8, 2022 at 3:28 AM Ævar Arnfjörð Bjarmason <[e-mail]> wrote:
> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
> > 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`.
> > [...]
> > Signed-off-by: Johannes Schindelin <[e-mail]>
> > ---
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > +             COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>
> I think the "--no-bin-wrappers --no-chain-lint" and the "while at it"
> here should be stripped out, and put into its own commit.
>
> So, perhaps we should have a performance hack due to Windows's slowness,
> but:
>
>  C. Since I sent [1] we've had submitted chainlint.pl in-flight
>     series. It's partly trying to take special considerations to be fast
>     on Windows. I don't think it's the case with that series that this
>     needs to be skipped on Windows anymore (and if it is, Eric would
>     like to know).

I had the opportunity to test chainlint.pl on an 8-core machine using
both Linux and Windows 10. The machine is more than a few years old,
though not nearly as old as my own development machine. Although
chainlint.pl checked all test definitions in the entire project in
less than one _second_ on Linux, it took two _minutes_ on Windows 10
on the same machine (using all 8 cores). Despite the fact that Perl is
run just once to check all test definitions -- unlike chainlint.sed
running 'sed' tens of thousands of times, chainlint.pl is still
painfully slow on Windows, so I certainly understand the desire to use
--no-chain-lint, at least on Windows.

Johannes Schindelin Oct. 18, 2022, 2:02 p.m. UTC | #4

Hi Victoria,

On Wed, 7 Sep 2022, Victoria Dye wrote:

> Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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()
> >
>
>

Patch

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
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
 endforeach()