| Message ID | 9cf14984c0a71b1ccdff7db0699571bf5af1209b.1660143750.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Some fixes and an improvement for using CTest on Windows | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > While at it, enable the command trace via `-x` and verbose output via > `-v`, otherwise it would be near impossible to diagnose any problems. This sounds like a completely unrelated change. Do the tests behave so differently here, compared to how they are run in t/Makefile, where -vx is not the default? The only plausible reason I can think of that this change "while at it" is justifiable as part of the other change is if this is only ever used in the CI environment and no interactive human would be sitting in front of the session that runs tests here. If that is the case, I do agree it would make sense, since it would be much less convenient to go back to a failing test and rerun it with -v or -x or whatever. But that needs to be explained in the proposed log message. Thanks. > 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 > WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) > endforeach()
On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > In Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and > `--no-chain-lint`, mainly to win back some time caused by the serious > performance penalty paid for the tests relying so heavily on POSIX shell > scripting, which only works by using a POSIX emulation layer. > > Let's do the same when running the tests, say, in Visual Studio. > > While at it, enable the command trace via `-x` and verbose output via > `-v`, otherwise it would be near impossible to diagnose any problems. > > 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 > WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) > endforeach() Is this authored before a561962479c (cmake: fix CMakeLists.txt on Linux, 2022-05-24) was merged? The "say, in Visual Studio" seems to elide that we'll now run these tests differently when you run with cmake everywhere. It seems much better to pass some "test arguments" to cmake itself, which we'd then call from the ci specifically. Then e.g. a Linux user of cmake wouldn't wonder why the "make test" spots e.g. a chain-lint issue that the cmake testing would hide.
Hi Junio, On Wed, 10 Aug 2022, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > While at it, enable the command trace via `-x` and verbose output via > > `-v`, otherwise it would be near impossible to diagnose any problems. > > This sounds like a completely unrelated change. It may sound like it, but it is not. In order to make sense of the broken tests, I needed access to more verbose output than our test scripts provide by default. When running the test suite on the command-line, it is easy to tell the user "oh, if you need more information, just call the test script with these here options: ...". This is not an option when running the tests within Visual Studio. Does this clarify the intention and validity of the proposed patch? If so, I will gladly try my best to improve the commit message to explain that intention better. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Junio, > > On Wed, 10 Aug 2022, Junio C Hamano wrote: > >> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> >> writes: >> >> > While at it, enable the command trace via `-x` and verbose output via >> > `-v`, otherwise it would be near impossible to diagnose any problems. >> >> This sounds like a completely unrelated change. > > It may sound like it, but it is not. In order to make sense of the broken > tests, I needed access to more verbose output than our test scripts > provide by default. > > When running the test suite on the command-line, it is easy to tell the > user "oh, if you need more information, just call the test script with > these here options: ...". > > This is not an option when running the tests within Visual Studio. I gave an example of "CI environment it is cumbersome to go back and run only a single one" in the review you are responding to that may explain why such a change is needed, and you gave us exactly the context that was lacking here. The environment does not let users run the tests as anything but a single monolithic ball of wax. > Does this clarify the intention and validity of the proposed patch? If so, > I will gladly try my best to improve the commit message to explain that > intention better. It explains why -x -v is needed and needs to be a part of VS+CMake topic, and it will help readers to have it in the explanation. It still does not justify why it has to be a part of a step to omit bin-warppers and skip chain-lint that has nothing to do it, though. Thanks.
Hi Junio, On Tue, 16 Aug 2022, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > On Wed, 10 Aug 2022, Junio C Hamano wrote: > > > >> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > >> writes: > >> > >> > While at it, enable the command trace via `-x` and verbose output via > >> > `-v`, otherwise it would be near impossible to diagnose any problems. > >> > >> This sounds like a completely unrelated change. > > > > It may sound like it, but it is not. In order to make sense of the broken > > tests, I needed access to more verbose output than our test scripts > > provide by default. > > > > When running the test suite on the command-line, it is easy to tell the > > user "oh, if you need more information, just call the test script with > > these here options: ...". > > > > This is not an option when running the tests within Visual Studio. > > I gave an example of "CI environment it is cumbersome to go back and > run only a single one" in the review you are responding to that may > explain why such a change is needed, and you gave us exactly the > context that was lacking here. The environment does not let users > run the tests as anything but a single monolithic ball of wax. Good, I will amend the commit message. > > Does this clarify the intention and validity of the proposed patch? If so, > > I will gladly try my best to improve the commit message to explain that > > intention better. > > It explains why -x -v is needed and needs to be a part of VS+CMake > topic, and it will help readers to have it in the explanation. It > still does not justify why it has to be a part of a step to omit > bin-warppers and skip chain-lint that has nothing to do it, though. Oh, that ;-) To be honest, I reflexively do that on Windows because it saves _so much time_. I will amend the commit message to clarify that, too. Ciao, Dscho
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()